Handle toDataURL with no context #8725

dzbarsky · 2015-11-29T00:07:22Z

eefriedman · 2015-11-29T00:26:21Z

components/script/dom/htmlcanvaselement.rs

@@ -279,7 +279,7 @@ impl HTMLCanvasElementMethods for HTMLCanvasElement {
            let encoded = encoded.to_base64(STANDARD);
            Ok(DOMString::from(format!("data:{};base64,{}", mime_type, encoded)))
        } else {
-            Err(Error::NotSupported)
+            Ok(DOMString::from(format!("data:image/png;")))


"When its canvas context mode is none, a canvas element has no rendering context, and its bitmap must be fully transparent black with an intrinsic width equal to the numeric value of the element's width attribute and an intrinsic height equal to the numeric value of the element's height attribute, those values being interpreted in CSS pixels, and being updated as the attributes are set, changed, or removed." (https://html.spec.whatwg.org/multipage/scripting.html#the-canvas-element).

In other words, a canvas always has content, so we must always return a valid image with the specified width and height.

dzbarsky · 2015-11-29T03:08:58Z

Good catch. We're matching Chrome's output now.

eefriedman · 2015-11-29T03:52:34Z

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

components/script/dom/htmlcanvaselement.rs, line 271 [r2] (raw file):
Please make this explicitly None, and add an explicit branch for any unhandled case. I think handling for webgl contexts is missing?

components/script/dom/htmlcanvaselement.rs, line 272 [r2] (raw file):
If I'm not mistaken, this is solid white, and the spec says "fully transparent black". At the very least, there should be a comment explicitly noting what color this is.

Also, a test for this behavior would be nice.

Comments from the review on Reviewable.io

dzbarsky · 2015-11-29T04:06:21Z

Don't have time to write a test right now, but if you mark me delegate+ and I'll land with a test.

eefriedman · 2015-11-29T05:07:41Z

I'd like to review the test to double-check that it make sense.

Also, this doesn't build as-is; looks like a bug in the lint. I'll look into it.


        Fix false positive in unrooted_must_root lint.

Encountered in servo#8725.


        Fix false positive in unrooted_must_root lint.

Encountered in servo#8725.


        Auto merge of #8731 - eefriedman:root-lint-pattern, r=Manishearth

Fix false positive in unrooted_must_root lint. Encountered in #8725.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8731)


        Fix false positive in unrooted_must_root lint.

Encountered in servo#8725.

bors-servo · 2015-12-18T00:25:24Z

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

dzbarsky · 2015-12-23T22:38:21Z

Fixed the test @eefriedman

eefriedman · 2015-12-24T19:02:25Z

components/script/dom/htmlcanvaselement.rs

+        // Step 2.
+        if self.Width() == 0 || self.Height() == 0 {
+            return Ok(DOMString::from("data:,"));
+        }


Does step 2 need to come before step 3?

Otherwise looks fine.

@bors-servo delegate+

Not sure if GetImageData can actually throw here, but I rearranged the steps just to be safe.

dzbarsky · 2015-12-24T19:46:29Z

@bors-servo r+

bors-servo · 2015-12-24T19:46:29Z

🔑 Insufficient privileges


        Handle toDataURL with no context

michaelwu · 2015-12-24T20:02:27Z

@bors-servo r=eefriedman

bors-servo · 2015-12-24T20:02:28Z

📌 Commit 84ec9c4 has been approved by eefriedman

bors-servo · 2015-12-24T20:34:40Z

⌛ Testing commit 84ec9c4 with merge 7db6ce4...


        Auto merge of #8725 - dzbarsky:no_context, r=eefriedman

Handle toDataURL with no context  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8725)

bors-servo · 2015-12-24T21:06:43Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Nov 29, 2015

eefriedman reviewed Nov 29, 2015
View changes

eefriedman self-assigned this Nov 29, 2015

dzbarsky force-pushed the dzbarsky:no_context branch from ab3d527 to 73956ac Nov 29, 2015

dzbarsky force-pushed the dzbarsky:no_context branch from 73956ac to a6e875b Nov 29, 2015

eefriedman added a commit to eefriedman/servo that referenced this pull request Nov 29, 2015

Fix false positive in unrooted_must_root lint.

d2e50d0

Encountered in servo#8725.

eefriedman mentioned this pull request Nov 29, 2015

Fix false positive in unrooted_must_root lint. #8731

Merged

eefriedman added a commit to eefriedman/servo that referenced this pull request Nov 30, 2015

Fix false positive in unrooted_must_root lint.

962c06e

Encountered in servo#8725.

asajeffrey added a commit to asajeffrey/servo that referenced this pull request Dec 2, 2015

Fix false positive in unrooted_must_root lint.

e25ef7c

Encountered in servo#8725.

highfive added the S-needs-rebase label Dec 18, 2015

dzbarsky force-pushed the dzbarsky:no_context branch from a6e875b to f5386a6 Dec 23, 2015

eefriedman reviewed Dec 24, 2015
View changes

dzbarsky force-pushed the dzbarsky:no_context branch 2 times, most recently from 50d0f25 to 84ec9c4 Dec 24, 2015

Handle toDataURL with no context

Loading status checks…

84ec9c4

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Dec 24, 2015

servo / servo

Handle toDataURL with no context #8725

Handle toDataURL with no context #8725

dzbarsky commented Nov 29, 2015

This comment has been minimized.

dzbarsky commented Nov 29, 2015

eefriedman commented Nov 29, 2015

dzbarsky commented Nov 29, 2015

eefriedman commented Nov 29, 2015

bors-servo commented Dec 18, 2015

dzbarsky commented Dec 23, 2015

This comment has been minimized.

This comment has been minimized.

dzbarsky commented Dec 24, 2015

bors-servo commented Dec 24, 2015

michaelwu commented Dec 24, 2015

bors-servo commented Dec 24, 2015

bors-servo commented Dec 24, 2015

bors-servo commented Dec 24, 2015

servo / servo

Join GitHub today

Handle toDataURL with no context #8725

Handle toDataURL with no context #8725

Conversation

dzbarsky commented Nov 29, 2015

This comment has been minimized.

eefriedman Nov 29, 2015

dzbarsky commented Nov 29, 2015

eefriedman commented Nov 29, 2015

dzbarsky commented Nov 29, 2015

eefriedman commented Nov 29, 2015

bors-servo commented Dec 18, 2015

dzbarsky commented Dec 23, 2015

This comment has been minimized.

eefriedman Dec 24, 2015

This comment has been minimized.

dzbarsky Dec 24, 2015

dzbarsky commented Dec 24, 2015

bors-servo commented Dec 24, 2015

michaelwu commented Dec 24, 2015

bors-servo commented Dec 24, 2015

bors-servo commented Dec 24, 2015

bors-servo commented Dec 24, 2015