Implemented the plumbing for paint worklets #17150

asajeffrey · 2017-06-03T17:26:19Z

This PR implements the plumbing for paint worklets:

Adding CSS values for paint worklets.
Implementing a skeleton for the PaintWorkletGlobalScope webidl.
Implementing an executor for paint worklet tasks, and passing it from script to layout.
Building the display list items for paint worklet images.

This PR does not implement registering or calling paint worklets in JS.

Before it merges, this PR needs a reftest added for basic paint worklet functionality.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
There are tests for these changes

This change is

highfive · 2017-06-03T17:26:24Z

Heads up! This PR modifies the following files:

@bholley: components/style/values/specified/image.rs, components/style/values/generics/image.rs
@KiChjang: components/script_layout_interface/message.rs, components/script/dom/webidls/PaintWorkletGlobalScope.webidl, components/script/dom/mod.rs, components/script_traits/Cargo.toml, components/script_traits/Cargo.toml and 10 more
@fitzgen: components/script_layout_interface/message.rs, components/script/dom/webidls/PaintWorkletGlobalScope.webidl, components/script/dom/mod.rs, components/script_traits/Cargo.toml, components/script_traits/Cargo.toml and 7 more
@emilio: components/layout/display_list_builder.rs, components/style/values/specified/image.rs, components/style/values/generics/image.rs, components/layout/context.rs

highfive · 2017-06-03T17:26:25Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify style, layout, net, and script code, but no tests are modified. Please consider adding a test!

asajeffrey · 2017-06-03T17:26:50Z

cc @jdm @metajack @tschneidereit

emilio · 2017-06-03T17:57:47Z

components/style/values/specified/image.rs

@@ -98,6 +99,9 @@ impl Parse for Image {
        if let Ok(gradient) = input.try(|i| Gradient::parse(context, i)) {
            return Ok(GenericImage::Gradient(gradient));
        }
+        if let Ok(paint_worklet) = input.try(|i| PaintWorklet::parse(context, i)) {


You're going to remove this path for Gecko, I think.

Err, that sounded quite inquisitive, I meant "You're going to need to remove" :)

emilio · 2017-06-03T17:58:20Z

components/style/values/specified/image.rs

@@ -673,6 +677,18 @@ impl Parse for ColorStop {
    }
 }

+impl Parse for PaintWorklet {
+    fn parse(_context: &ParserContext, input: &mut Parser) -> Result<Self, ()> {
+        input.try(|i| i.expect_function_matching("paint"))?;


No input.try needed here, afaict.

emilio · 2017-06-03T17:59:51Z

components/script/dom/worklet.rs

+        let task = WorkletTask::Paint(PaintWorkletTask::DrawAPaintImage(name, concrete_object_size, sender));
+        let timeout = Duration::from_millis(PAINT_TIMEOUT_MILLISECONDS);
+        self.schedule_a_worklet_task(task);
+        receiver.recv_timeout(timeout)?


Heh, glad recv_timeout was useful after all!

emilio · 2017-06-03T18:00:30Z

components/script/dom/paintworkletglobalscope.rs

+        }
+    }
+
+    pub fn draw_a_paint_image(&self,


Probably no need to be pub?

emilio · 2017-06-03T18:02:07Z

components/script/dom/window.rs

@@ -371,6 +374,14 @@ impl Window {
        self.webvr_thread.clone()
    }

+    pub fn new_paint_worklet(&self) -> Root<Worklet> {


no need for pub here either.

emilio · 2017-06-03T18:03:14Z

components/layout_thread/lib.rs

@@ -689,6 +695,10 @@ impl LayoutThread {
            Msg::SetFinalUrl(final_url) => {
                self.url = final_url;
            },
+            Msg::SetPaintWorkletExecutor(executor) => {
+                debug!("Setting the paint worklet executor");
+                self.paint_worklet_executor = Some(executor);


debug_assert!(self.paint_worklet_executor.is_none())? This would need to be refactored if/when we fix #14885

asajeffrey

Thanks for the quick review!

asajeffrey · 2017-06-03T19:13:13Z

components/layout_thread/lib.rs

@@ -689,6 +695,10 @@ impl LayoutThread {
            Msg::SetFinalUrl(final_url) => {
                self.url = final_url;
            },
+            Msg::SetPaintWorkletExecutor(executor) => {
+                debug!("Setting the paint worklet executor");
+                self.paint_worklet_executor = Some(executor);


asajeffrey · 2017-06-03T19:13:58Z

components/script/dom/paintworkletglobalscope.rs

+        }
+    }
+
+    pub fn draw_a_paint_image(&self,


asajeffrey · 2017-06-03T19:14:39Z

components/script/dom/window.rs

@@ -371,6 +374,14 @@ impl Window {
        self.webvr_thread.clone()
    }

+    pub fn new_paint_worklet(&self) -> Root<Worklet> {


asajeffrey · 2017-06-03T19:15:01Z

components/script/dom/worklet.rs

+        let task = WorkletTask::Paint(PaintWorkletTask::DrawAPaintImage(name, concrete_object_size, sender));
+        let timeout = Duration::from_millis(PAINT_TIMEOUT_MILLISECONDS);
+        self.schedule_a_worklet_task(task);
+        receiver.recv_timeout(timeout)?


asajeffrey · 2017-06-03T19:15:48Z

components/style/values/specified/image.rs

@@ -98,6 +99,9 @@ impl Parse for Image {
        if let Ok(gradient) = input.try(|i| Gradient::parse(context, i)) {
            return Ok(GenericImage::Gradient(gradient));
        }
+        if let Ok(paint_worklet) = input.try(|i| PaintWorklet::parse(context, i)) {


asajeffrey · 2017-06-03T19:17:39Z

components/style/values/specified/image.rs

@@ -673,6 +677,18 @@ impl Parse for ColorStop {
    }
 }

+impl Parse for PaintWorklet {
+    fn parse(_context: &ParserContext, input: &mut Parser) -> Result<Self, ()> {
+        input.try(|i| i.expect_function_matching("paint"))?;


emilio

The CSS and display list building bits look fine to me with the nits, assuming build_display_list_for_webrender_image is just moved code.

The dom bits look harmless, but I haven't been following along enough the worklet stuff to consider myself confident enough to stamp it, so better if you get @jdm to do that.

Also, as you've already pointed out, a test would be quite nice :)

emilio · 2017-06-03T19:42:49Z

components/script/dom/paintworkletglobalscope.rs

+    fn draw_a_paint_image(&self,
+                              name: Atom,
+                              concrete_object_size: Size2D<Au>,
+                              sender: Sender<Result<Image, PaintWorkletError>>)


nit: indentation :)

emilio · 2017-06-03T19:43:31Z

components/script/dom/worklet.rs

@@ -561,7 +578,9 @@ impl WorkletThread {
        // TODO: Caching.
        // TODO: Avoid re-parsing the origin as a URL.
        let resource_fetcher = self.global_init.resource_threads.sender();
-        let origin_url = ServoUrl::parse(&*origin.unicode_serialization()).expect("Failed to parse origin as URL.");
+        let origin_url = ServoUrl::parse(&*origin.unicode_serialization())
+            .or_else(|_| ServoUrl::parse("about:blank"))


nit: unwrap_or_else instead?

emilio · 2017-06-03T19:45:48Z

components/style/values/generics/image.rs

+impl ToCss for PaintWorklet {
+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+        dest.write_str("paint(")?;
+        dest.write_str(&*self.name)?;


this needs to use serialize_identifier to handle escaping properly.

asajeffrey

Thanks!

asajeffrey · 2017-06-03T20:37:32Z

components/script/dom/paintworkletglobalscope.rs

+    fn draw_a_paint_image(&self,
+                              name: Atom,
+                              concrete_object_size: Size2D<Au>,
+                              sender: Sender<Result<Image, PaintWorkletError>>)


asajeffrey · 2017-06-03T20:40:57Z

components/script/dom/worklet.rs

@@ -561,7 +578,9 @@ impl WorkletThread {
        // TODO: Caching.
        // TODO: Avoid re-parsing the origin as a URL.
        let resource_fetcher = self.global_init.resource_threads.sender();
-        let origin_url = ServoUrl::parse(&*origin.unicode_serialization()).expect("Failed to parse origin as URL.");
+        let origin_url = ServoUrl::parse(&*origin.unicode_serialization())
+            .or_else(|_| ServoUrl::parse("about:blank"))


asajeffrey · 2017-06-03T20:42:49Z

components/style/values/generics/image.rs

+impl ToCss for PaintWorklet {
+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+        dest.write_str("paint(")?;
+        dest.write_str(&*self.name)?;


asajeffrey · 2017-06-03T20:46:25Z

r? @jdm do you want to have a look at the script/worklet side of things?

jdm · 2017-06-05T15:17:06Z

components/script/dom/paintworkletglobalscope.rs

+pub struct PaintWorkletGlobalScope {
+    // The worklet global for this object
+    worklet_global: WorkletGlobalScope,
+    // A buffer to draw into


Make these ///?

jdm · 2017-06-05T15:18:01Z

components/script/dom/paintworkletglobalscope.rs

+        debug!("Creating paint worklet global scope for pipeline {}.", pipeline_id);
+        let global = box PaintWorkletGlobalScope {
+            worklet_global: WorkletGlobalScope::new_inherited(pipeline_id, base_url, init),
+            buffer: DOMRefCell::new(Vec::new()),


Default::default()

jdm · 2017-06-05T15:19:31Z

components/script/dom/paintworkletglobalscope.rs

+                          name: Atom,
+                          concrete_object_size: Size2D<Au>,
+                          sender: Sender<Result<Image, PaintWorkletError>>)
+    {


nit: { on the previous line

Fixed, though I prefer the brace-on-its-own-line style when the fn defn ended up being multi-line.

asajeffrey

Made changes.

asajeffrey · 2017-06-06T16:36:25Z

components/script/dom/paintworkletglobalscope.rs

+pub struct PaintWorkletGlobalScope {
+    // The worklet global for this object
+    worklet_global: WorkletGlobalScope,
+    // A buffer to draw into


asajeffrey · 2017-06-06T16:36:35Z

components/script/dom/paintworkletglobalscope.rs

+        debug!("Creating paint worklet global scope for pipeline {}.", pipeline_id);
+        let global = box PaintWorkletGlobalScope {
+            worklet_global: WorkletGlobalScope::new_inherited(pipeline_id, base_url, init),
+            buffer: DOMRefCell::new(Vec::new()),


asajeffrey · 2017-06-06T16:37:22Z

components/script/dom/paintworkletglobalscope.rs

+                          name: Atom,
+                          concrete_object_size: Size2D<Au>,
+                          sender: Sender<Result<Image, PaintWorkletError>>)
+    {


Fixed, though I prefer the brace-on-its-own-line style when the fn defn ended up being multi-line.

jdm · 2017-06-06T17:30:00Z

Looks good. Just needs squashing and a test.

jdm · 2017-06-06T17:30:33Z

And this:

cargo build -v

error: failed to parse lock file at: /home/travis/build/servo/servo/Cargo.lock

Caused by:

  package `app_units 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)` is specified as a dependency, but is missing from the package list

asajeffrey · 2017-06-07T18:50:42Z

@bors-servo r=jdm

bors-servo · 2017-06-07T18:50:42Z

📌 Commit fd17dcd has been approved by jdm

bors-servo · 2017-06-07T18:57:08Z

⌛ Testing commit fd17dcd with merge bf46da0...

Implemented the plumbing for paint worklets  This PR implements the plumbing for paint worklets: * Adding CSS values for paint worklets. * Implementing a skeleton for the `PaintWorkletGlobalScope` webidl. * Implementing an executor for paint worklet tasks, and passing it from script to layout. * Building the display list items for paint worklet images. This PR does not implement registering or calling paint worklets in JS. Before it merges, this PR needs a reftest added for basic paint worklet functionality. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] There are tests for these changes    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17150)

bors-servo · 2017-06-07T19:49:30Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing bf46da0 to master...

asajeffrey added A-content/css Interacting with CSS from web content (parsing, serializing, introspection) A-content/script Related to the script thread S-needs-tests New tests have been requested by a reviewer. labels Jun 3, 2017

highfive assigned emilio Jun 3, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 3, 2017

emilio reviewed Jun 3, 2017

View reviewed changes

asajeffrey commented Jun 3, 2017

View reviewed changes

emilio approved these changes Jun 3, 2017

View reviewed changes

asajeffrey commented Jun 3, 2017

View reviewed changes

highfive assigned jdm and unassigned emilio Jun 3, 2017

asajeffrey pushed a commit to asajeffrey/servo that referenced this pull request Jun 5, 2017

PR servo#17150

57a12c6

jdm requested changes Jun 5, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 5, 2017

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jun 6, 2017

asajeffrey commented Jun 6, 2017

View reviewed changes

jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 6, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 7, 2017

asajeffrey force-pushed the script-paint-worklets-plumbing branch from bc84493 to e8db3ea Compare June 7, 2017 18:35

Implemented the plumbing for paint worklets.

fd17dcd

asajeffrey force-pushed the script-paint-worklets-plumbing branch from 2e133b0 to fd17dcd Compare June 7, 2017 18:49

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Jun 7, 2017

bors-servo merged commit fd17dcd into servo:master Jun 7, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jun 7, 2017

bors-servo mentioned this pull request Jun 7, 2017

Properly set origin of fetch requests #16508

Merged

5 tasks

Implemented the plumbing for paint worklets #17150

Implemented the plumbing for paint worklets #17150

Conversation

asajeffrey commented Jun 3, 2017 • edited by larsbergstrom

highfive commented Jun 3, 2017

highfive commented Jun 3, 2017

asajeffrey commented Jun 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey commented Jun 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Jun 6, 2017

jdm commented Jun 6, 2017

asajeffrey commented Jun 7, 2017

bors-servo commented Jun 7, 2017

bors-servo commented Jun 7, 2017

bors-servo commented Jun 7, 2017

asajeffrey commented Jun 3, 2017 •

edited by larsbergstrom