Closed Bug 979782 Opened 7 years ago Closed 3 years ago

Consider implementing lazy frame construction for display:contents descendants

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: mats, Assigned: emilio)

References

Details

(Keywords: perf)

Attachments

(2 files)

Bug 979782: Fixup FindFrameForContentSibling to don't duplicate work and trigger assertions for display: contents.
59 bytes, text/x-review-board-request
mats
: review+
Details
Bug 979782: Enable lazy frame construction for display: contents direct descendants.
59 bytes, text/x-review-board-request
mats
: review+
Details 
No description provided.
Summary: Consider implementing lazy frame construction for display-box:contents descendants → Consider implementing lazy frame construction for display:contents descendants
See Also: → 1338678
Attachment #8896213 - Flags: review?(cam)
Gah, r?heycam because bz nor mats are accepting review requests. Cam, see the discussion in bug 1338678, where nobody had a clear reason this couldn't be done.
Attachment #8896213 - Flags: review?(cam) → review?(mats)
Comment on attachment 8896213 [details]
Bug 979782: Fixup FindFrameForContentSibling to don't duplicate work and trigger assertions for display: contents.

https://reviewboard.mozilla.org/r/167506/#review199810

r=mats
Attachment #8896213 - Flags: review?(mats) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 27833f71028a -d 90a9bac24763: rebasing 431063:27833f71028a "Bug 979782: Enable lazy frame construction for display: contents direct descendants. r=mats" (tip)
merging layout/base/nsCSSFrameConstructor.cpp
warning: conflicts while merging layout/base/nsCSSFrameConstructor.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/369a5330ddec
Enable lazy frame construction for display: contents direct descendants. r=mats
Backed out for failing Linux crashtests at layout/style/crashtests/1402472.html assertion count 2 is more than expected 0 assertions:

https://hg.mozilla.org/integration/mozilla-inbound/rev/adba88e0504d1fd3efd6444b77ef9bb8238211f5

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=369a5330ddec3dcbd1c09c0665f906e50c6dd69c&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=140996790&repo=mozilla-inbound
> REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/reftest/tests/layout/style/crashtests/1402472.html | assertion count 2 is more than expected 0 assertions
Flags: needinfo?(emilio)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #7)
> Backed out for failing Linux crashtests at
> layout/style/crashtests/1402472.html assertion count 2 is more than expected
> 0 assertions:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> adba88e0504d1fd3efd6444b77ef9bb8238211f5
> 
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=369a5330ddec3dcbd1c09c0665f906e50c6dd69c&filter-
> resultStatus=usercancel&filter-resultStatus=runnable&filter-
> resultStatus=retry&filter-resultStatus=testfailed&filter-
> resultStatus=busted&filter-resultStatus=exception
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=140996790&repo=mozilla-
> inbound
> > REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/reftest/tests/layout/style/crashtests/1402472.html | assertion count 2 is more than expected 0 assertions

I should clearly have landed this before... :)
Comment on attachment 8896213 [details]
Bug 979782: Fixup FindFrameForContentSibling to don't duplicate work and trigger assertions for display: contents.

Mozreview is kinda drunk, this is the patch that needs review.
Flags: needinfo?(emilio)
Attachment #8896213 - Flags: review+ → review?(mats)
Comment on attachment 8924194 [details]
Bug 979782: Enable lazy frame construction for display: contents direct descendants.

Aand this one was reviewed.
Attachment #8924194 - Flags: review?(mats) → review+
Comment on attachment 8896213 [details]
Bug 979782: Fixup FindFrameForContentSibling to don't duplicate work and trigger assertions for display: contents.

Gah, I'm not sure I want to keep dealing with mozreview :)
Attachment #8896213 - Flags: review?(mats)
Comment on attachment 8896213 [details]
Bug 979782: Fixup FindFrameForContentSibling to don't duplicate work and trigger assertions for display: contents.

https://reviewboard.mozilla.org/r/167506/#review200528

r=mats with the nit fixed

::: layout/base/nsCSSFrameConstructor.cpp:6898
(Diff revision 4)
>      if (!sibling) {
>        // ... then ::after / ::before on the opposite end.



::: layout/base/nsCSSFrameConstructor.cpp:6898
(Diff revision 4)
>      if (!sibling) {
>        // ... then ::after / ::before on the opposite end.

Nit: the null-check here is now redundant, so I think we should remove it to avoid confusion.

::: layout/base/nsCSSFrameConstructor.cpp:6900
(Diff revision 4)
>        sibling = aPrevSibling ? nsLayoutUtils::GetAfterFrame(aContent)
>                               : nsLayoutUtils::GetBeforeFrame(aContent);

This is another bug isn't it?  I think I meant this to be the opposite
of the check we do before recursing into children, i.e. like this:

      sibling = aPrevSibling ? nsLayoutUtils::GetBeforeFrame(aContent)
                             : nsLayoutUtils::GetAfterFrame(aContent);

That is, if we have:

x { display:contents; }
x::before { content: "before"; }
<x></x>

then if aPrevSibling is true,  we want to first check for an ::after frame,
which we do before recursing into children, then if no child frame was
found, we want to check for a ::before frame here (and vice versa if
aPrevSibling is false).

Could you file a follow-up bug for this, unless you want to address it here?
Attachment #8896213 - Flags: review?(mats) → review+
Comment on attachment 8924194 [details]
Bug 979782: Enable lazy frame construction for display: contents direct descendants.

https://reviewboard.mozilla.org/r/195410/#review200546

(same patch as before I assume)
Attachment #8924194 - Flags: review?(mats) → review+
Comment on attachment 8896213 [details]
Bug 979782: Fixup FindFrameForContentSibling to don't duplicate work and trigger assertions for display: contents.

https://reviewboard.mozilla.org/r/167506/#review200550

::: layout/base/nsCSSFrameConstructor.cpp:6898
(Diff revision 4)
>      if (!sibling) {
>        // ... then ::after / ::before on the opposite end.

It is not (actually I had to update the patch because I noticed this, my first patch was as you suggested).

If we remove the null check may clobber the `before` or `after` frame that we got at the beginning.

::: layout/base/nsCSSFrameConstructor.cpp:6900
(Diff revision 4)
>        sibling = aPrevSibling ? nsLayoutUtils::GetAfterFrame(aContent)
>                               : nsLayoutUtils::GetBeforeFrame(aContent);

Indeed it is. I'll file and find a test-case.
Right, it's a regression from bug 1355351:
http://searchfox.org/mozilla-central/diff/6fe2b3e89d6476bf7150a4205ed094beeea17882/layout/base/nsCSSFrameConstructor.cpp#6732
so handling it as a separate bug is better for tracking purposes.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8012fdbac3eb
Fixup FindFrameForContentSibling to don't duplicate work and trigger assertions for display: contents. r=mats
https://hg.mozilla.org/integration/autoland/rev/86abda160c8a
Enable lazy frame construction for display: contents direct descendants. r=mats
> If we remove the null check may clobber the `before` or `after` frame that we got at the beginning.

Ah good point, I missed that.  Perhaps the code would be simpler to follow if we
move the "... then ::after / ::before on the opposite end" part into the first
"if (!sibling)" block after the code you just added?
(In reply to Mats Palmgren (:mats) from comment #21)
> > If we remove the null check may clobber the `before` or `after` frame that we got at the beginning.
> 
> Ah good point, I missed that.  Perhaps the code would be simpler to follow
> if we
> move the "... then ::after / ::before on the opposite end" part into the
> first
> "if (!sibling)" block after the code you just added?

I did that, looked at it, and didn't really love it, but we can do this in bug 1413619.
OK, it looks slightly simpler to me, fwiw.  We can also remove the
    if (!sibling) {
      return nullptr;
    }

and instead make the else-branch just be an if-statement:
  if (!sibling || sibling->GetContent() != aContent) {

I think the result makes it clearer that we use the first non-null
result from :::after/before, children, ::before/after and (now) only
fall-through in the pseudo case.
Actually, I guess "sibling->GetContent() != aContent" would be true in the pseudo case
so that won't work.
https://hg.mozilla.org/mozilla-central/rev/8012fdbac3eb
https://hg.mozilla.org/mozilla-central/rev/86abda160c8a
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee: nobody → emilio
You need to log in before you can comment on or make changes to this bug.