Closed Bug 821269 Opened 7 years ago Closed 7 years ago

Fix build warnings in accessible/src/html

Categories

(Core :: Disability Access APIs, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: catalinn.iordache, Assigned: catalinn.iordache)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch v3.patch (obsolete) — Splinter Review
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:17.0) Gecko/20100101 Firefox/17.0
Build ID: 20121129151900

Steps to reproduce:

I resolve some build warnings in HTMLTableAccessible.cpp  and also change the signature of the function GetCellDataAt in layout/tables/nsITableLayout.h
I'm a beginner developer and this is my first contribution to the Mozilla codebase.
Attachment #691789 - Flags: review?
Component: Untriaged → Disability Access APIs
Product: Firefox → Core
Hardware: x86 → All
Version: 17 Branch → unspecified
Comment on attachment 691789 [details] [diff] [review]
v3.patch

this patch conflicts with bug 781409 which removes nsITableLayout all together.
Attachment #691789 - Flags: review?
so should i get over it because my patch conflicts with bug 781409 ?
(In reply to Catalin Iordache from comment #3)
> so should i get over it because my patch conflicts with bug 781409 ?

please provide a new patch once that bug lands if there are still warnings.
Duplicate of this bug: 821111
btw, please use -p -U 8 options when you create a patch (https://developer.mozilla.org/en/docs/Creating_a_patch)
Status: UNCONFIRMED → NEW
Ever confirmed: true
alexdander sorry for that. I know the command, but i think i rushed . Also cand you look at the other bug i tried to resolve 821396 ?
(In reply to Catalin Iordache from comment #7)
> alexdander sorry for that. I know the command, but i think i rushed .

don't be sorry, just fix that next time :)

> Also
> cand you look at the other bug i tried to resolve 821396 ?

You shouldn't leave review request empty, always try to find specific person for review. I'm not a peer of content module so I asked Olli for review.
(In reply to Catalin Iordache from comment #3)
> so should i get over it because my patch conflicts with bug 781409 ?

that bug landed a couple days ago so you could work on this now if you like.
Attachment #691789 - Flags: review?(trev.saunders)
Comment on attachment 691789 [details] [diff] [review]
v3.patch

uhm, you should update this patch so it applies.
Attachment #691789 - Flags: review?(trev.saunders)
Attached patch p1v1.patch (obsolete) — Splinter Review
Remove build warnings in accessible/src/html/HTMLTableAccessible.cpp , without modifying any other files
Attachment #691789 - Attachment is obsolete: true
Attachment #692973 - Flags: review?(trev.saunders)
Comment on attachment 692973 [details] [diff] [review]
p1v1.patch

>diff --git a/accessible/src/html/HTMLTableAccessible.cpp b/accessible/src/html/HTMLTableAccessible.cpp
>--- a/accessible/src/html/HTMLTableAccessible.cpp
>+++ b/accessible/src/html/HTMLTableAccessible.cpp
>@@ -510,18 +510,18 @@ HTMLTableAccessible::SelectedCellCount()
>     for (uint32_t colIdx = 0; colIdx < colCount; colIdx++) {
>       nsresult rv = tableLayout->GetCellDataAt(rowIdx, colIdx,
>                                                *getter_AddRefs(domElement),

it looks like you need to update your tree, tableLayout doesn't exist anymore.
Attachment #692973 - Flags: review?(trev.saunders)
Attached patch p1v2.patch (obsolete) — Splinter Review
updating repository; resolving build warnings
Attachment #692973 - Attachment is obsolete: true
Attachment #693098 - Flags: review?(trev.saunders)
roc, is there a reason nsITableCellLayout::Get Row / Column Index() return signed values? or just that nsITableCellLayout is silly?
Signed values are sometimes easier/safer to work with. You're less likely to accidentally wrap around.
Comment on attachment 693098 [details] [diff] [review]
p1v2.patch

>-  uint32_t count = 0, rowCount = RowCount(), colCount = ColCount();
>-  for (uint32_t rowIdx = 0; rowIdx < rowCount; rowIdx++) {
>-    for (uint32_t colIdx = 0; colIdx < colCount; colIdx++) {
>+  int32_t count = 0, rowCount = RowCount(), colCount = ColCount();
>+  for (int32_t rowIdx = 0; rowIdx < rowCount; rowIdx++) {
>+    for (int32_t colIdx = 0; colIdx < colCount; colIdx++) {

this is wrong, you're just trading a signed / unsigned comparison for a implicit unsigned -> signed conversion.

I'd think ideally we'd change nsTableCellFrame, but I'd be fine with checking that it returns a value > 0 and casting that for now.

(same comment applies toe below chunks too)
Attachment #693098 - Flags: review?(trev.saunders)
Attached patch bug-821269-fixed (obsolete) — Splinter Review
Checking if >=0 and the do the castings
Attachment #693098 - Attachment is obsolete: true
Attachment #693711 - Flags: review?(trev.saunders)
Comment on attachment 693711 [details] [diff] [review]
bug-821269-fixed

>+      if(startRow >= 0 && startCol >= 0)
>+        if ((uint32_t)startRow == rowIdx && (uint32_t)startCol == colIdx)
>+          count++;

please put them in the same if so
if (startCol >= 0 && uint32_t(startCol) == col && startRow >= 0 && uint32_t(startRow) == row)

here and below.

r=me with that
Attachment #693711 - Flags: review?(trev.saunders) → review+
and should I put your name in the commit ( like r = Trevor Saunders) ?
(In reply to Catalin Iordache from comment #20)
> and should I put your name in the commit ( like r = Trevor Saunders) ?

bug XXX - fix something or other r=tbsaunde is the usual sort of thing.
Made the changes that you asked for.
Attachment #693711 - Attachment is obsolete: true
Attachment #694179 - Flags: review?(trev.saunders)
Attachment #694179 - Flags: review?(trev.saunders)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1035217debb3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.