898333 - make ElementRestyler::Restyle traverse tree through continuations

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

As I suggested in bug 828312 comment 69, we need to change ElementRestyler::Restyle (to be refactored in bug 898329) so that the tree traversal works differently.

Currently ElementRestyler::Restyle does a normal depth-first traversal of the frame tree (with an exception for outer/inner tables, a.k.a. tables and table wrappers).  However, in bug 828312 I want to change the semantics of change hints so that we only do difference computation on the first continuation.  This, in turn, requires that we process the descendants of all continuations (and same-style parts of block-in-inline splits) inside the code where we process the first continuation.  In other words, this requires that we do a depth-first search as though continuations and same-style block-in-inline splits were merged back together.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://tbpl.mozilla.org/?tree=Try&rev=f9fcec3737cc

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://tbpl.mozilla.org/?tree=Try&rev=ea02a143e546

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://tbpl.mozilla.org/?tree=Try&rev=8c16631f5359

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: , patch 1: Restyle the :after pseudo-element after the content children.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: , patch 2: Change RestyleManager::Restyle's tree traversal to reach next-continuations (and same-display block-in-inline siblings) from their prev-continuation rather than from their parent.

This patch fundamentally makes three changes:

 (1) Change the way RestyleSelf is used so that it is called in a loop,
     over all of the same-style continuations and block-in-inline
     siblings.
     (I had a note here reminding myself:
       TODO: Don't set hints for descendants!
     but I know longer remember what it meant.)

 (2) Change the traversal of children to traverse all of the children of
     the items traversed in (1).

 (3) When traversing children, skip children that are continuations,
     since we already reached them in (1) and (2).

A later patch will change the RestyleSelf loop to copy for the later
continuations rather than repeating the work.  This will mean reverting
many of the RestyleSelf changes contained here.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 805488 [details] [diff] [review]
, patch 1:  Restyle the :after pseudo-element after the content children.

Hmm.  So the order was what it was so that if we have to add/remove an ::after we don't have to waste time restyling the kids.

I guess that's a rare case anyway.  r=me

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seemed necessary (at least for simplicity) in patch 2 (see the changes in patch 2 to the same code), and it was easy to split into its own patch.  (I thought about splitting up patch 2 more, but didn't see a nice way to create fully working intermediate states.)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 805489 [details] [diff] [review]
, patch 2:  Change RestyleManager::Restyle's tree traversal to reach next-continuations (and same-display block-in-inline siblings) from their prev-continuation rather than from their parent.

>     but I know longer remember what it meant.)

"no longer"?

>+                               nsIFrame* aContinuation, // TEMPORARY

Where will it get removed?  Same for the other temporary bits.

>+ * with the same style) all of its next continuations with the same

Comma before "all", please.

r=me

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> >+                               nsIFrame* aContinuation, // TEMPORARY
> 
> Where will it get removed?  Same for the other temporary bits.

Some will get removed in bug 828312 patch 11 (attachment 806310 [details] [diff] [review]), and the rest will, I hope, get removed in a followup bug to restructure things to set the style context directly on all the continuations in a single RestyleSelf call instead of having to invoke RestyleSelf for each continuation.  (Though that followup might require further followups to fix the same pattern in ReParentStyleContext.)

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed bug 918064 on that followup, and annotated the TEMPORARY comments with when they're expected to be removed (though only one remains after bug 828312, and it points to bug 918064).

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b680053ea755
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed9c22ef51e0

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ed9c22ef51e0

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b680053ea755