1366914 - Remove pref layout.css.background-clip-text.enabled

Status: RESOLVED FIXED

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

Description

[Manish Goregaokar [:manishearth]]

layout.css.background-clip-text.enabled controls whether or not we parse `background-clip: text`

Comment 1

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1366914 - Remove pref layout.css.background-clip-text.enabled;

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

Comment 2

[Manish Goregaokar [:manishearth]]

Gecko supports this via a pref observer that modifies the keyword table, in http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/layout/base/nsLayoutUtils.cpp#449. Is there a performance-related reason for doing that?

And, if so, should we do something similar by making that method flip a static?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

My first question would be, do we really need to support this pref anymore? It was flipped on in bug 1264905, which means we start shipping background-clip: text since Firefox 48. I suppose we may not want to unship it, so probably we can just remove this pref and support it unconditionally.

cjku, dholbert, what do you think?


(In reply to Manish Goregaokar [:manishearth] from comment #2)
> Gecko supports this via a pref observer that modifies the keyword table, in
> http://searchfox.org/mozilla-central/rev/
> 6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/layout/base/nsLayoutUtils.cpp#449.
> Is there a performance-related reason for doing that?
> 
> And, if so, should we do something similar by making that method flip a
> static?

For this, in general, pref query would cost a hash table lookup, which is considered an unnecessary extra cost in parsing, so using pref observer with a static may be preferable. It may not matter too much in this specific case, though, I guess.

Comment 4

[u459114]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> My first question would be, do we really need to support this pref anymore?
> It was flipped on in bug 1264905, which means we start shipping
> background-clip: text since Firefox 48. I suppose we may not want to unship
> it, so probably we can just remove this pref and support it unconditionally.
> 
> cjku, dholbert, what do you think?
I don't see a sigh that we are going to unship it in near future and this feature had been recorded in background L4 spec.
Vote for removing this pref.

Comment 5

[Manish Goregaokar [:manishearth]]

pref removal https://treeherder.mozilla.org/#/jobs?repo=try&revision=414ea73ffa2e812bdcb5ed8aac570385fdc230fc

Comment 6

[Manish Goregaokar [:manishearth]]

Comment on attachment 8870209 [details]
Bug 1366914 - Remove pref layout.css.background-clip-text.enabled;

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

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8870209 [details]
Bug 1366914 - Remove pref layout.css.background-clip-text.enabled;

https://reviewboard.mozilla.org/r/141658/#review145316

::: layout/style/test/property_database.js:7841
(Diff revision 2)
> -  gCSSProperties["background-clip"].other_values.push(
> +gCSSProperties["background-clip"].other_values.push(
> -    "text",
> +  "text",
> -    "content-box, text",
> +  "content-box, text",
> -    "text, border-box",
> +  "text, border-box",
> -    "text, text"
> +  "text, text"
> -  );
> +);
> -  gCSSProperties["background"].other_values.push(
> +gCSSProperties["background"].other_values.push(
> -    "url(404.png) green padding-box text",
> +  "url(404.png) green padding-box text",
> -    "content-box text url(404.png) blue"
> +  "content-box text url(404.png) blue"
> -  );
> +);

You can just merge these values to the table above.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8870209 [details]
Bug 1366914 - Remove pref layout.css.background-clip-text.enabled;

https://reviewboard.mozilla.org/r/141658/#review145480

r=me with nits:

::: commit-message-f9ca9:1
(Diff revision 2)
> +Bug 1366914 - Remove layout.css.background-clip-text.enabled; r?dholbert

s/Remove/Remove pref/

::: layout/style/test/property_database.js:7841
(Diff revision 2)
> -  gCSSProperties["background-clip"].other_values.push(
> +gCSSProperties["background-clip"].other_values.push(
> -    "text",
> +  "text",
> -    "content-box, text",
> +  "content-box, text",
> -    "text, border-box",
> +  "text, border-box",
> -    "text, text"
> +  "text, text"
> -  );
> +);
> -  gCSSProperties["background"].other_values.push(
> +gCSSProperties["background"].other_values.push(
> -    "url(404.png) green padding-box text",
> +  "url(404.png) green padding-box text",
> -    "content-box text url(404.png) blue"
> +  "content-box text url(404.png) blue"
> -  );
> +);

I agree with Xidorn -- please just directly add these to the arrays on the entry for "background-clip".

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> I suppose we may not want to unship
> it, so probably we can just remove this pref and support it unconditionally.
> 
> cjku, dholbert, what do you think?

Seems fine to me. In this case.  I'm not sure that will be the case for all the other keyword-table-stomping pref-callbacks in nsLayoutUtils.cpp, though.  (If we need to deal with this one for Stylo, I assume we'll need to deal with those others, too.)

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 8870209 [details]
Bug 1366914 - Remove pref layout.css.background-clip-text.enabled;

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/927cb2c2a059
Remove pref layout.css.background-clip-text.enabled; r=dholbert

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/927cb2c2a059