Closed Bug 1408312 Opened 5 years ago Closed 5 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(3 files, 7 obsolete files)

Bug 1408312 - Part 1: Add ServoCSSParser utility class.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1408312 - Part 2: Replace nsCSSParser usage in inDOMUtils::IsValidCSSColor.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1408312 - Part 3: Replace nsCSSParser/nsRuleNode usage for color computation in Servo styled documents.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
CanvasRenderingContext2D uses nsCSSParser for parsing colors in CanvasGradient::AddColorStop and CanvasRenderingContext2D::ParseColor.  This should change to calling into Servo.
DocumentRendererChild::RenderDocument also parses a color.
Summary: make CanvasRenderingContext2D use Servo for color parsing → make CanvasRenderingContext2D and DocumentRendererChild use Servo for color parsing
There's also nsPresContext::MakeColorPref.
Summary: make CanvasRenderingContext2D and DocumentRendererChild use Servo for color parsing → make CanvasRenderingContext2D, DocumentRendererChild and nsPresContext use Servo for color parsing
And also in inDOMUtils::ColorToRGBA and inDOMUtils::IsValidCSSColor.
Summary: make CanvasRenderingContext2D, DocumentRendererChild and nsPresContext use Servo for color parsing → use Servo for color parsing in various places where we currently use nsCSSParser
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8918722 - Flags: review?(xidorn+moz)
Attachment #8918723 - Flags: review?(xidorn+moz)
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.
Attachment #8918722 - Attachment is obsolete: true
Attachment #8918722 - Flags: review?(xidorn+moz)
Attachment #8918723 - Attachment is obsolete: true
Attachment #8918723 - Flags: review?(xidorn+moz)
Attachment #8918724 - Attachment is obsolete: true
Attachment #8918724 - Flags: review?(xidorn+moz)
Attachment #8918725 - Attachment is obsolete: true
Attachment #8918725 - Flags: review?(xidorn+moz)
Attachment #8918726 - Attachment is obsolete: true
Attachment #8918726 - Flags: review?(xidorn+moz)
Attachment #8918738 - Flags: review?(xidorn+moz)
Attachment #8918739 - Flags: review?(xidorn+moz)
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 on attachment 8918741 [details]
Bug 1408312 - Part 2: Replace nsCSSParser usage in inDOMUtils::IsValidCSSColor.

https://reviewboard.mozilla.org/r/189564/#review194714
Attachment #8918741 - Flags: review?(xidorn+moz) → review+
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?
Attachment #8918742 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8918740 [details]
Bug 1408312 - Part 1: Add ServoCSSParser utility class.

https://reviewboard.mozilla.org/r/189562/#review194722
Attachment #8918740 - Flags: review?(xidorn+moz) → review+
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 on attachment 8918739 [details]
geckolib: Add FFI functions for color parsing.

https://reviewboard.mozilla.org/r/189560/#review194726
Attachment #8918739 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8918738 [details]
style: Refactor specified::Color parsing/computation functions.

https://reviewboard.mozilla.org/r/189558/#review194728
Attachment #8918738 - Flags: review?(xidorn+moz) → review+
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.)
Attachment #8918738 - Attachment is obsolete: true
Attachment #8918739 - Attachment is obsolete: true
Pushed by cmccormack@mozilla.com:
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
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
Flags: needinfo?(cam)
Pushed by cmccormack@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.