1345362 - stylo: Make -moz-pagebreak anonymous boxes non-inheriting, so their style doesn't depend on their parent

Status: RESOLVED FIXED

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1345362 part 2. Introduce nsCSSAnonBoxes::NonInheritingTypeForPseudoTag for looking up the type of non-inheriting anonymous box for a given pseudo tag.

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1345362 part 3. Introduce CSSPseudoElementType::NonInheritingAnonBox to represent non-inheriting anon boxes.

The idea is to be able to call the right function during style resolution time
without doing a linear walk via nsCSSAnonBoxes::IsNonInheritingAnonBox.

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1345362 part 5. Assert that we don't have a non-inheriting anon box in nsFrame::UpdateStyleOfChildAnonBox.

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1345362 part 6. Make ::-moz-pagebreak a non-inheriting anon box.

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

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8844800 [details]
Bug 1345362 part 2.  Introduce nsCSSAnonBoxes::NonInheritingTypeForPseudoTag for looking up the type of non-inheriting anonymous box for a given pseudo tag.

https://reviewboard.mozilla.org/r/118112/#review119978

::: layout/style/nsCSSAnonBoxes.h:59
(Diff revision 1)
>  #define CSS_ANON_BOX(_name, _value) /* nothing */
>  #define CSS_NON_INHERITING_ANON_BOX(_name, _value) _name == aPseudo ||
>  #include "nsCSSAnonBoxList.h"
>  #undef CSS_NON_INHERITING_ANON_BOX
>  #undef CSS_ANON_BOX
>        0;

FWIW this should probably be "false".

Comment 6

[Cameron McCormack (:heycam)]

Will look at the rest of these properly tomorrow, but a few things first:

1. Why the different behaviour for ResolveStyleForPlaceholder and ResolveStyleForNonInheritingAnonymousBox?
2. mNonInheritingStyleContexts is a perfect candidate for mozilla::EnumeratedArray.  That would eliminate a bunch of the casts to NonInheritedBase.
3. I kind of want CSSPseudoElementType::AnonBox to be called InheritingNonBox, for symmetry.  AnonBox by itself sounds like it encompass both, especially given the naming of nsCSSAnonBoxes::IsAnonBox.
4. Sorry about the StyleSetHandle setup, which results in a bit of duplication; I really should get around to turning that into a proper base class / derived class relationship.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> FWIW this should probably be "false".

Agreed, will fix.

> 1. Why the different behaviour for ResolveStyleForPlaceholder and ResolveStyleForNonInheritingAnonymousBox?

The former never has any CSS rules applying to it.  The latter can.  So the former can skip all the rulewalker stuff and just pass the root rulenode when creating the stylecontext, while the latter has to figure out the right rulenode.

> 2. mNonInheritingStyleContexts is a perfect candidate for mozilla::EnumeratedArray.

Ah, I didn't know we had this thing!  I will change it accordingly, in a changeset I will slide under these.  It will definitely make things more pleasant!

> 3. I kind of want CSSPseudoElementType::AnonBox to be called InheritingNonBox, for symmetry.

Will do.

> 4. Sorry about the StyleSetHandle setup, which results in a bit of duplication;

Yeah, I tried putting this array in the Ptr when I was working on bug 1343078, but then realized that assigning a StyleSetHandle copies the Ptr, so putting something like this there doesn't work.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1345362 part 1. Use mozilla::EnumeratedArray for mNonInheritingStyleContexts.

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844800 [details]
Bug 1345362 part 2.  Introduce nsCSSAnonBoxes::NonInheritingTypeForPseudoTag for looking up the type of non-inheriting anonymous box for a given pseudo tag.

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844801 [details]
Bug 1345362 part 3.  Introduce CSSPseudoElementType::NonInheritingAnonBox to represent non-inheriting anon boxes.

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844802 [details]
Bug 1345362 part 5.  Assert that we don't have a non-inheriting anon box in nsFrame::UpdateStyleOfChildAnonBox.

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844803 [details]
Bug 1345362 part 6.  Make ::-moz-pagebreak a non-inheriting anon box.

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

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8845140 [details]
Bug 1345362 part 1.  Use mozilla::EnumeratedArray for mNonInheritingStyleContexts.

https://reviewboard.mozilla.org/r/118358/#review120274

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8844801 [details]
Bug 1345362 part 3.  Introduce CSSPseudoElementType::NonInheritingAnonBox to represent non-inheriting anon boxes.

https://reviewboard.mozilla.org/r/118114/#review120280

::: layout/base/GeckoRestyleManager.cpp:2876
(Diff revision 2)
>            styleSet->ResolveStyleWithReplacement(element, pseudoElement,
>                                                  newContext, oldExtraContext,
>                                                  nsRestyleHint(0));
>        }
> -    } else if (extraPseudoType == CSSPseudoElementType::AnonBox) {
> +    } else if (extraPseudoType == CSSPseudoElementType::InheritingAnonBox) {
>        newExtraContext = styleSet->ResolveAnonymousBoxStyle(extraPseudoTag,

Would you mind renaming ResolveAnonymousBoxStyle to ResolveInheritingAnonymousBoxStyle, too?

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8844802 [details]
Bug 1345362 part 5.  Assert that we don't have a non-inheriting anon box in nsFrame::UpdateStyleOfChildAnonBox.

https://reviewboard.mozilla.org/r/118116/#review120286

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8844803 [details]
Bug 1345362 part 6.  Make ::-moz-pagebreak a non-inheriting anon box.

https://reviewboard.mozilla.org/r/118118/#review120290

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844801 [details]
Bug 1345362 part 3.  Introduce CSSPseudoElementType::NonInheritingAnonBox to represent non-inheriting anon boxes.

https://reviewboard.mozilla.org/r/118114/#review120280

> Would you mind renaming ResolveAnonymousBoxStyle to ResolveInheritingAnonymousBoxStyle, too?

Done, added as a part 4.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1345362 part 4. Rename ResolveAnonymousBoxStyle to ResolveInheritingAnonymousBoxStyle.

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8845140 [details]
Bug 1345362 part 1.  Use mozilla::EnumeratedArray for mNonInheritingStyleContexts.

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844800 [details]
Bug 1345362 part 2.  Introduce nsCSSAnonBoxes::NonInheritingTypeForPseudoTag for looking up the type of non-inheriting anonymous box for a given pseudo tag.

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844801 [details]
Bug 1345362 part 3.  Introduce CSSPseudoElementType::NonInheritingAnonBox to represent non-inheriting anon boxes.

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844802 [details]
Bug 1345362 part 5.  Assert that we don't have a non-inheriting anon box in nsFrame::UpdateStyleOfChildAnonBox.

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

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844803 [details]
Bug 1345362 part 6.  Make ::-moz-pagebreak a non-inheriting anon box.

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

Comment 24

[Cameron McCormack (:heycam)]

Comment on attachment 8845237 [details]
Bug 1345362 part 4.  Rename ResolveAnonymousBoxStyle to ResolveInheritingAnonymousBoxStyle.

https://reviewboard.mozilla.org/r/118422/#review120318

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/382046a719ff
part 1.  Use mozilla::EnumeratedArray for mNonInheritingStyleContexts.  r=heycam
https://hg.mozilla.org/integration/autoland/rev/df570f682fea
part 2.  Introduce nsCSSAnonBoxes::NonInheritingTypeForPseudoTag for looking up the type of non-inheriting anonymous box for a given pseudo tag.  r=heycam
https://hg.mozilla.org/integration/autoland/rev/8fc5d55b1e9d
part 3.  Introduce CSSPseudoElementType::NonInheritingAnonBox to represent non-inheriting anon boxes.  r=heycam
https://hg.mozilla.org/integration/autoland/rev/369ab8db0369
part 4.  Rename ResolveAnonymousBoxStyle to ResolveInheritingAnonymousBoxStyle.  r=heycam
https://hg.mozilla.org/integration/autoland/rev/97269aca06bf
part 5.  Assert that we don't have a non-inheriting anon box in nsFrame::UpdateStyleOfChildAnonBox.  r=heycam
https://hg.mozilla.org/integration/autoland/rev/7ac30073bcac
part 6.  Make ::-moz-pagebreak a non-inheriting anon box.  r=heycam

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/382046a719ff
https://hg.mozilla.org/mozilla-central/rev/df570f682fea
https://hg.mozilla.org/mozilla-central/rev/8fc5d55b1e9d
https://hg.mozilla.org/mozilla-central/rev/369ab8db0369
https://hg.mozilla.org/mozilla-central/rev/97269aca06bf
https://hg.mozilla.org/mozilla-central/rev/7ac30073bcac