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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: heycam, Assigned: heycam)
References
Details
Attachments
(3 files, 7 obsolete files)
CanvasRenderingContext2D uses nsCSSParser for parsing colors in CanvasGradient::AddColorStop and CanvasRenderingContext2D::ParseColor. This should change to calling into Servo.
Assignee | ||
Comment 1•5 years ago
|
||
DocumentRendererChild::RenderDocument also parses a color.
Summary: make CanvasRenderingContext2D use Servo for color parsing → make CanvasRenderingContext2D and DocumentRendererChild use Servo for color parsing
Assignee | ||
Comment 2•5 years ago
|
||
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
Assignee | ||
Comment 3•5 years ago
|
||
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 | ||
Updated•5 years ago
|
Assignee: nobody → cam
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b83035eea0b41f8818ea229129cdba0e16a72f97
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•5 years ago
|
Attachment #8918722 -
Flags: review?(xidorn+moz)
Attachment #8918723 -
Flags: review?(xidorn+moz)
Assignee | ||
Comment 10•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Attachment #8918722 -
Attachment is obsolete: true
Attachment #8918722 -
Flags: review?(xidorn+moz)
Assignee | ||
Updated•5 years ago
|
Attachment #8918723 -
Attachment is obsolete: true
Attachment #8918723 -
Flags: review?(xidorn+moz)
Assignee | ||
Updated•5 years ago
|
Attachment #8918724 -
Attachment is obsolete: true
Attachment #8918724 -
Flags: review?(xidorn+moz)
Assignee | ||
Updated•5 years ago
|
Attachment #8918725 -
Attachment is obsolete: true
Attachment #8918725 -
Flags: review?(xidorn+moz)
Assignee | ||
Updated•5 years ago
|
Attachment #8918726 -
Attachment is obsolete: true
Attachment #8918726 -
Flags: review?(xidorn+moz)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•5 years ago
|
Attachment #8918738 -
Flags: review?(xidorn+moz)
Attachment #8918739 -
Flags: review?(xidorn+moz)
Comment 16•5 years ago
|
||
mozreview-review |
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•5 years ago
|
||
mozreview-review |
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 18•5 years ago
|
||
mozreview-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 19•5 years ago
|
||
mozreview-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+
Assignee | ||
Comment 20•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=72c8a5a4a78c8f137cb7197cf7c91788072d0804
Comment 21•5 years ago
|
||
mozreview-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 22•5 years ago
|
||
mozreview-review |
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 23•5 years ago
|
||
mozreview-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+
Assignee | ||
Comment 24•5 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•5 years ago
|
Attachment #8918738 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
Attachment #8918739 -
Attachment is obsolete: true
Comment 28•5 years ago
|
||
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
![]() |
||
Comment 29•5 years ago
|
||
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)
Assignee | ||
Comment 30•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b811ffff2d822b12a5a2d81bb094bce4a66c11ea
Flags: needinfo?(cam)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 34•5 years ago
|
||
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
![]() |
||
Comment 35•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2e36022d5347 https://hg.mozilla.org/mozilla-central/rev/9fb995f9e984 https://hg.mozilla.org/mozilla-central/rev/4e4073c0e143
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•