Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement drawing an image from a CSS style value into a canvas #17634

Merged
merged 1 commit into from Jul 21, 2017

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Jul 7, 2017

Implemented drawing a CSS style value into a canvas, which is needed for the Houdini CSS Paint API.

This PR is dependent on #17364.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #17432.
  • These changes do not require tests because the existing CSS paint API wpt test cases test this behaviour.

This change is Reviewable

@highfive
Copy link

highfive commented Jul 7, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/servo/restyle_damage.rs
  • @canaltinova: components/style/servo/restyle_damage.rs
  • @KiChjang: components/script/dom/webidls/StylePropertyMapReadOnly.webidl, components/script/dom/testworkletglobalscope.rs, components/script/dom/mod.rs, components/script/dom/stylepropertymapreadonly.rs, components/script/dom/window.rs and 15 more
  • @fitzgen: components/script/dom/webidls/StylePropertyMapReadOnly.webidl, components/script/dom/testworkletglobalscope.rs, components/script/dom/mod.rs, components/script/dom/stylepropertymapreadonly.rs, components/script/dom/window.rs and 15 more
  • @emilio: components/layout/display_list_builder.rs, components/style/servo/restyle_damage.rs, components/layout/context.rs, components/layout/Cargo.toml, components/layout/lib.rs
@highfive
Copy link

highfive commented Jul 7, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
Copy link
Member

emilio left a comment

Looks reasonable to me... Probably @jdm should take a look for the DOM bits.

// Paint worklets may depend on custom properties,
// so if they have changed we should repaint.
if old.get_custom_properties() != new.get_custom_properties() {
damage.insert(REPAINT);

This comment has been minimized.

Copy link
@emilio

emilio Jul 11, 2017

Member

I think this could be too overkill... Could we do better? I guess this is fine for now.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Jul 11, 2017

Author Member

Not sure how much better we could do, rather annoyingly. We could try to be smart about which properties are being monitored by a paint worklet.

@@ -388,9 +373,9 @@ impl WorkletThreadRole {
#[derive(Clone)]
struct WorkletThreadInit {
/// Senders
primary_sender: Sender<WorkletData>,

This comment has been minimized.

Copy link
@emilio

emilio Jul 11, 2017

Member

All these members would benefit from some documentation.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Jul 11, 2017

Author Member

Not sure what would be added here, other than "the sender to the primary worklet thread" :)

@bors-servo
Copy link
Contributor

bors-servo commented Jul 12, 2017

The latest upstream changes (presumably #17364) made this pull request unmergeable. Please resolve the merge conflicts.

@asajeffrey asajeffrey force-pushed the asajeffrey:canvas-image-css-style-value branch from 17be732 to 7c800a9 Jul 17, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 17, 2017

The dependent PRs for this one have landed, so this one is ready for review. @emilio suggested I r? @jdm

@highfive highfive assigned jdm and unassigned emilio Jul 17, 2017
asajeffrey added a commit to asajeffrey/servo that referenced this pull request Jul 18, 2017
@asajeffrey asajeffrey mentioned this pull request Jul 18, 2017
4 of 4 tasks complete
asajeffrey added a commit to asajeffrey/servo that referenced this pull request Jul 19, 2017
asajeffrey added a commit to asajeffrey/servo that referenced this pull request Jul 20, 2017
@asajeffrey asajeffrey mentioned this pull request Jul 20, 2017
4 of 4 tasks complete
asajeffrey added a commit to asajeffrey/servo that referenced this pull request Jul 20, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

The latest upstream changes (presumably #17499) made this pull request unmergeable. Please resolve the merge conflicts.

@asajeffrey asajeffrey force-pushed the asajeffrey:canvas-image-css-style-value branch from 7c800a9 to bf8bcd9 Jul 21, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 21, 2017

If #17625 is addressed before this lands, e.g. by @brainlessdeveloper in #17812, we can remove base_url from https://github.com/asajeffrey/servo/blob/bf8bcd9a401586d6f0d2e258831e6181d1aaa174/components/script/dom/canvasrenderingcontext2d.rs#L77. URLs should be resolved by style relative to the stylesheet, not by script.

@jdm
jdm approved these changes Jul 21, 2017
// Any missing image URLs.
missing_image_urls: DOMRefCell<Vec<ServoUrl>>,
// The base URL for resolving CSS image URL values.
// Needed because of https://github.com/servo/servo/issues/17625

This comment has been minimized.

Copy link
@jdm

jdm Jul 21, 2017

Member

Use /// instead //.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Jul 21, 2017

Author Member

Done.

@asajeffrey asajeffrey force-pushed the asajeffrey:canvas-image-css-style-value branch from bf8bcd9 to 2318caf Jul 21, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 21, 2017

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

📌 Commit 2318caf has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

Testing commit 2318caf with merge 76ad913...

bors-servo added a commit that referenced this pull request Jul 21, 2017
Implement drawing an image from a CSS style value into a canvas

<!-- Please describe your changes on the following line: -->

Implemented drawing a CSS style value into a canvas, which is needed for the Houdini CSS Paint API.

This PR is dependent on #17364.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #17432.
- [X] These changes do not require tests because the existing CSS paint API wpt test cases test this behaviour.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17634)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

@bors-servo bors-servo merged commit 2318caf into servo:master Jul 21, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.