Last Comment Bug 821269 - Fix build warnings in accessible/src/html
: Fix build warnings in accessible/src/html
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: mozilla20
Assigned To: Catalin Iordache
:
:
Mentors:
: 821111 (view as bug list)
Depends on:
Blocks: 819664
  Show dependency treegraph
 
Reported: 2012-12-13 05:26 PST by Catalin Iordache
Modified: 2013-08-04 11:54 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v3.patch (3.87 KB, patch)
2012-12-13 05:26 PST, Catalin Iordache
no flags Details | Diff | Splinter Review
p1v1.patch (2.88 KB, patch)
2012-12-17 08:51 PST, Catalin Iordache
no flags Details | Diff | Splinter Review
p1v2.patch (2.96 KB, patch)
2012-12-17 13:57 PST, Catalin Iordache
no flags Details | Diff | Splinter Review
bug-821269-fixed (2.64 KB, patch)
2012-12-18 18:29 PST, Catalin Iordache
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
bug-821269-fixed final (2.51 KB, patch)
2012-12-19 18:12 PST, Catalin Iordache
no flags Details | Diff | Splinter Review

Description Catalin Iordache 2012-12-13 05:26:10 PST
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
Comment 1 Catalin Iordache 2012-12-13 05:27:17 PST
I'm a beginner developer and this is my first contribution to the Mozilla codebase.
Comment 2 Trevor Saunders (:tbsaunde) 2012-12-13 08:00:38 PST
Comment on attachment 691789 [details] [diff] [review]
v3.patch

this patch conflicts with bug 781409 which removes nsITableLayout all together.
Comment 3 Catalin Iordache 2012-12-13 08:21:29 PST
so should i get over it because my patch conflicts with bug 781409 ?
Comment 4 Trevor Saunders (:tbsaunde) 2012-12-13 08:32:12 PST
(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 5 Trevor Saunders (:tbsaunde) 2012-12-13 14:49:56 PST
*** Bug 821111 has been marked as a duplicate of this bug. ***
Comment 6 alexander :surkov 2012-12-13 21:40:33 PST
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 2012-12-14 00:01:08 PST
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 2012-12-15 05:24:26 PST
(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) 2012-12-17 06:48:31 PST
(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) 2012-12-17 08:11:54 PST
Comment on attachment 691789 [details] [diff] [review]
v3.patch

uhm, you should update this patch so it applies.
Comment 11 Catalin Iordache 2012-12-17 08:51:18 PST
Created attachment 692973 [details] [diff] [review]
p1v1.patch

Remove build warnings in accessible/src/html/HTMLTableAccessible.cpp , without modifying any other files
Comment 12 Trevor Saunders (:tbsaunde) 2012-12-17 09:53:11 PST
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 2012-12-17 13:57:35 PST
Created attachment 693098 [details] [diff] [review]
p1v2.patch

updating repository; resolving build warnings
Comment 14 Trevor Saunders (:tbsaunde) 2012-12-18 02:24:51 PST
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) 2012-12-18 02:36:04 PST
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) 2012-12-18 02:36:37 PST
But basically I don't know.
Comment 17 Trevor Saunders (:tbsaunde) 2012-12-18 05:03:59 PST
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 2012-12-18 18:29:00 PST
Created attachment 693711 [details] [diff] [review]
bug-821269-fixed

Checking if >=0 and the do the castings
Comment 19 Trevor Saunders (:tbsaunde) 2012-12-19 11:48:39 PST
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 2012-12-19 13:26:04 PST
and should I put your name in the commit ( like r = Trevor Saunders) ?
Comment 21 Trevor Saunders (:tbsaunde) 2012-12-19 13:57:58 PST
(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 2012-12-19 18:12:14 PST
Created attachment 694179 [details] [diff] [review]
bug-821269-fixed final

Made the changes that you asked for.
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-12-23 08:24:22 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/1035217debb3
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-12-23 13:05:11 PST
https://hg.mozilla.org/mozilla-central/rev/1035217debb3

Note You need to log in before you can comment on or make changes to this bug.