Closed Bug 1420117 Opened 7 years ago Closed 7 years ago

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

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla59

Tracking Flags:

Tracking

Status

firefox59

---

fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(5 files, 2 obsolete files)

geckolib: Add FFI function to parse a @counter-style name. 7 years ago Cameron McCormack (:heycam) 59 bytes, text/x-review-board-request	xidorn : review+	Details
Bug 1420117 - Part 1: Add a ServoCSSParser::ParseCounterStyleName method. 7 years ago Cameron McCormack (:heycam) 59 bytes, text/x-review-board-request	xidorn : review+	Details
Bug 1420117 - Part 2: Use Servo CSS parser in nsCSSCounterStyleRule::SetName. 7 years ago Cameron McCormack (:heycam) 59 bytes, text/x-review-board-request	xidorn : review+	Details
style: Add FFI function to parse a @counter-style descriptor. 7 years ago Cameron McCormack (:heycam) 59 bytes, text/x-review-board-request	xidorn : review+	Details
Bug 1420117 - Part 3: Add a ServoCSSParser::ParseCounterStyleDescriptor method. 7 years ago Cameron McCormack (:heycam) 59 bytes, text/x-review-board-request	xidorn : review+	Details
Bug 1420117 - Part 4: Use Servo CSS parser in nsCSSCounterStyleRule::SetDescriptor. 7 years ago Cameron McCormack (:heycam) 59 bytes, text/x-review-board-request	xidorn : review+	Details
Servo PR 7 years ago Cameron McCormack (:heycam) 41 bytes, text/x-github-pull-request		Details \| Review

Cameron McCormack (:heycam)

Assignee

Description

•

7 years ago

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

Cameron McCormack (:heycam)

Assignee

Comment 1

•

7 years ago

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

Assignee: nobody → cam

Status: NEW → ASSIGNED

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Cameron McCormack (:heycam)

Assignee

Updated

•

7 years ago

Attachment #8931621 - Flags: review?(xidorn+moz)

Attachment #8931624 - Flags: review?(xidorn+moz)

Xidorn Quan [:xidorn] UTC+11

Comment 8

•

7 years ago

mozreview-review

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+

Xidorn Quan [:xidorn] UTC+11

Comment 9

•

7 years ago

mozreview-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.

Xidorn Quan [:xidorn] UTC+11

Comment 10

•

7 years ago

mozreview-review

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+

Xidorn Quan [:xidorn] UTC+11

Comment 11

•

7 years ago

mozreview-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+

Xidorn Quan [:xidorn] UTC+11

Comment 12

•

7 years ago

mozreview-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+

Xidorn Quan [:xidorn] UTC+11

Comment 13

•

7 years ago

mozreview-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+

Xidorn Quan [:xidorn] UTC+11

Comment 14

•

7 years ago

mozreview-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+

Cameron McCormack (:heycam)

Assignee

Comment 15

•

7 years ago

mozreview-review-reply

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.

Cameron McCormack (:heycam)

Assignee

Comment 16

•

7 years ago

mozreview-review

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.

Jet Villegas (inactive)

Updated

•

7 years ago

Priority: -- → P3

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Cameron McCormack (:heycam)

Assignee

Updated

•

7 years ago

Attachment #8931621 - Attachment is obsolete: true

Cameron McCormack (:heycam)

Assignee

Updated

•

7 years ago

Attachment #8931624 - Attachment is obsolete: true

Cameron McCormack (:heycam)

Assignee

Comment 21

•

7 years ago

Attached file Servo PR — Details

Pulsebot

Comment 22

•

7 years ago

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

Andreea Pavel [:apavel]

Comment 23

•

7 years ago

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

Flags: needinfo?(cam)

Pulsebot

Comment 24

•

7 years ago

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

Cameron McCormack (:heycam)

Assignee

Comment 25

•

7 years ago

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

Flags: needinfo?(cam)

Pulsebot

Comment 26

•

7 years ago

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

Cosmin Sabou [:CosminS]

Comment 27

•

7 years ago

bugherder

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

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox59: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla59

You need to log in before you can comment on or make changes to this bug.