Closed Bug 1420117 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(5 files, 2 obsolete files)

I overlooked this one before, since I thought we didn't need anything from nsCSSRules.cpp (where nsCSSCounterStyle lived before bug 1420113).
Attachment #8931621 - Flags: review?(xidorn+moz)
Attachment #8931624 - Flags: review?(xidorn+moz)
Comment on attachment 8931626 [details]
Bug 1420117 - Part 4: Use Servo CSS parser in nsCSSCounterStyleRule::SetDescriptor.

https://reviewboard.mozilla.org/r/202788/#review208150
Attachment #8931626 - Flags: review?(xidorn+moz) → review+
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 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.
Attachment #8931625 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8931624 [details]
style: Add FFI function to parse a @counter-style descriptor.

https://reviewboard.mozilla.org/r/202784/#review208160
Attachment #8931624 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8931623 [details]
Bug 1420117 - Part 2: Use Servo CSS parser in nsCSSCounterStyleRule::SetName.

https://reviewboard.mozilla.org/r/202782/#review208164
Attachment #8931623 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8931622 [details]
Bug 1420117 - Part 1: Add a ServoCSSParser::ParseCounterStyleName method.

https://reviewboard.mozilla.org/r/202780/#review208166
Attachment #8931622 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8931621 [details]
geckolib: Add FFI function to parse a @counter-style name.

https://reviewboard.mozilla.org/r/202778/#review208168
Attachment #8931621 - Flags: review?(xidorn+moz) → review+
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 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.
Priority: -- → P3
Attachment #8931621 - Attachment is obsolete: true
Attachment #8931624 - Attachment is obsolete: true
Attached file Servo PR
Pushed by archaeopteryx@coole-files.de:
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
Backout by apavel@mozilla.com:
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
Pushed by cmccormack@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.