1408312 - use Servo for color parsing in various places where we currently use nsCSSParser

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

CanvasRenderingContext2D uses nsCSSParser for parsing colors in CanvasGradient::AddColorStop and CanvasRenderingContext2D::ParseColor.  This should change to calling into Servo.

Comment 1

[Cameron McCormack (:heycam)]

DocumentRendererChild::RenderDocument also parses a color.

Comment 2

[Cameron McCormack (:heycam)]

There's also nsPresContext::MakeColorPref.

Comment 3

[Cameron McCormack (:heycam)]

And also in inDOMUtils::ColorToRGBA and inDOMUtils::IsValidCSSColor.

Comment 4

[Cameron McCormack (:heycam)]

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

Comment 5

[Cameron McCormack (:heycam)]

Attachment: style: Refactor specified::Color parsing/computation functions.

So they can be called without a context.

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

Comment 6

[Cameron McCormack (:heycam)]

Attachment: geckolib: Add FFI functions for color parsing.

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

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1408312 - Part 1: Add ServoCSSParser utility class.

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

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Bug 1408312 - Part 2: Replace nsCSSParser usage in inDOMUtils::IsValidCSSColor.

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

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Bug 1408312 - Part 3: Replace nsCSSParser/nsRuleNode usage for color computation in Servo styled documents.

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

Comment 10

[Cameron McCormack (:heycam)]

Hmm, I realise now that we can't unconditionally use the Servo functions until we remove support for --disable-stylo, and that these patches would break current Android.

Comment 11

[Cameron McCormack (:heycam)]

Attachment: style: Refactor specified::Color parsing/computation functions.

So they can be called without a context.

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

Comment 12

[Cameron McCormack (:heycam)]

Attachment: geckolib: Add FFI functions for color parsing.

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

Comment 13

[Cameron McCormack (:heycam)]

Attachment: Bug 1408312 - Part 1: Add ServoCSSParser utility class.

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

Comment 14

[Cameron McCormack (:heycam)]

Attachment: Bug 1408312 - Part 2: Replace nsCSSParser usage in inDOMUtils::IsValidCSSColor.

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

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Bug 1408312 - Part 3: Replace nsCSSParser/nsRuleNode usage for color computation in Servo styled documents.

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

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8918739 [details]
geckolib: Add FFI functions for color parsing.

https://reviewboard.mozilla.org/r/189560/#review194712

::: servo/ports/geckolib/glue.rs:4211
(Diff revision 1)
>  pub unsafe extern "C" fn Servo_SelectorList_Drop(list: RawServoSelectorListOwned) {
>      let _ = list.into_box::<::selectors::SelectorList<SelectorImpl>>();
>  }
> +
> +#[no_mangle]
> +pub unsafe extern "C" fn Servo_IsValidCSSColor(

Can we not mark the whole function `unsafe`...? Especially `Servo_ComputeColor`, I guess, which isn't that trivial.

::: servo/ports/geckolib/glue.rs:4214
(Diff revision 1)
> +    use cssparser::{Parser, ParserInput};
> +    use style::values::specified;
> +
> +    let value = (*value).to_string();
> +
> +    let mut input = ParserInput::new(&value);
> +    let mut parser = Parser::new(&mut input);
> +
> +    let result = parser.parse_entirely(specified::Color::parse_color);

You may want to move this part into a separate function so that it can be reused by both FFI functions.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8918741 [details]
Bug 1408312 - Part 2: Replace nsCSSParser usage in inDOMUtils::IsValidCSSColor.

https://reviewboard.mozilla.org/r/189564/#review194714

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8918742 [details]
Bug 1408312 - Part 3: Replace nsCSSParser/nsRuleNode usage for color computation in Servo styled documents.

https://reviewboard.mozilla.org/r/189566/#review194716

::: dom/canvas/CanvasRenderingContext2D.cpp:752
(Diff revision 1)
>    }
>  
> +  nscolor color;
> +  bool ok;
> +
> +  nsCOMPtr<nsIPresShell> shell = mContext ? mContext->GetPresShell() : nullptr;

It doesn't seem to me you need to hold a strong reference to shell?

::: layout/inspector/inDOMUtils.cpp
(Diff revision 1)
>    if (!isColor) {
>      aValue.setNull();
>      return NS_OK;
>    }
>  
> -  nsRuleNode::ComputeColor(cssValue, nullptr, nullptr, color);

This line should be moved before `#endif` rather than get removed, shouldn't it?

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8918740 [details]
Bug 1408312 - Part 1: Add ServoCSSParser utility class.

https://reviewboard.mozilla.org/r/189562/#review194722

Comment 20

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72c8a5a4a78c8f137cb7197cf7c91788072d0804

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8918740 [details]
Bug 1408312 - Part 1: Add ServoCSSParser utility class.

https://reviewboard.mozilla.org/r/189562/#review194724

::: layout/style/ServoCSSParser.h:37
(Diff revision 1)
> +  static bool ComputeColor(ServoStyleSet* aStyleSet,
> +                           nscolor aCurrentColor,
> +                           const nsAString& aValue,
> +                           nscolor* aResultColor);

You can use `Maybe<nscolor>` instead if you want.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8918739 [details]
geckolib: Add FFI functions for color parsing.

https://reviewboard.mozilla.org/r/189560/#review194726

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8918738 [details]
style: Refactor specified::Color parsing/computation functions.

https://reviewboard.mozilla.org/r/189558/#review194728

Comment 24

[Cameron McCormack (:heycam)]

Comment on attachment 8918742 [details]
Bug 1408312 - Part 3: Replace nsCSSParser/nsRuleNode usage for color computation in Servo styled documents.

https://reviewboard.mozilla.org/r/189566/#review194716

> This line should be moved before `#endif` rather than get removed, shouldn't it?

Yes, thanks for spotting that.  (I decided to duplicate the early return stuff instead of having it outside the ifdef.)

Comment 25

[Cameron McCormack (:heycam)]

Comment on attachment 8918740 [details]
Bug 1408312 - Part 1: Add ServoCSSParser utility class.

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

Comment 26

[Cameron McCormack (:heycam)]

Comment on attachment 8918741 [details]
Bug 1408312 - Part 2: Replace nsCSSParser usage in inDOMUtils::IsValidCSSColor.

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

Comment 27

[Cameron McCormack (:heycam)]

Comment on attachment 8918742 [details]
Bug 1408312 - Part 3: Replace nsCSSParser/nsRuleNode usage for color computation in Servo styled documents.

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bcffb222c970
Part 1: Add ServoCSSParser utility class. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/93065f7849c5
Part 2: Replace nsCSSParser usage in inDOMUtils::IsValidCSSColor. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/f13bc708c440
Part 3: Replace nsCSSParser/nsRuleNode usage for color computation in Servo styled documents. r=xidorn

Comment 29

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

Backed out for failing web-platform-test /2dcontext/fill-and-stroke-styles/2d.gradient.object.current.html with stylo disabled:

https://hg.mozilla.org/integration/autoland/rev/3202667b1bec2828401f19112422249fff274291
https://hg.mozilla.org/integration/autoland/rev/3997a9aeb2d7510e8116dc3a2d6eaefb4b836a3f
https://hg.mozilla.org/integration/autoland/rev/93bb88c3fbe100f2334c699789ea1b163681171d

Push which ran failing tests: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d5d5e0d4ef333dd704469654bb9567d6d4ce1c4e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=137220384&repo=autoland

[task 2017-10-16T12:09:08.288Z] 12:09:08     INFO - TEST-START | /2dcontext/fill-and-stroke-styles/2d.gradient.object.current.html
[task 2017-10-16T12:09:08.349Z] 12:09:08     INFO - PID 1050 | 1508155748343	Marionette	DEBUG	Register listener.js for window 2147484118
[task 2017-10-16T12:09:08.506Z] 12:09:08     INFO - 
[task 2017-10-16T12:09:08.507Z] 12:09:08     INFO - TEST-UNEXPECTED-FAIL | /2dcontext/fill-and-stroke-styles/2d.gradient.object.current.html | Canvas test: 2d.gradient.object.current - An invalid or illegal string was specified
[task 2017-10-16T12:09:08.507Z] 12:09:08     INFO - @http://web-platform.test:8000/2dcontext/fill-and-stroke-styles/2d.gradient.object.current.html:28:1
[task 2017-10-16T12:09:08.509Z] 12:09:08     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1485:20
[task 2017-10-16T12:09:08.509Z] 12:09:08     INFO - _addTest/</<@http://web-platform.test:8000/common/canvas-tests.js:62:13
[task 2017-10-16T12:09:08.509Z] 12:09:08     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1485:20
[task 2017-10-16T12:09:08.509Z] 12:09:08     INFO - _addTest/<@http://web-platform.test:8000/common/canvas-tests.js:59:9
[task 2017-10-16T12:09:08.509Z] 12:09:08     INFO - EventListener.handleEvent*on_event@http://web-platform.test:8000/resources/testharness.js:712:9
[task 2017-10-16T12:09:08.509Z] 12:09:08     INFO - _addTest@http://web-platform.test:8000/common/canvas-tests.js:57:5
[task 2017-10-16T12:09:08.509Z] 12:09:08     INFO - @http://web-platform.test:8000/2dcontext/fill-and-stroke-styles/2d.gradient.object.current.html:20:1
[task 2017-10-16T12:09:08.509Z] 12:09:08     INFO - TEST-OK | /2dcontext/fill-and-stroke-styles/2d.gradient.object.current.html | took 217ms

Comment 30

[Cameron McCormack (:heycam)]

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

Comment 31

[Cameron McCormack (:heycam)]

Comment on attachment 8918740 [details]
Bug 1408312 - Part 1: Add ServoCSSParser utility class.

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

Comment 32

[Cameron McCormack (:heycam)]

Comment on attachment 8918741 [details]
Bug 1408312 - Part 2: Replace nsCSSParser usage in inDOMUtils::IsValidCSSColor.

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

Comment 33

[Cameron McCormack (:heycam)]

Comment on attachment 8918742 [details]
Bug 1408312 - Part 3: Replace nsCSSParser/nsRuleNode usage for color computation in Servo styled documents.

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

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e36022d5347
Part 1: Add ServoCSSParser utility class. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/9fb995f9e984
Part 2: Replace nsCSSParser usage in inDOMUtils::IsValidCSSColor. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/4e4073c0e143
Part 3: Replace nsCSSParser/nsRuleNode usage for color computation in Servo styled documents. r=xidorn

Comment 35

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

https://hg.mozilla.org/mozilla-central/rev/2e36022d5347
https://hg.mozilla.org/mozilla-central/rev/9fb995f9e984
https://hg.mozilla.org/mozilla-central/rev/4e4073c0e143