Closed Bug 1430014 Opened 5 years ago Closed 5 years ago

stylo: add build time option to disable the old style system

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(8 files)

Bug 1430014 - Part 1: Add --enable-stylo=only configure option and MOZ_OLD_STYLE define.
59 bytes, text/x-review-board-request
xidorn
: review+
glandium
: review+
Details
Bug 1430014 - Part 2: Adjust test assertion expectations.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1430014 - Part 3: Add DOMWindowUtils.isOldStyleSystemEnabled.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1430014 - Part 4: Skip failing/unnecessary tests when the old style system is not present.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1430014 - Part 5: #ifdef out unnecessary code when the old style system is not built.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1430014 - Part 6: Stop building old style system classes when MOZ_OLD_STYLE is not defined.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1430014 - Part 7: #ifdef out a bit more animation-related code.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Update to part 1 - Enable old_style by default
1.60 KB, patch
glandium
: review+
Details | Diff | Splinter Review 
We want this configurable so that we can (a) disable the old style system at build time for Android, before we're ready to do so for chrome documents on desktop, and (b) so we can easily switch it back on in case of emergency.
status-firefox57: --- → wontfix
status-firefox58: --- → wontfix
status-firefox-esr52: --- → unaffected
Priority: -- → P1
Summary: add build time option to disable the old style system → stylo: add build time option to disable the old style system
Without --disable-old-style-system: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47befe69f35b865e297df62adff3b460d3492495
With --disable-old-style-system: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0663642fab113255bc45280cb799caccf25adf2 (ignore the Stylo disabled tests there, which make no sense with --dissable-old-style-system)
Comment on attachment 8942842 [details]
Bug 1430014 - Part 1: Add --enable-stylo=only configure option and MOZ_OLD_STYLE define.

https://reviewboard.mozilla.org/r/213090/#review218782

::: toolkit/moz.configure:805
(Diff revision 1)
>  
>  set_config('SERVO_TARGET_DIR', servo_target_dir)
>  
> +# Disabling of the old style system
> +
> +option('--disable-old-style-system',

I think a better way to handle this is to, rather than add a separate option and then having to handle the conflict with --disable-stylo, would be to add another choice to the --enable-stylo option, which currently accepts --enable-stylo and --enable-stylo=build, like --enable-stylo=only.
Attachment #8942842 - Flags: review?(mh+mozilla)
Comment on attachment 8942842 [details]
Bug 1430014 - Part 1: Add --enable-stylo=only configure option and MOZ_OLD_STYLE define.

https://reviewboard.mozilla.org/r/213090/#review218810
Attachment #8942842 - Flags: review?(mh+mozilla) → review+
For Android, I get these apk sizes:

--enable-stylo:      33,969,855 bytes
--enable-stylo=only: 33,822,055 bytes

so that's only a 144 KiB saving.
Comment on attachment 8942842 [details]
Bug 1430014 - Part 1: Add --enable-stylo=only configure option and MOZ_OLD_STYLE define.

https://reviewboard.mozilla.org/r/213090/#review219092

::: modules/libpref/init/all.js:5798
(Diff revision 2)
> +#else
> +pref("layout.css.servo.chrome.enabled", true);
> +#endif

Why do we still need this pref (as well as the stylo pref above)? Can we just remove them when the old style system is not available?
Attachment #8942842 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8942843 [details]
Bug 1430014 - Part 2: Adjust test assertion expectations.

https://reviewboard.mozilla.org/r/213092/#review219094
Attachment #8942843 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8942844 [details]
Bug 1430014 - Part 3: Add DOMWindowUtils.isOldStyleSystemEnabled.

https://reviewboard.mozilla.org/r/213094/#review219100
Attachment #8942844 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #17)
> Why do we still need this pref (as well as the stylo pref above)? Can we
> just remove them when the old style system is not available?

I haven't tested, but it might cause trouble with e.g. reftest manifests that mention the pref, if the pref doen't exist.
(In reply to Cameron McCormack (:heycam) from comment #20)
> (In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #17)
> > Why do we still need this pref (as well as the stylo pref above)? Can we
> > just remove them when the old style system is not available?
> 
> I haven't tested, but it might cause trouble with e.g. reftest manifests
> that mention the pref, if the pref doen't exist.

Hmmm, okay that makes sense. We can remove them in a separate bug. It could be a bit confusing, but it's probably fine as far as this build time option isn't the default.
Depends on: 1426223
Comment on attachment 8942845 [details]
Bug 1430014 - Part 4: Skip failing/unnecessary tests when the old style system is not present.

https://reviewboard.mozilla.org/r/213096/#review219106

::: layout/style/test/test_stylo_blocklist-01.html:10
(Diff revision 2)
>    <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>  </head>
>  <body>
>  <script>
> +let utils = SpecialPowers.getDOMWindowUtils(window);

We can probably wait for the removal of blocklist in bug 1426223 to happen first so that we don't need this change to the test.

Actually, it seems that if the test can be removed, `isOldStyleSystemAvailable` added in part 3 is also not needed anymore.
Attachment #8942845 - Flags: review?(xidorn+moz) → review+
We can also remove some code in gfx/layers/AnimationHelper.cpp, you can check USE_STYLO_ON_COMPOSITOR there.
(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #22)
> We can probably wait for the removal of blocklist in bug 1426223 to happen
> first so that we don't need this change to the test.

I feel like we should wait until the old style system code is removed completely before removing the blocklist (or, really, remove them at the same time).  It is unlikely we'll need to use it, but I don't think we should remove that option.
I think we can also disable StyleAnimationValue::Accumulate, Add and Interpolate and functions used inside them.  Those are big to some extent.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> I think we can also disable StyleAnimationValue::Accumulate, Add and
> Interpolate and functions used inside them.  Those are big to some extent.

Ideally StyleAnimationValue can be dropped if I am missing something.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #26)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> > I think we can also disable StyleAnimationValue::Accumulate, Add and
> > Interpolate and functions used inside them.  Those are big to some extent.
> 
> Ideally StyleAnimationValue can be dropped if I am missing something.

s/if/unless/
Yes, I managed to drop StyleAnimationValue and a few other bits.  I'll upload a separate patch for that.
(In reply to Cameron McCormack (:heycam) from comment #24)
> (In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #22)
> > We can probably wait for the removal of blocklist in bug 1426223 to happen
> > first so that we don't need this change to the test.
> 
> I feel like we should wait until the old style system code is removed
> completely before removing the blocklist (or, really, remove them at the
> same time).  It is unlikely we'll need to use it, but I don't think we
> should remove that option.

I don't think it's worth keeping... but I guess I can live with having it for a bit longer.
(In reply to Cameron McCormack (:heycam) from comment #16)
> For Android, I get these apk sizes:
> 
> --enable-stylo:      33,969,855 bytes
> --enable-stylo=only: 33,822,055 bytes

With the animation stuff removed it's 33,812,226 bytes, so just another 10 KiB.
Comment on attachment 8942846 [details]
Bug 1430014 - Part 5: #ifdef out unnecessary code when the old style system is not built.

https://reviewboard.mozilla.org/r/213098/#review219132

r=me with comments addressed.

::: dom/animation/KeyframeEffectReadOnly.cpp:236
(Diff revision 2)
> +#ifdef MOZ_OLD_STYLE
>    static_assert(IsSame<StyleType, GeckoStyleContext>::value ||
>                  IsSame<StyleType, const ServoStyleContext>::value,
>                  "StyleType should be GeckoStyleContext* or "
>                  "const ServoStyleContext*");
> +#endif

Should we do `static_assert` that `StyleType` is always `const ServoStyleContext` now when old style system is disabled? That way we can also inform future developer who is removing the code that this function can be de-templated (rather than just blindly removing all code wrapped inside the `#ifdef`).

::: dom/animation/KeyframeEffectReadOnly.cpp:968
(Diff revision 2)
> +#ifdef MOZ_OLD_STYLE
>    static_assert(IsSame<StyleType, GeckoStyleContext>::value ||
>                  IsSame<StyleType, const ServoStyleContext>::value,
>                  "StyleType should be GeckoStyleContext* or "
>                  "const ServoStyleContext*");
> +#endif

Same here.

::: dom/animation/KeyframeUtils.cpp:945
(Diff revision 2)
>    }
>  
>    result.emplace(aProperty, Move(value));
>    return result;
> +#else
> +  MOZ_CRASH("old style system disabled");

I'm wondering wouldn't compilers require you to return something here? C++ doesn't have void type, so I don't know how this would work... but if it works, it works, I guess.

::: dom/animation/KeyframeUtils.cpp:1099
(Diff revision 2)
>  /**
>   * The variation of the above function. This is for Servo backend.
>   */

Consider copying the comment from the function above to here?

::: dom/base/nsDocument.cpp:1973
(Diff revision 2)
>      }
>    }
>  
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mChannel)
> +#ifdef MOZ_OLD_STYLE
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mStyleAttrStyleSheet)

But it seems that you haven't actually removed this field from nsIDocument?

::: dom/base/nsINode.cpp:2851
(Diff revision 2)
> -      if (!aMatchInfo ||
> -          nsCSSRuleProcessor::SelectorListMatches(element,
> +      if (!aMatchInfo
> +#ifdef MOZ_OLD_STYLE
> +          || nsCSSRuleProcessor::SelectorListMatches(element,
> -                                                  aMatchInfo->mMatchContext,
> +                                                     aMatchInfo->mMatchContext,
> -                                                  aMatchInfo->mSelectorList)) {
> +                                                     aMatchInfo->mSelectorList)
> +#endif

I guess you can just use `true` here when old style system is disabled, and optionally add an assertion that `aMatchInfo` is always null.

::: dom/base/nsStyleLinkElement.cpp:430
(Diff revision 2)
> +#ifdef MOZ_OLD_STYLE
>        oldScopeElement = mStyleSheet->AsGecko()->GetScopeElement();
> +#else
> +      MOZ_CRASH("old style system disabled");
> +#endif

I guess you can wrap the whole `if (mStyleSheet)` block into the `#ifdef`, probably also `oldScopeElement`, then you can probably wrap `UpdateIsElementInStyleScopeFlagOnSubtree` as well.

::: dom/canvas/CanvasRenderingContext2D.cpp
(Diff revision 2)
> -  // FIXME(bug 1420026).
> -  if (false) {
> -    nsCOMPtr<nsIPresShell> presShell = GetPresShell();
> +  nsCOMPtr<nsIPresShell> presShell = GetPresShell();
> -    ServoStyleSet* set = presShell ? presShell->StyleSet()->AsServo() : nullptr;
> +  ServoStyleSet* set = presShell ? presShell->StyleSet()->GetAsServo() : nullptr;
>  
> +  bool useServoParser =
> +#ifdef MOZ_OLD_STYLE
> +    set;
> +#else
> +    true;
> +#endif

It looks like you are enabling the new stylo code path even when the old style system is not removed.

You probably should fix bug 1420026 first, rather than silently enabling the path here, I guess...

::: editor/libeditor/HTMLEditor.cpp:2914
(Diff revision 2)
>    if (sheet->IsServo()) {
>      // XXXheycam ServoStyleSheets don't support being enabled/disabled yet.
>      NS_ERROR("stylo: ServoStyleSheets can't be disabled yet");
>      return true;
>    }
> +#ifdef MOZ_OLD_STYLE
>    sheet->AsGecko()->SetDisabled(false);
> +#else
> +  MOZ_CRASH("old style system disabled");
> +#endif

I'm surprised that we didn't hit any problem here... I believe we should be able to enable / disable servo style sheets now. Mind filing a bug for fixing this?

::: layout/base/RestyleManager.cpp:637
(Diff revision 2)
>    if (IsServo()) {
>      // XXXheycam For now, we know that we don't use the same inheritance
>      // hierarchy for certain cases, so just skip these assertions until
>      // we work out what we want to assert (bug 1322570).
>      return;
>    }

It seems that bug 1322570 has been fixed. Can we wrap this whole method in `#ifdef`?

::: layout/style/GenericSpecifiedValuesInlines.h:93
(Diff revision 2)
>    static_assert(
>      !mozilla::IsSame<decltype(&MOZ_STYLO_THIS_TYPE::SetIntValue),
>                       decltype(&MOZ_STYLO_SERVO_TYPE::SetKeywordValue)>::value,
>      "Servo subclass should define its own SetKeywordValue");

We probably should still keep servo's assertion for now?

::: layout/style/ServoUtils.h:125
(Diff revision 2)
>    static_assert(!mozilla::IsSame<decltype(&MOZ_STYLO_THIS_TYPE::method_),   \
>                                   decltype(&MOZ_STYLO_SERVO_TYPE::method_)>  \
>                  ::value, "Servo subclass should define its own " #method_); \

Again, I think we should probably keep the static assertion since we still need to ensure we don't recursively call oneself.
Attachment #8942846 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8942847 [details]
Bug 1430014 - Part 6: Stop building old style system classes when MOZ_OLD_STYLE is not defined.

https://reviewboard.mozilla.org/r/213100/#review219528

r=me with nits addressed.

::: dom/animation/moz.build:43
(Diff revision 2)
> +if CONFIG['MOZ_OLD_STYLE']:
> +    EXPORTS.mozilla += [
> +        'AnimValuesStyleRule.h',
> +    ]

I feel that you can merge the two if blocks together. Probably not a big issue, though.

::: layout/base/RestyleManager.cpp:19
(Diff revision 2)
>  #include "nsIPresShellInlines.h"
> +#include "nsStyleUtil.h"
> +#include "StickyScrollContainer.h"
> +#include "mozilla/EffectSet.h"
> +#include "mozilla/ViewportFrame.h"
> +#include "mozilla/SVGTextFrame.h"

You can use `SVGTextFrame.h` directly. `layout/base/moz.build` includes `../svg` in its `LOCAL_INCLUDES`, so you don't need to expose this file into `include/mozilla`.

::: layout/svg/moz.build:32
(Diff revision 2)
>      'SVGObserverUtils.h',
>  ]
>  
>  EXPORTS.mozilla += [
>      'SVGContextPaint.h',
> +    'SVGTextFrame.h',

This change is not needed. See above.
Attachment #8942847 - Flags: review?(xidorn+moz) → review+
I suspect that if you simply remove nsIDocument::mStyleBackendType and related setting methods, then make nsIDocument::GetStyleBackendType() consistently return mozilla::StyleBackendType::Servo (and consequently nsIDocument::IsStyledByServo() would always return true), you would see some more codesize reduction, because the compiler would be able to remove more Gecko-specific code guarded by IsStyledByServo() check.
Attachment #8943112 - Flags: review?(xidorn+moz) → review?(hikezoe)
I have no idea what does it mean for removing USE_STYLO_ON_COMPOSITOR and enable stylo on compositor for Android. I suspect this should probably be done in a separate bug. But anyway, I'd leave it to animation people.
Comment on attachment 8943112 [details]
Bug 1430014 - Part 7: #ifdef out a bit more animation-related code.

https://reviewboard.mozilla.org/r/213408/#review219540

::: gfx/config/gfxVars.h
(Diff revision 1)
> -#if defined(MOZ_STYLO) && !defined(ANDROID)
> -  #define USE_STYLO_ON_COMPOSITOR true
> -#else
> -  #define USE_STYLO_ON_COMPOSITOR false
> -#endif

As Xidorn said, we should remove this flag in a separate bug. But... yeah. annoying.  Curretnly we don't use stylo on the compositor for Android at all even if the pref is enabled, so anyway we should use stylo there as well, but if we decided to not ship stylo for Android in Firefox 59, we can't get back.  Oh, if we decided to not ship stylo for Android in Firefox 59, we build the binary with --disable-stylo flag?  If so, I am fine with the flag removal in this bug.
Attachment #8943112 - Flags: review?(hikezoe) → review+
Comment on attachment 8942846 [details]
Bug 1430014 - Part 5: #ifdef out unnecessary code when the old style system is not built.

https://reviewboard.mozilla.org/r/213098/#review219132

> But it seems that you haven't actually removed this field from nsIDocument?

Right, we keep this object since it's where we store cached style="" attributes, but the patch makes it no longer inherit from nsIStyleRuleProcessor.  Traversing/unlinking nsISupports-inherited objects like this one is fine, even if it has no special CC behavior itself, but the traversing won't work after dropping nsIStyleRuleProcessor unless we add a CC implementation.

We could probably just drop this NS_IMPL_CYCLE_COLLECTION_TRAVERSE line even for the old style system.

> I'm surprised that we didn't hit any problem here... I believe we should be able to enable / disable servo style sheets now. Mind filing a bug for fixing this?

Filed bug 1431279.
(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #34)
> I suspect that if you simply remove nsIDocument::mStyleBackendType and
> related setting methods, then make nsIDocument::GetStyleBackendType()
> consistently return mozilla::StyleBackendType::Servo (and consequently
> nsIDocument::IsStyledByServo() would always return true), you would see some
> more codesize reduction, because the compiler would be able to remove more
> Gecko-specific code guarded by IsStyledByServo() check.

Doing that reduced the apk to 33,808,665 bytes, so that saved 3.5 KiB.
I guess nsCSSKeyword and the related lookup tables contribute something to the code size which we probably should investigate how to remove at some point...
(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #39)
> I guess nsCSSKeyword and the related lookup tables contribute something to
> the code size which we probably should investigate how to remove at some
> point...

This reminded me that we can also remove nsStyleAnimType and their values in nsCSSPropList.h.
So I did some try pushes for this, and it seems the patch breaks mingw32. From the error message, I suspect that mingw32 build doesn't get MOZ_OLD_STYLE define at all.
It seems this also breaks browser_webconsole_message_categories.js#9, which is a test checking console message from css parser.
Depends on: 1420026
Attachment #8946975 - Flags: review?(mh+mozilla) → review+
Looks like everything works now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b1e584d69a0fb08d625faf70793d904a62f6d2b (patch applied)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c93570a24c9958d62a8b321a607b1b8cf0993416 (only stylo)

There are lots of failures from the second try push, mostly in the stylo-disabled platform (which is kind of expected) and stylo-chrome.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6de1de817673
Part 1: Add --enable-stylo=only configure option and MOZ_OLD_STYLE define. r=glandium,xidorn
https://hg.mozilla.org/integration/autoland/rev/00eb76cb9633
Part 2: Adjust test assertion expectations. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/e15f4b6912be
Part 3: Skip failing/unnecessary tests when the old style system is not present. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/b41519a01488
Part 4: #ifdef out unnecessary code when the old style system is not built. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/91687b8690ea
Part 5: Stop building old style system classes when MOZ_OLD_STYLE is not defined. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/6993eb29af7b
Part 6: #ifdef out a bit more animation-related code. r=hiro
You need to log in before you can comment on or make changes to this bug.