1016404 - Defining APZC_LOG in multiple .cpp files doesn't play well with unified builds

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Ran into this problem on a local build. If APZCTreeManager.cpp and AsyncPanZoomController.cpp are built in the same unified compilation unit it errors out because APZ_LOG is defined twice. This might only happen if you actually have logging enabled, I'm not sure.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Since we want to allow enabling these loggings on a per-file basis, it seems like the best solution is to just rename one of them. I suggest renaming APZC_LOG to APZCTM_LOG in APZCTreeManager.cpp. (This approach also makes it consistent with AEM_LOG in ActiveElementManager).

Comment 2

[Sayan Paul]

How to get started with this bug?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Hi Sayan, the first thing to do is to get a build environment set up where you can compile Firefox and run it. There are instructions on how to do this at https://developer.mozilla.org/en/docs/Simple_Firefox_build

Once you have that done, you can start working on the patch for this bug. Just change all the APZC_LOG stuff in gfx/layers/apz/src/APZCTreeManager.cpp to APZCTM_LOG.

Then generate a patch for the changes and attach it to this bug for review following the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Feel free to post here with any questions you have, or join us on IRC (#gfx) or (#introduction).

Comment 4

[Sayan Paul]

ok then i'll be working on this bug

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Great! I've assigned the bug to you.

Comment 6

[Sayan Paul]

Attachment: Required changes as mentioned in problem description.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8433347 [details] [diff] [review]
Required changes as mentioned in problem description.

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

This looks good! Two small changes and it should be ready for landing. One is to update the commit message to describe what the patch is doing, rather than re-stating the bug title. In this case something like "Bug 1016404 - Rename APZC_LOG in APZCTreeManager to avoid unified build conflicts. r=kats" would be good. (The r=kats indicates that I've reviewed it). The other change is below.

Please upload an updated patch with these two changes and we can get it landed in the tree. Thanks!

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +26,5 @@
>  
>  #include <algorithm>                    // for std::stable_sort
>  
> +#define APZCTM_LOG(...)
> +// #define APZCTM_LOG(...) printf_stderr("APZC: " __VA_ARGS__)

please change the "APZC: " string here to "APZCTM: " as well; sorry I forgot to mention that earlier.

Comment 8

[Sayan Paul]

Attachment: Bug 1016404 - Rename APZC_LOG in APZCTreeManager to avoid unified build conflicts. r=kats

Comment 9

[Sayan Paul]

Attachment: Bug 1016404 - Rename APZC_LOG in APZCTreeManager to avoid unified build conflicts. r=kats

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8434316 [details] [diff] [review]
Bug 1016404 - Rename APZC_LOG in APZCTreeManager to avoid unified build conflicts. r=kats

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

Perfect, thanks! I will land this change as soon as the tree has reopened.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/b2g-inbound/rev/c8dc973cc733

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/c8dc973cc733

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Nice work Sayan. Let me know if you'd like me to suggest another bug for you!

Comment 14

[Sayan Paul]

Yes I would like to work on other bugs

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Sayan, bug 1001582 might be a good one to try next - a little bit harder than this one but not by that much.