Closed Bug 1162751 Opened 5 years ago Closed 5 years ago

Remove usage of |#ifdef PR_LOGGING| from image

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(3 files)

In bug 1161238 we plan on removing |--disable-logging| which makes |#ifdef PR_LOGGING| redundant.
PR_LOGGING is now always defined, we can remove #ifdefs checking for it.
Attachment #8603469 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
The original assumption was that PR_LOGGING wouldn't be enabled in release
builds. Considering that this warning isn't actionable and PR_LOGGING is now
always defined it's better to just always disable pallete index checking.
Attachment #8603471 - Flags: review?(seth)
Check that logging is enabled before performing potentially expensive
operations.
Attachment #8603580 - Flags: review?(seth)
Comment on attachment 8603471 [details] [diff] [review]
Part 2: Always disable pallete index checking

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

Looks good.
Attachment #8603471 - Flags: review?(seth) → review+
Comment on attachment 8603580 [details] [diff] [review]
Part 3: Wrap expensive calls in PR_LOG_TEST

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

Verbose code gets even more verbose. Sigh. Thanks for doing this, though.

::: image/src/imgRequest.cpp
@@ +258,5 @@
>        if (mLoader) {
>          mLoader->SetHasNoProxies(this, mCacheEntry);
>        }
>      }
> +    else if (PR_LOG_TEST(GetImgLog(), PR_LOG_DEBUG)) {

Can you do me a favor and move the |else| up onto the previous line to bring this more in line with the style in the rest of the file?
Attachment #8603580 - Flags: review?(seth) → review+
(In reply to Seth Fowler [:seth] from comment #6)
> Can you do me a favor and move the |else| up onto the previous line to bring
> this more in line with the style in the rest of the file?

I'll update before I push.
Part 2 looks OK to me.  There is a check for PR_LOGGING in media/libpng/pnglibconf.h as well, which I can take care of if you like.  Removing it will increase the size of libpng by a few kb.
Flags: needinfo?(seth)
"a few kb" == 9.5 kb increase in size of the bundled libpng (49k to 58.5k)
Attachment #8603469 - Flags: review?(nfroyd) → review+
(In reply to Glenn Randers-Pehrson from comment #8)
> Part 2 looks OK to me.  There is a check for PR_LOGGING in
> media/libpng/pnglibconf.h as well, which I can take care of if you like. 
> Removing it will increase the size of libpng by a few kb.

Thanks for looking into that Glenn! I'll cc you and seth when I start cleaning up the media/ directory.
Flags: needinfo?(seth)
You need to log in before you can comment on or make changes to this bug.