Boris Zbarsky [:bzbarsky] Assignee
	Description • 8 years ago

      No description provided.

	Cameron McCormack (:heycam)
	Comment 5 • 8 years ago
mozreview-review

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

	Cameron McCormack (:heycam)
	Comment 6 • 8 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 8 years ago

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

	Cameron McCormack (:heycam)
	Comment 13 • 8 years ago
mozreview-review

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

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

	Cameron McCormack (:heycam)
	Comment 14 • 8 years ago
mozreview-review

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?

	Cameron McCormack (:heycam)
	Comment 15 • 8 years ago
mozreview-review

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

	Cameron McCormack (:heycam)
	Comment 16 • 8 years ago
mozreview-review

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

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 8 years ago
mozreview-review-reply

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.

	Cameron McCormack (:heycam)
	Comment 24 • 8 years ago
mozreview-review

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

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

	Pulsebot
	Comment 25 • 8 years ago

Pushed by bzbarsky@mozilla.com:
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

	Carsten Book [:Tomcat]
	Comment 26 • 8 years ago
bugherder

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