Closed Bug 1371457 Opened 7 years ago Closed 7 years ago

Stylo: DevTools failures because "Styles" profiler markers for restyling aren't emitted

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(3 files)

Bug 1371457 - Change restyle marker data to animation state.
59 bytes, text/x-review-board-request
bholley
: review+
gregtatum
: review+
Details
Bug 1371457 - Add restyle markers for Stylo.
59 bytes, text/x-review-board-request
bholley
: review+
gregtatum
: review+
Details
Bug 1371457 - Update animation restyle tests.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Gecko emits "Styles" profiler markers[1] when restyles are happening. Stylo doesn't appear to do so thus far. This causes failures in tests like: * devtools/server/tests/browser/browser_markers-styles.js[2] which expects the markers to exist and have a restyle hint. [1]: http://searchfox.org/mozilla-central/search?q=%22Styles%22&case=true&regexp=false&path=layout [2]: https://treeherder.mozilla.org/logviewer.html#?job_id=105301401&repo=try&lineNumber=87046
:bholley, do you intend for Stylo to expose profiler markers during restyle like Gecko does? (I realize Stylo may have different associated data to report than Gecko's restyle hint, but let's first work out if we're going to report the markers at all.)
Flags: needinfo?(bobbyholley)
According to :mstange, the perf.html UI doesn't surface these markers currently, so perhaps we could live without them if we want to. (The old profiler in DevTools does still use them, but we could probably live with that disappearing for a time, if needed.)
Is this a dup of bug 1344668?
Ah thanks, I forgot about that! I guess I'll depend on that bug, and leave this for the test failure specifically, which we may decide to disable for stylo or something (if we don't emit the data). Looks like :bholley was consulted in the other big, so clearing ni? here.
Depends on: 1344668
Flags: needinfo?(bobbyholley)
Summary: Stylo: "Styles" profiler markers for restyling aren't emitted → Stylo: DevTools failures because "Styles" profiler markers for restyling aren't emitted
Well, hang on. That bug is about attaching restyle hints to the profile markers, which is different than emitting the profile markers themselves. We should totally emit the markers, since we want the devtools timeline to remain useful. Can we just strip out the restyle from the API and emit the plain markers?
Flags: needinfo?(jryans)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5) > Well, hang on. That bug is about attaching restyle hints to the profile > markers, which is different than emitting the profile markers themselves. We > should totally emit the markers, since we want the devtools timeline to > remain useful. Can we just strip out the restyle from the API and emit the > plain markers? Ah yes, I agree it would be good to at least emit the markers first, and then we can consider the associated data more deeply in bug 1344668. This particular test _does_ also check for the hint (in addition to the marker), but it seems reasonable to make that conditional on Gecko mode (or remove it from the test) until we've worked what associated restyle data makes sense to expose for Stylo. It looks like it should pretty straightforward to record the markers without the hints. My code search in comment 0 was for the wrong type of marker. We'd want to sprinkle start and stop RestyleTimelineMarkers[1] without the hint data to accomplish this. [1]: http://searchfox.org/mozilla-central/search?q=RestyleTimelineMarker&case=true&regexp=false&path=
Flags: needinfo?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6) > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5) > > Well, hang on. That bug is about attaching restyle hints to the profile > > markers, which is different than emitting the profile markers themselves. We > > should totally emit the markers, since we want the devtools timeline to > > remain useful. Can we just strip out the restyle from the API and emit the > > plain markers? > > Ah yes, I agree it would be good to at least emit the markers first, and > then we can consider the associated data more deeply in bug 1344668. > > This particular test _does_ also check for the hint (in addition to the > marker), but it seems reasonable to make that conditional on Gecko mode (or > remove it from the test) until we've worked what associated restyle data > makes sense to expose for Stylo. I think we should just stop exposing restyle hints period, as discussed in the other bug. > It looks like it should pretty straightforward to record the markers without > the hints. Yep, I think we basically just want to stick the markers in ServoStyleSet::PrepareAndTraverseSubtree and call it a day.
Assignee: nobody → jryans
Priority: -- → P2
No longer depends on: 1344668
See Also: → 1344668
Comment on attachment 8881915 [details] Bug 1371457 - Change restyle marker data to animation state. https://reviewboard.mozilla.org/r/152980/#review158142
Attachment #8881915 - Flags: review?(bobbyholley) → review+
Comment on attachment 8881916 [details] Bug 1371457 - Add restyle markers for Stylo. https://reviewboard.mozilla.org/r/152982/#review158146 Per IRL discussion, let's add markers for the post-traversal too.
Attachment #8881916 - Flags: review?(bobbyholley) → review-
Comment on attachment 8881915 [details] Bug 1371457 - Change restyle marker data to animation state. :bholley should re-review this patch, as it has changed quite a bit.
Attachment #8881915 - Flags: review+ → review?(bobbyholley)
Comment on attachment 8881915 [details] Bug 1371457 - Change restyle marker data to animation state. https://reviewboard.mozilla.org/r/152980/#review158570
Attachment #8881915 - Flags: review?(bobbyholley) → review+
Comment on attachment 8881916 [details] Bug 1371457 - Add restyle markers for Stylo. https://reviewboard.mozilla.org/r/152982/#review158582 Per discussion: * RAII class * Don't overlap the StyleDocument with ProcessPostTraversal * Separate marker for change hint processing.
Attachment #8881916 - Flags: review?(bobbyholley) → review-
Comment on attachment 8882238 [details] Bug 1371457 - Update animation restyle tests. https://reviewboard.mozilla.org/r/153332/#review158584 Really thanks for doing this animation thing! ::: dom/animation/test/chrome/test_restyles.html:136 (Diff revision 1) > ok(animation.isRunningOnCompositor); > > div.animationDuration = '200s'; > > var markers = await observeStyling(5); > - is(markers.length, 0, > + // SMIL restyles are triggered for this case, leading to a non-zero count. As discussed, we have no idea the extra restyles is come from SMIL or not, so please drop the SMIL word and file a bug and leave the bug number here.
Attachment #8882238 - Flags: review?(hikezoe) → review+
Comment on attachment 8882238 [details] Bug 1371457 - Update animation restyle tests. https://reviewboard.mozilla.org/r/153332/#review158584 > As discussed, we have no idea the extra restyles is come from SMIL or not, so please drop the SMIL word and file a bug and leave the bug number here. Looks like this actually passes on try (it fails locally due to a known macOS issue), so I'll revert this part.
Comment on attachment 8881916 [details] Bug 1371457 - Add restyle markers for Stylo. https://reviewboard.mozilla.org/r/152982/#review158616
Attachment #8881916 - Flags: review?(bobbyholley) → review+
Comment on attachment 8881915 [details] Bug 1371457 - Change restyle marker data to animation state. https://reviewboard.mozilla.org/r/152980/#review159566 It all looks reasonable to me.
Attachment #8881915 - Flags: review?(gtatum) → review+
Comment on attachment 8881916 [details] Bug 1371457 - Add restyle markers for Stylo. https://reviewboard.mozilla.org/r/152982/#review159952 Works for me!
Attachment #8881916 - Flags: review?(gtatum) → review+
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/80679a3f2961 Change restyle marker data to animation state. r=bholley,gregtatum https://hg.mozilla.org/integration/autoland/rev/4128cdabed05 Add restyle markers for Stylo. r=bholley,gregtatum https://hg.mozilla.org/integration/autoland/rev/91c446afc7e2 Update animation restyle tests. r=hiro
Depends on: 1379516
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: