Closed Bug 979133 Opened 10 years ago Closed 10 years ago

add restyle debug logging

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(2 files, 2 obsolete files)

patch v2
64.00 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
sample output
1.56 KB, text/plain
Details
Attached patch patch (obsolete) — — Splinter Review 
I've added some debugging output to the restyling process for bug 931668, and I figure it might be useful to have it in the tree.
Attachment #8385109 - Flags: review?(dbaron)
Attached file sample output (obsolete) — 
Some sample output with the logging turned on.
I'm a little queasy about putting this on Document; I'd prefer not to have Document differ between debug and non-debug builds.  It seems better to put it on DOMWindowUtils, unconditionally, and then have it no-op in non-DEBUG builds.

Then again, roc would probably say that you should just use PR_LOGging rather than rolling your own conditional logging solution.  Would that work?x

I haven't looked at the details of the info being logged, yet, though.
Flags: needinfo?(cam)
Oh, I guess the ability to do the logging for just one document is a big advantage of the API over PR_LOG.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #3)
> I'm a little queasy about putting this on Document; I'd prefer not to have
> Document differ between debug and non-debug builds.  It seems better to put
> it on DOMWindowUtils, unconditionally, and then have it no-op in non-DEBUG
> builds.

If I put it in DOMWindowUtils will I still be able to call it from regular content?  I want to be able to add simple calls to enable/disable the logging from within a non-chrome document.

> Then again, roc would probably say that you should just use PR_LOGging
> rather than rolling your own conditional logging solution.  Would that work?x

I could change it to PR_LOG, though I can never remember the right environment variable syntax to turn it on.  Maybe my start/stopRestyleTracing can turn the PR_LOGging on/off?
Flags: needinfo?(cam) → needinfo?(dbaron)
(In reply to Cameron McCormack (:heycam) from comment #5)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #3)
> > I'm a little queasy about putting this on Document; I'd prefer not to have
> > Document differ between debug and non-debug builds.  It seems better to put
> > it on DOMWindowUtils, unconditionally, and then have it no-op in non-DEBUG
> > builds.
> 
> If I put it in DOMWindowUtils will I still be able to call it from regular
> content?  I want to be able to add simple calls to enable/disable the
> logging from within a non-chrome document.

I believe you could, since most (maybe even all) nsDOMWindowUtils public methods begin with:
  if (!nsContentUtils::IsCallerChrome()) {
    return NS_ERROR_DOM_SECURITY_ERR;
  }

You should probably run the idea past a dom peer, though.

> > Then again, roc would probably say that you should just use PR_LOGging
> > rather than rolling your own conditional logging solution.  Would that work?x
> 
> I could change it to PR_LOG, though I can never remember the right
> environment variable syntax to turn it on.  Maybe my
> start/stopRestyleTracing can turn the PR_LOGging on/off?

Not sure.  I'm not a huge fan of PR_LOGging either.
Flags: needinfo?(dbaron)
Is this worth resurrecting, perhaps with the above comments?
Flags: needinfo?(cam)
Yes, I'll resurrect this once I land bug 931668, as I keep needing to update the logging patch as I make changes there.
Depends on: 931668
Flags: needinfo?(cam)
Attached patch patch v2 — — Splinter Review 
I've decided to stay away from PR_LOG.  The logging is now done with a macro to avoid cluttering up the restyle methods too much.  I've also removed the DOM stuff; maybe I'll keep them in a separate patch linked to from an MDN page, for people who want to enable/disable restyle logging easily from a document.
Attachment #8385109 - Attachment is obsolete: true
Attachment #8385109 - Flags: review?(dbaron)
Attachment #8486809 - Flags: review?(dbaron)
Attached file sample output — 
This is from a change to the transform property on an element.
Attachment #8385110 - Attachment is obsolete: true
Comment on attachment 8486809 [details] [diff] [review]
patch v2

You should probably split the mfbt patch into a separate patch,
and request review from an mfbt peer.  That said, I think it
would make more sense to call the class AutoIncrement.

But see also bug 669234.
Flags: needinfo?(cam)
I had a slight concern about using AutoRestore since then my macro would be two lines, and obviously I can't brace them up into a statement block.  But I guess it's not too awful; I think I'll just do that.
Flags: needinfo?(cam)
Comment on attachment 8486809 [details] [diff] [review]
patch v2

It might be nice to make the #ifdef DEBUG in this patch conditional
on something else (RESTYLE_LOGGING), and then define RESTYLE_LOGGING
#ifdef DEBUG, to make things more visible.  (Need to make sure that
define is in a header included by everything that tests the condition,
though.)


>-        if (!oldContext->IsShared() && !newContext->IsShared()) {
>+        if (copyFromContinuation) {
>+          LOG_RESTYLE("not swapping style structs, since we copied from a "
>+                      "continuation");
>+        } else if (oldContext->IsShared()) {
>+          LOG_RESTYLE("not swapping style structs, since the old context is "
>+                      "shared");
>+        } else if (newContext->IsShared()) {
>+          LOG_RESTYLE("not swapping style structs, since the new context is "
>+                      "shared");
>+        } else {
>+          LOG_RESTYLE("swapping style structs between %p and %p",
>+                      oldContext.get(), newContext.get());


Adding the copyFromContinuation test here is a substantive
change that I suspect you didn't intend.  Or maybe you did intend
it, and intended it to land with 931668?


I'm a bit unhappy about RestyleHintToString and ChangeHintToString
duplicating the list of hints.  I think it might be better to use
the preprocessor for that, although I'm not crazy about the resulting
indirection either.  At the very least, you need comments in nsChangeHint.h.

They also use AppendPrintf unnecessarily, where they could just
use AppendLiteral.


I'd also prefer NS_ConvertUTF16toUTF8 for logging over
NS_LossyConvertUTF16toASCII; most developers' terminals these days
should be UTF-8, no?

Seems like RestyleTracker::ShouldLogRestyle() and
RestyleTracker::LoggingDepth() should be inline.  (At least unless
the compiler prevents it.)

>+  // XXX Asserting just on the children would be good.

This is fine, but probably could be in a separate patch.  Feel free
to land with my review, though.

In the STYLE_STRUCT_RESET and STYLE_STRUCT_INHERITED in
nsStyleContext.cpp, please fix the end-of-line \s.

nsStyleContext::DumpWithStructs should be next to nsStyleContext::List
in the .cpp file as well, and should probably be called ListWithStructs
for clarity.  Though List shows the rules which DumpWithStructs doesn't,
so maybe rename List to ListWithRules?

r=dbaron on everything except the mfbt bits.

(And I'm fine with switching to AutoRestore if you want to do that.)
Attachment #8486809 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #14)
> Comment on attachment 8486809 [details] [diff] [review]
> patch v2
> 
> >-        if (!oldContext->IsShared() && !newContext->IsShared()) {
> >+        if (copyFromContinuation) {
...
> Adding the copyFromContinuation test here is a substantive
> change that I suspect you didn't intend.  Or maybe you did intend
> it, and intended it to land with 931668?

No, not sure how that happened.  I'll revert that part.

> I'm a bit unhappy about RestyleHintToString and ChangeHintToString
> duplicating the list of hints.  I think it might be better to use
> the preprocessor for that, although I'm not crazy about the resulting
> indirection either.  At the very least, you need comments in nsChangeHint.h.

I'll add a comment now and file a bug to construct the enums from pre-processed files.

> I'd also prefer NS_ConvertUTF16toUTF8 for logging over
> NS_LossyConvertUTF16toASCII; most developers' terminals these days
> should be UTF-8, no?

Yep, sure.

> Seems like RestyleTracker::ShouldLogRestyle() and
> RestyleTracker::LoggingDepth() should be inline.  (At least unless
> the compiler prevents it.)

Yeah they should be; RestyleTracker.h and RestyleManager.h referencing each other would make that tricky, but I can stick the bodies in a RestyleTrackerInlines.h file.

> >+  // XXX Asserting just on the children would be good.
> 
> This is fine, but probably could be in a separate patch.  Feel free
> to land with my review, though.

Oops, that's an old comment; I already added a limited AssertStructsNotUsedElsewhere call in here.

> nsStyleContext::DumpWithStructs should be next to nsStyleContext::List
> in the .cpp file as well, and should probably be called ListWithStructs
> for clarity.  Though List shows the rules which DumpWithStructs doesn't,
> so maybe rename List to ListWithRules?

I didn't meant to leave DumpWithStructs in there; it's not used by the other logging code.  I'll remove it.

> r=dbaron on everything except the mfbt bits.
> 
> (And I'm fine with switching to AutoRestore if you want to do that.)

Yeah I'll do that.
David, do you know if there's a natural place on wiki.m.o or MDN to mention this?
Flags: needinfo?(dbaron)
(In reply to Cameron McCormack (:heycam) from comment #15)
> > nsStyleContext::DumpWithStructs should be next to nsStyleContext::List
> > in the .cpp file as well, and should probably be called ListWithStructs
> > for clarity.  Though List shows the rules which DumpWithStructs doesn't,
> > so maybe rename List to ListWithRules?
> 
> I didn't meant to leave DumpWithStructs in there; it's not used by the other
> logging code.  I'll remove it.

I'm going to re-add it, with some changes, in a new bug.
Blocks: 1072724
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/84a355b48a1a for -wError bustage on linux32 debug and static analysis.
https://hg.mozilla.org/mozilla-central/rev/d4e7b198391c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: