1162751 - Remove usage of |#ifdef PR_LOGGING| from image

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Eric Rahm [:erahm]]

In bug 1161238 we plan on removing |--disable-logging| which makes |#ifdef PR_LOGGING| redundant.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Part 1: Remove instances of #ifdef PR_LOGGING in image

PR_LOGGING is now always defined, we can remove #ifdefs checking for it.

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 2: Always disable pallete index checking

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.

Comment 3

[Eric Rahm [:erahm]]

Attachment: Part 3: Wrap expensive calls in PR_LOG_TEST

Check that logging is enabled before performing potentially expensive
operations.

Comment 4

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9ad197bc167

Comment 5

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8603471 [details] [diff] [review]
Part 2: Always disable pallete index checking

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

Looks good.

Comment 6

[Seth Fowler [:seth] [:s2h]]

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?

Comment 7

[Eric Rahm [:erahm]]

(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.

Comment 8

[Glenn Randers-Pehrson]

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.

Comment 9

[Glenn Randers-Pehrson]

"a few kb" == 9.5 kb increase in size of the bundled libpng (49k to 58.5k)

Comment 10

[Eric Rahm [:erahm]]

(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.

Comment 11

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6983be82c92b
https://hg.mozilla.org/integration/mozilla-inbound/rev/37f361e463b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9af70b6e6de

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6983be82c92b
https://hg.mozilla.org/mozilla-central/rev/37f361e463b3
https://hg.mozilla.org/mozilla-central/rev/f9af70b6e6de