1300544 - Useless declaration in layout/base/nsDisplayList.cpp nsDisplayFilter::PrintEffects

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

https://dxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?q=layout%2Fbase%2FnsDisplayList.cpp&redirect_type=direct#7020
This declaration is useless.

Reporting it as good first bug

Comment 1

[Aniket Vyas]

Hi, 

   I can take up this bug. Looks like we just need to get rid of the declaration. 

   What tests would I need to run on the patch ? 

Thanks,
Aniket

Comment 2

[Aniket Vyas]

Attachment: bug1300544.diff

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8788715 [details] [diff] [review]
bug1300544.diff

I don't know for the test, you could do a try run but the code is obvious enough that we should be fine.
Now, you should try to find a reviewer

Comment 4

[Aniket Vyas]

(In reply to Sylvestre Ledru [:sylvestre] from comment #3)
> Comment on attachment 8788715 [details] [diff] [review]
> bug1300544.diff
> 
> I don't know for the test, you could do a try run but the code is obvious
> enough that we should be fine.
> Now, you should try to find a reviewer

Ohh cool. And the best way to find a reviewer would be looking at the component owners ?

Comment 5

[Sylvestre Ledru [:Sylvestre]]

This explains all: 
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_-_Get_your_code_reviewed

Comment 6

[Aniket Vyas]

Perfect ! Thanks Sylvestre.

Comment 7

[Aniket Vyas]

Perfect ! Thanks Sylvestre.

Comment 8

[Aniket Vyas]

Hi Asa, 

   Can you please look at this change set. 

Thanks,
Aniket

Comment 9

[Asa Dotzler [:asa]]

I'm not the right person to look at this. Sorry.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8788715 [details] [diff] [review]
bug1300544.diff

BZ might not be the best reviewer here.

If you look here:
https://hg.mozilla.org/mozilla-central/annotate/ab70808cd4b6c6ad9a57a9f71cfa495fcea0aecd/layout/base/nsDisplayList.cpp#l7021
(hg blame on the file), you will see that the change has been introduced by this commit:
https://hg.mozilla.org/mozilla-central/annotate/102ceafb7343/layout/base/nsDisplayList.cpp#l6988 by cku

Comment 11

[u459114]

Comment on attachment 8788715 [details] [diff] [review]
bug1300544.diff

Review of attachment 8788715 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for me, but I am not layout/view peer. mstange, please help to review it.
 	
Aniket, you may refer to this page to find out a suitable one for reviewing:
https://wiki.mozilla.org/Modules/Core

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8788715 [details] [diff] [review]
bug1300544.diff

Thanks, Aniket.

Comment 13

[Aniket Vyas]

Thanks a lot guys for the help ! 

What should be the next steps for getting the code in ? 

1. Do I need to run any specific tests ? 
2. What would be the commit procedure ? 

Thanks,
Aniket

Comment 14

[Markus Stange [:mstange]]

In this case you do not need to run any tests, because this code is only used in debug logging output, which is not analyzed in tests, and because the change is so simple that it's very easy to see that it will not have any adverse effects.

Please attach the patch with the correct author + commit message, as described here:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
And then request checkin using the checkin-needed keyword on the bug, as described here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 15

[Aniket Vyas]

Attachment: bug1300544.diff

Comment 16

[Aniket Vyas]

Have updated the diff. Regarding the checkin-needed keyword, i only see "checkin" keyword. Probably I do not have the privileges to add the keyword ?

Comment 17

[Markus Stange [:mstange]]

The commit message should also list your reviewers. Add this at the end: r=cjku, r=mstange

I can add the checkin-needed keyword to the bug once you've done that.

Comment 18

[Aniket Vyas]

Attachment: bug1300544.diff

Comment 19

[Aniket Vyas]

Done. Thanks for the hand holding Markus.

Comment 20

[Markus Stange [:mstange]]

Thanks!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93cf1a5af6a0
Remove the useless declaration in layout/base/nsDisplayList.cpp <nsDisplayFilter::PrintEffects>. r=cjku, r=mstange

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93cf1a5af6a0

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93cf1a5af6a0