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 HTMLCanvasElement.probablySupportsContext #11461

Closed

Conversation

@mskrzypkows
Copy link

mskrzypkows commented May 27, 2016


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #11403
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented May 27, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/htmlcanvaselement.rs, components/script/dom/webidls/HTMLCanvasElement.webidl
@nox
Copy link
Member

nox commented May 27, 2016

r? @emilio

I think this change is incorrect, because GetContext will cache the context, so if you try to check whether you can create a 2d context, and then try to get for real a canvas context, you won't get it.

@highfive highfive assigned emilio and unassigned Ms2ger May 27, 2016
cx: *mut JSContext,
id: DOMString,
attributes: Vec<HandleValue>) -> bool {
if self.GetContext(cx, id, attributes).is_some() {

This comment has been minimized.

Copy link
@dzbarsky

dzbarsky May 28, 2016

Member

just return self.GetContext(cx, id, attributes).is_some() (and you don't actualy need the return keyword)

@emilio
Copy link
Member

emilio commented May 28, 2016

@nox is right here, we can't do this that way.

In fact, there's no real support for this method in any other browser (and it's arguable), see this for example.

In any case, if we plan to implement this, we should use an heuristic, and not calling GetContext() (since the whole point of this API is avoiding the GetContext() overhead).

In my opinion, we should return true if the id is one of those we support (i.e. webgl, experimental-webgl or 2d), and maybe we want to make it return false if getting the WebGLContextAttributes we see that antialias is true (it's something we don't support right now), but that's an extra thing to change when we support it, so... I'd just check the id.

That being said, I'm a bit skeptical about the utility of this method.

-S-awaiting-review +S-needs-code-changes

Previously, nox (Anthony Ramine) wrote…

r? @emilio

I think this change is incorrect, because GetContext will cache the context, so if you try to check whether you can create a 2d context, and then try to get for real a canvas context, you won't get it.


Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@Ms2ger
Copy link
Contributor

Ms2ger commented May 29, 2016

We should probably figure out why no other browser implemented this; it's not particularly likely to be implementation difficulty.

@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2016

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

@mskrzypkows
Copy link
Author

mskrzypkows commented May 31, 2016

Here is whatwg thread: https://lists.w3.org/Archives/Public/public-whatwg-archive/2013Jun/0098.html and last message: https://lists.w3.org/Archives/Public/public-whatwg-archive/2013Sep/0248.html
As I understand from the discussion , not all people agree that this API is really useful. Should I continue the task?

@mskrzypkows
Copy link
Author

mskrzypkows commented Jun 7, 2016

Is there any decision about this task?

@emilio
Copy link
Member

emilio commented Jun 23, 2016

I just filled whatwg/html#1459

@emilio
Copy link
Member

emilio commented Jun 24, 2016

The spec has been modified in order to no longer include this method.

Thanks for the contribution though @mskrzypkows, it's been crucial to raise the issue! :-)

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.

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