Fix build warnings in accessible/src/html

RESOLVED FIXED in mozilla20

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Catalin Iordache, Assigned: Catalin Iordache)

Tracking

unspecified
mozilla20
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 691789 [details] [diff] [review]
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
(Assignee)

Comment 1

5 years ago
I'm a beginner developer and this is my first contribution to the Mozilla codebase.
(Assignee)

Updated

5 years ago
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?
(Assignee)

Comment 3

5 years ago
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

Comment 6

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

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 7

5 years ago
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

5 years ago
(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.
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 11

5 years ago
Created attachment 692973 [details] [diff] [review]
p1v1.patch

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)
(Assignee)

Comment 13

5 years ago
Created attachment 693098 [details] [diff] [review]
p1v2.patch

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.
But basically I don't know.
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)
(Assignee)

Comment 18

5 years ago
Created attachment 693711 [details] [diff] [review]
bug-821269-fixed

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+
(Assignee)

Comment 20

5 years ago
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.
(Assignee)

Comment 22

5 years ago
Created attachment 694179 [details] [diff] [review]
bug-821269-fixed final

Made the changes that you asked for.
Attachment #693711 - Attachment is obsolete: true
Attachment #694179 - Flags: review?(trev.saunders)
(Assignee)

Updated

5 years ago
Attachment #694179 - Flags: review?(trev.saunders)
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/1035217debb3
Assignee: nobody → catalinn.iordache
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1035217debb3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Blocks: 819664
You need to log in before you can comment on or make changes to this bug.