1420117 - replace uses of nsCSSParser in nsCSSCounterStyleRule.cpp with Servo parser calls

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

I overlooked this one before, since I thought we didn't need anything from nsCSSRules.cpp (where nsCSSCounterStyle lived before bug 1420113).

Comment 1

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57df79b34282ec3621c336b126333b0eb5d2297c

Comment 2

[Cameron McCormack (:heycam)]

Attachment: geckolib: Add FFI function to parse a @counter-style name.

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

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1420117 - Part 1: Add a ServoCSSParser::ParseCounterStyleName method.

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

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Bug 1420117 - Part 2: Use Servo CSS parser in nsCSSCounterStyleRule::SetName.

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

Comment 5

[Cameron McCormack (:heycam)]

Attachment: style: Add FFI function to parse a @counter-style descriptor.

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

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Bug 1420117 - Part 3: Add a ServoCSSParser::ParseCounterStyleDescriptor method.

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

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1420117 - Part 4: Use Servo CSS parser in nsCSSCounterStyleRule::SetDescriptor.

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8931626 [details]
Bug 1420117 - Part 4: Use Servo CSS parser in nsCSSCounterStyleRule::SetDescriptor.

https://reviewboard.mozilla.org/r/202788/#review208150

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8931626 [details]
Bug 1420117 - Part 4: Use Servo CSS parser in nsCSSCounterStyleRule::SetDescriptor.

https://reviewboard.mozilla.org/r/202788/#review208152

::: layout/style/nsCSSCounterStyleRule.cpp:430
(Diff revision 1)
>    nsCSSValue value;
> -  nsIURI* baseURL = nullptr;
> -  nsIPrincipal* principal = nullptr;
> -  if (StyleSheet* sheet = GetStyleSheet()) {
> -    baseURL = sheet->GetBaseURI();
> -    principal = sheet->Principal();
> +  bool ok;
> +
> +  StyleSheet* sheet = GetStyleSheet();
> +
> +  if (!sheet || sheet->IsServo()) {

I'm a bit worry about this. Since stylo can still be disabled (and it is still disabled in some configurations), probably this kind of code should be guarded by `MOZ_STYLO` somehow.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8931625 [details]
Bug 1420117 - Part 3: Add a ServoCSSParser::ParseCounterStyleDescriptor method.

https://reviewboard.mozilla.org/r/202786/#review208154

::: layout/style/ServoCSSParser.h:70
(Diff revision 1)
> +   * @param aURLExtraData URL data for parsing. This would be used for
> +   *   image value URL resolution.

I don't think we support image value URL in counter-style descriptor at the moment, though.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8931624 [details]
style: Add FFI function to parse a @counter-style descriptor.

https://reviewboard.mozilla.org/r/202784/#review208160

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8931623 [details]
Bug 1420117 - Part 2: Use Servo CSS parser in nsCSSCounterStyleRule::SetName.

https://reviewboard.mozilla.org/r/202782/#review208164

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8931622 [details]
Bug 1420117 - Part 1: Add a ServoCSSParser::ParseCounterStyleName method.

https://reviewboard.mozilla.org/r/202780/#review208166

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8931621 [details]
geckolib: Add FFI function to parse a @counter-style name.

https://reviewboard.mozilla.org/r/202778/#review208168

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8931626 [details]
Bug 1420117 - Part 4: Use Servo CSS parser in nsCSSCounterStyleRule::SetDescriptor.

https://reviewboard.mozilla.org/r/202788/#review208152

> I'm a bit worry about this. Since stylo can still be disabled (and it is still disabled in some configurations), probably this kind of code should be guarded by `MOZ_STYLO` somehow.

Yeah, I only did it this way because my forthcoming patches to add #ifdef MOZ_OLD_STYLE to remove the old style system would be slightly simpler here.  I can preserve the current behavior of using the Gecko path if the sheet is null, when the old style system is still enabled.

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8931625 [details]
Bug 1420117 - Part 3: Add a ServoCSSParser::ParseCounterStyleDescriptor method.

https://reviewboard.mozilla.org/r/202786/#review208274

::: layout/style/ServoCSSParser.h:70
(Diff revision 1)
> +   * @param aURLExtraData URL data for parsing. This would be used for
> +   *   image value URL resolution.

Yes, you are right.  (I saw the enum value is commented out currently.)  But may as well pass it in now.

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8931622 [details]
Bug 1420117 - Part 1: Add a ServoCSSParser::ParseCounterStyleName method.

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

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8931623 [details]
Bug 1420117 - Part 2: Use Servo CSS parser in nsCSSCounterStyleRule::SetName.

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

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8931625 [details]
Bug 1420117 - Part 3: Add a ServoCSSParser::ParseCounterStyleDescriptor method.

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

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8931626 [details]
Bug 1420117 - Part 4: Use Servo CSS parser in nsCSSCounterStyleRule::SetDescriptor.

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

Comment 21

[Cameron McCormack (:heycam)]

Attachment: Servo PR

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7292e7fee0e
Part 1: Add a ServoCSSParser::ParseCounterStyleName method. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/83b36cccea28
Part 2: Use Servo CSS parser in nsCSSCounterStyleRule::SetName. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/5cceea9740eb
Part 3: Add a ServoCSSParser::ParseCounterStyleDescriptor method. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/2560a150250d
Part 4: Use Servo CSS parser in nsCSSCounterStyleRule::SetDescriptor. r=xidorn

Comment 23

[Andreea Pavel [:apavel]]

Backed out for failing mochitest failures layout/style/test/test_counter_descriptor_storage.html r=backout on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2560a150250d769e02fa843d4f254e347dba11ff&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=149687314&repo=autoland&lineNumber=3959

Backout: https://hg.mozilla.org/integration/autoland/rev/ef99e0f85f43877252d3f39793e48041dd42d8a8

Comment 24

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9037e227d8ac
Backed out 1 changesets because the gecko part has to be backed out for the mochitests r=backout on a CLOSED TREE

Comment 25

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88ce370b5e8c03bfb62d1ed7a4392fd00122df9c

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/127afca34613
Part 1: Add a ServoCSSParser::ParseCounterStyleName method. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/6596e7cf4ec5
Part 2: Use Servo CSS parser in nsCSSCounterStyleRule::SetName. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/734af481e8e6
Part 3: Add a ServoCSSParser::ParseCounterStyleDescriptor method. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/434d211ad7c9
Part 4: Use Servo CSS parser in nsCSSCounterStyleRule::SetDescriptor. r=xidorn

Comment 27

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/127afca34613
https://hg.mozilla.org/mozilla-central/rev/6596e7cf4ec5
https://hg.mozilla.org/mozilla-central/rev/734af481e8e6
https://hg.mozilla.org/mozilla-central/rev/434d211ad7c9