979782 - Consider implementing lazy frame construction for display:contents descendants

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 979782: Fixup FindFrameForContentSibling to don't duplicate work and trigger assertions for display: contents.

There's no reason I can think of this wouldn't work, and try is totally green
without it.

Review commit: https://reviewboard.mozilla.org/r/167506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167506/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8896213 [details]
Bug 979782: Fixup FindFrameForContentSibling to don't duplicate work and trigger assertions for display: contents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167506/diff/1-2/

Comment 4

[Mats Palmgren (inactive)]

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

Comment 5

[Mozilla Autoland]

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)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/369a5330ddec
Enable lazy frame construction for display: contents direct descendants. r=mats

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 8

[Emilio Cobos Álvarez (: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 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 979782: Enable lazy frame construction for display: contents direct descendants.

There's no reason I can think of this wouldn't work, and try is totally green
without it.

Review commit: https://reviewboard.mozilla.org/r/195410/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195410/

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8896213 [details]
Bug 979782: Fixup FindFrameForContentSibling to don't duplicate work and trigger assertions for display: contents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167506/diff/2-3/

Comment 11

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8924194 [details]
Bug 979782: Enable lazy frame construction for display: contents direct descendants.

Aand this one was reviewed.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8896213 [details]
Bug 979782: Fixup FindFrameForContentSibling to don't duplicate work and trigger assertions for display: contents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167506/diff/3-4/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8924194 [details]
Bug 979782: Enable lazy frame construction for display: contents direct descendants.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195410/diff/1-2/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

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 :)

Comment 16

[Mats Palmgren (inactive)]

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?

Comment 17

[Mats Palmgren (inactive)]

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)

Comment 18

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 19

[Mats Palmgren (inactive)]

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.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Mats Palmgren (inactive)]

> 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?

Comment 22

[Emilio Cobos Álvarez (:emilio)]

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

Comment 23

[Mats Palmgren (inactive)]

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.

Comment 24

[Mats Palmgren (inactive)]

Actually, I guess "sibling->GetContent() != aContent" would be true in the pseudo case
so that won't work.

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8012fdbac3eb
https://hg.mozilla.org/mozilla-central/rev/86abda160c8a