1411517 - stylo: "layout.css.stylo-blocklist.enabled" pref's default value should be false

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

This is a minor Stylo performance bug we should fix in Beta 57.

We ship an empty Stylo domain blocklist in Beta 57 and Nightly 58. We included a test domain that we removed last week in bug 1409958.

Since the default blocklist ("layout.css.stylo-blocklist.blocked_domains" pref) is empty, we should set the "layout.css.stylo-blocklist.enabled" pref = false so we don't waste time trying to read the empty blocklist pref. If we need to ship our Stylo system add-on to update the blocklist pref, it can also set the enabled pref = true.

Comment 1

[Chris Peterson [:cpeterson]]

Xidorn, do you have a moment to fix and uplift this one-line pref change?

We should disable the Stylo blocklist pref to avoid the slow path of trying to parse the (empty) "blocked_domains" pref string. If we need to update the blocklist after release, the Stylo system add-on can set the enabled pref = true at the same time.

Comment 2

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This would be a good one to fix, tracked for 57.

Comment 3

[Jeremy Chen ([e-mail])]

Since this is not handled yet so far, and I kind of have some time slot for creating this simple patch, so, I'm going to upload a patch here.

Hi xidorn, could you help push this simple patch for me once you finish the review?

Hi Chris, could you request the uplift for me once this patch get merged to m-c?

Comment 4

[Jeremy Chen ([e-mail])]

Attachment: Bug 1411517 - stylo: "layout.css.stylo-blocklist.enabled" pref's default value should be false.

At present, the "layout.css.stylo-blocklist.blocked_domains" pref is empty. It
is probably meaningless to set "layout.css.stylo-blocklist.enabled" pref to true
by default. We can set "layout.css.stylo-blocklist.enabled" pref to true by the
styloblocklist system add-on once the "layout.css.stylo-blocklist.blocked_domains"
pref is non-empty.

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8922485 [details]
Bug 1411517 - stylo: "layout.css.stylo-blocklist.enabled" pref's default value should be false.

https://reviewboard.mozilla.org/r/193562/#review198780

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96f54c7e0a1a
stylo: "layout.css.stylo-blocklist.enabled" pref's default value should be false. r=xidorn

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8922485 [details]
Bug 1411517 - stylo: "layout.css.stylo-blocklist.enabled" pref's default value should be false.

Approval Request Comment
[Feature/Bug causing the regression]: stylo blocklist
[User impact if declined]: maybe slightly slower than it should be, but no big impact
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: just flip a pref to skip a bunch of new code path which we are not using at the moment
[String changes made/needed]: n/a

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Jeremy Chen [:jeremychen] UTC+8 (away 24 Oct – 12 Nov) from comment #3)
> Since this is not handled yet so far, and I kind of have some time slot for
> creating this simple patch, so, I'm going to upload a patch here.

Thanks for helping while you are on your trip! :)

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/96f54c7e0a1a

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8922485 [details]
Bug 1411517 - stylo: "layout.css.stylo-blocklist.enabled" pref's default value should be false.

Beta57+

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4465ffadaee2