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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | wontfix |
firefox58 | --- | wontfix |
firefox59 | --- | wontfix |
firefox60 | --- | fixed |
People
(Reporter: heycam, Assigned: heycam)
References
Details
Attachments
(8 files)
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
glandium
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
hiro
:
review+
|
Details |
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.
Updated•5 years ago
|
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•5 years ago
|
||
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 8•5 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•5 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 16•5 years ago
|
||
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 17•5 years ago
|
||
mozreview-review |
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 18•5 years ago
|
||
mozreview-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 19•5 years ago
|
||
mozreview-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+
Assignee | ||
Comment 20•5 years ago
|
||
(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.
Comment 21•5 years ago
|
||
(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.
Comment 22•5 years ago
|
||
mozreview-review |
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+
Comment 23•5 years ago
|
||
We can also remove some code in gfx/layers/AnimationHelper.cpp, you can check USE_STYLO_ON_COMPOSITOR there.
Assignee | ||
Comment 24•5 years ago
|
||
(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.
Comment 25•5 years ago
|
||
I think we can also disable StyleAnimationValue::Accumulate, Add and Interpolate and functions used inside them. Those are big to some extent.
Comment 26•5 years ago
|
||
(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.
Comment 27•5 years ago
|
||
(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/
Assignee | ||
Comment 28•5 years ago
|
||
Yes, I managed to drop StyleAnimationValue and a few other bits. I'll upload a separate patch for that.
Comment 29•5 years ago
|
||
(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.
Assignee | ||
Comment 30•5 years ago
|
||
(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 hidden (mozreview-request) |
Comment 32•5 years ago
|
||
mozreview-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 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 33•5 years ago
|
||
mozreview-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+
Comment 34•5 years ago
|
||
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.
Updated•5 years ago
|
Attachment #8943112 -
Flags: review?(xidorn+moz) → review?(hikezoe)
Comment 35•5 years ago
|
||
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 36•5 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 37•5 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 38•5 years ago
|
||
(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.
Comment 39•5 years ago
|
||
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...
Comment 40•5 years ago
|
||
(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.
Comment 41•5 years ago
|
||
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.
Comment 42•5 years ago
|
||
OK it seems I eventually got something to work. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b89895829303388ad49e3da37a2ac7afe33eeb5 https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b0eab68976db9ff340f5160bd297208a12127d0 There are some changes to part 1 which may need additional review.
Comment 43•5 years ago
|
||
It seems this also breaks browser_webconsole_message_categories.js#9, which is a test checking console message from css parser.
Comment 44•5 years ago
|
||
Attachment #8946975 -
Flags: review?(mh+mozilla)
Updated•5 years ago
|
Attachment #8946975 -
Flags: review?(mh+mozilla) → review+
Comment 45•5 years ago
|
||
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.
Comment 46•5 years ago
|
||
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
Comment 47•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6de1de817673 https://hg.mozilla.org/mozilla-central/rev/00eb76cb9633 https://hg.mozilla.org/mozilla-central/rev/e15f4b6912be https://hg.mozilla.org/mozilla-central/rev/b41519a01488 https://hg.mozilla.org/mozilla-central/rev/91687b8690ea https://hg.mozilla.org/mozilla-central/rev/6993eb29af7b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•