Closed Bug 1016404 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: kats, Assigned: sayan_666)

Details

(Whiteboard: [mentor=kats][good first bug][lang=c++])

Attachments

(1 file, 2 obsolete files)

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.
Summary: Defining APZ_LOG in multiple .cpp files doesn't play well with unified logging → Defining APZC_LOG in multiple .cpp files doesn't play well with unified logging
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).
Whiteboard: [mentor=kats][good first bug][lang=c++]
How to get started with this bug?
Flags: needinfo?(bugmail.mozilla)
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).
Flags: needinfo?(bugmail.mozilla)
ok then i'll be working on this bug
Great! I've assigned the bug to you.
Assignee: nobody → sayan_666
Attachment #8433347 - Flags: review?(bugmail.mozilla)
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.
Attachment #8433347 - Flags: review?(bugmail.mozilla) → review+
Summary: Defining APZC_LOG in multiple .cpp files doesn't play well with unified logging → Defining APZC_LOG in multiple .cpp files doesn't play well with unified builds
Attachment #8433347 - Attachment is obsolete: true
Attachment #8434313 - Flags: review?(bugmail.mozilla)
Attachment #8434313 - Attachment is obsolete: true
Attachment #8434313 - Flags: review?(bugmail.mozilla)
Attachment #8434316 - Flags: review?(bugmail.mozilla)
Status: NEW → ASSIGNED
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.
Attachment #8434316 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/c8dc973cc733
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Nice work Sayan. Let me know if you'd like me to suggest another bug for you!
Yes I would like to work on other bugs
Sayan, bug 1001582 might be a good one to try next - a little bit harder than this one but not by that much.
You need to log in before you can comment on or make changes to this bug.