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.

Comment on attachment 691789 [details] [diff] [review] v3.patch this patch conflicts with bug 781409 which removes nsITableLayout all together.

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.

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

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.

Comment on attachment 691789 [details] [diff] [review] v3.patch uhm, you should update this patch so it applies.

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

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.

updating repository; resolving build warnings

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)

Checking if >=0 and the do the castings

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

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.