821269 - Fix build warnings in accessible/src/html

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Catalin Iordache]

Attachment: v3.patch

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

Comment 1

[Catalin Iordache]

I'm a beginner developer and this is my first contribution to the Mozilla codebase.

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 691789 [details] [diff] [review]
v3.patch

this patch conflicts with bug 781409 which removes nsITableLayout all together.

Comment 3

[Catalin Iordache]

so should i get over it because my patch conflicts with bug 781409 ?

Comment 4

[Trevor Saunders (:tbsaunde)]

(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.

Comment 6

[alexander :surkov (:asurkov)]

btw, please use -p -U 8 options when you create a patch (https://developer.mozilla.org/en/docs/Creating_a_patch)

Comment 7

[Catalin Iordache]

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 ?

Comment 8

[alexander :surkov (:asurkov)]

(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.

Comment 9

[Trevor Saunders (:tbsaunde)]

(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.

Comment 10

[Trevor Saunders (:tbsaunde)]

Comment on attachment 691789 [details] [diff] [review]
v3.patch

uhm, you should update this patch so it applies.

Comment 11

[Catalin Iordache]

Attachment: p1v1.patch

Remove build warnings in accessible/src/html/HTMLTableAccessible.cpp , without modifying any other files

Comment 12

[Trevor Saunders (:tbsaunde)]

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.

Comment 13

[Catalin Iordache]

Attachment: p1v2.patch

updating repository; resolving build warnings

Comment 14

[Trevor Saunders (:tbsaunde)]

roc, is there a reason nsITableCellLayout::Get Row / Column Index() return signed values? or just that nsITableCellLayout is silly?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Signed values are sometimes easier/safer to work with. You're less likely to accidentally wrap around.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

But basically I don't know.

Comment 17

[Trevor Saunders (:tbsaunde)]

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)

Comment 18

[Catalin Iordache]

Attachment: bug-821269-fixed

Checking if >=0 and the do the castings

Comment 19

[Trevor Saunders (:tbsaunde)]

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

Comment 20

[Catalin Iordache]

and should I put your name in the commit ( like r = Trevor Saunders) ?

Comment 21

[Trevor Saunders (:tbsaunde)]

(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.

Comment 22

[Catalin Iordache]

Attachment: bug-821269-fixed final

Made the changes that you asked for.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1035217debb3

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1035217debb3