Closed Bug 1162673 Opened 5 years ago Closed 5 years ago

Remove usage of |#ifdef PR_LOGGING| from layout

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

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

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(2 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 #8602908 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Check that logging is enabled before performing potentially expensive
operations.
Attachment #8602909 - Flags: review?(nfroyd)
Attachment #8602908 - Flags: review?(nfroyd) → review+
Comment on attachment 8602909 [details] [diff] [review]
Part 2: Wrap expensive calls in PR_LOG_TEST

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

The nsPrintEngine.cpp change looks fine; I think the LogTextPerfStats PR_LOG_TEST might be better pushed to the callers to make it a little more obvious.  Asking dholbert to see what he thinks
Attachment #8602909 - Flags: review?(nfroyd)
Attachment #8602909 - Flags: review?(dholbert)
Attachment #8602909 - Flags: review+
Comment on attachment 8602909 [details] [diff] [review]
Part 2: Wrap expensive calls in PR_LOG_TEST

Punting to jdaggett who added LogTextPerfStats (in bug 934710) and hence can comment on it better than I can.

(The patch looks fine to me, though. Note that if we were to move the PR_LOG_TEST call out to the callers, per comment 4, we'd also have to move the "if (...)  { logLevel = PR_LOG_DEBUG }" customization out to each of the callers, too, which seems bad from a code duplication perspective.)
Attachment #8602909 - Flags: review?(dholbert) → review?(jdaggett)
(In reply to Daniel Holbert [:dholbert] from comment #5)
> (The patch looks fine to me, though. Note that if we were to move the
> PR_LOG_TEST call out to the callers, per comment 4, we'd also have to move
> the "if (...)  { logLevel = PR_LOG_DEBUG }" customization out to each of the
> callers, too, which seems bad from a code duplication perspective.)

Yes, this was my conclusion as well.
Comment on attachment 8602909 [details] [diff] [review]
Part 2: Wrap expensive calls in PR_LOG_TEST

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

r+ for the LogTextPerfStats portion
Attachment #8602909 - Flags: review?(jdaggett) → review+
https://hg.mozilla.org/mozilla-central/rev/520a0ded5a2f
https://hg.mozilla.org/mozilla-central/rev/d8977ecb8d2a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.