Expose WEBGL_debug_renderer_info through a chromium finch experiment.

Expose WEBGL_debug_renderer_info through a chromium finch experiment.

This is the blink side part: use a FrameLoaderClient query to decide
whether to expose this extension or not.  The decidion making will be
in the chromium side.

BUG=309831
TEST=no behavioral change until chromium side lands

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161008

[Zhenyao Mo, 2013-10-29 22:22:55]

Ken, James, could you please review this?

Due to the fact that we want to merge this back into the currently released
stable, I want to land this as soon as possible, because we can only test the
whole mechanism (with chromium side CL) properly once this is rolled into
chromium. Thanks a lot.

[jamesr, 2013-10-29 22:38:58]

Hm, does this mean we'll expose a new WebGL extension object in the registry,
etc?  Why not just change the return value of RENDERER / VENDOR?

[Ken Russell (switch to Gerrit), 2013-10-29 22:45:35]

On 2013/10/29 22:38:58, jamesr wrote:
> Hm, does this mean we'll expose a new WebGL extension object in the registry,
> etc?

Yes.

>  Why not just change the return value of RENDERER / VENDOR?

The WebGL spec defines these strings and the conformance suite validates them.
They still provide useful identifying information (which version of WebGL's
being used, for example) and it's not under consideration at the moment to
change their meaning.

[Zhenyao Mo, 2013-10-29 22:45:51]

On 2013/10/29 22:38:58, jamesr wrote:
> Hm, does this mean we'll expose a new WebGL extension object in the registry,
> etc?  Why not just change the return value of RENDERER / VENDOR?

That's against the WebGL spec and would require a spec change (otherwise
conformance fails)

[Zhenyao Mo, 2013-10-29 22:46:48]

This extension is already there, only before we only expose it to privileged
code in the browser, say, chrome apps.  Now we experiment with exposing it to
all.

[Zhenyao Mo, 2013-10-29 22:47:00]

chromium side CL: https://codereview.chromium.org/48113021/

[Ken Russell (switch to Gerrit), 2013-10-29 23:00:17]

LGTM overall. Minor comments.

https://codereview.chromium.org/51323003/diff/40001/Source/core/html/canvas/W...
File Source/core/html/canvas/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/51323003/diff/40001/Source/core/html/canvas/W...
Source/core/html/canvas/WebGLRenderingContext.cpp:627:
registerExtension<WebGLDebugRendererInfo>(m_webglDebugRendererInfo,
WebGLDebugRendererInfoExtension);
Would it work to use the argument (PrivilegedExtension |
WebGLDebugRendererInfoExtension) here? That might avoid the special-case test of
allowPrivilegedExtensions() in
WebGLRenderingContext::allowWebGLDebugRendererInfo below.

https://codereview.chromium.org/51323003/diff/40001/Source/core/loader/FrameL...
File Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/51323003/diff/40001/Source/core/loader/FrameL...
Source/core/loader/FrameLoaderClient.h:221: // Returns true if WebGL extension
WEBGL_debug_renderer_info is allowed.
Could you please add a newline above this line?

[Zhenyao Mo, 2013-10-29 23:05:52]

https://codereview.chromium.org/51323003/diff/40001/Source/core/html/canvas/W...
File Source/core/html/canvas/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/51323003/diff/40001/Source/core/html/canvas/W...
Source/core/html/canvas/WebGLRenderingContext.cpp:627:
registerExtension<WebGLDebugRendererInfo>(m_webglDebugRendererInfo,
WebGLDebugRendererInfoExtension);
On 2013/10/29 23:00:17, Ken Russell wrote:
> Would it work to use the argument (PrivilegedExtension |
> WebGLDebugRendererInfoExtension) here? That might avoid the special-case test
of
> allowPrivilegedExtensions() in
> WebGLRenderingContext::allowWebGLDebugRendererInfo below.

That's what I did in the first try, but it will complicate the logic, because it
will fail the test of allowPrivilegedExtensions() in general (unless it's
privileged code) and not be exposed.

https://codereview.chromium.org/51323003/diff/40001/Source/core/loader/FrameL...
File Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/51323003/diff/40001/Source/core/loader/FrameL...
Source/core/loader/FrameLoaderClient.h:221: // Returns true if WebGL extension
WEBGL_debug_renderer_info is allowed.
On 2013/10/29 23:00:17, Ken Russell wrote:
> Could you please add a newline above this line?

Done.

[jamesr, 2013-10-29 23:09:32]

I'm not familiar enough with WebFrameClient to know if this is the right
plumbing.  Maybe Adam or Darin can help you.

[Zhenyao Mo, 2013-10-29 23:36:06]

Darin or Adam: could you take a look?

Just FYI: this follows a preexisting example of allowWebGL() in WebFrameClient.

[abarth-chromium, 2013-10-30 06:36:16]

https://codereview.chromium.org/51323003/diff/130001/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/51323003/diff/130001/public/web/WebFrameClien...
public/web/WebFrameClient.h:375: virtual bool
allowWebGLDebugRendererInfo(WebFrame*) { return false; }
I don't think this is the right place.  Let me dig a bit.

[abarth-chromium, 2013-10-30 06:39:23]

I believe the right place is WebPermissionClient.  Take a look at how
allowMutationEvents is wired up.

[Alexei Svitkine (slow), 2013-10-30 18:07:47]

You still need a FieldTrialList::FindFullName() call from somewhere to detect
the trial group. But since this is a Blink CL, I guess it wouldn't be here
(unless there's a way to do it from Blink?).

[Alexei Svitkine (slow), 2013-10-30 18:08:20]

On 2013/10/30 18:07:47, Alexei Svitkine wrote:
> You still need a FieldTrialList::FindFullName() call from somewhere to detect
> the trial group. But since this is a Blink CL, I guess it wouldn't be here
> (unless there's a way to do it from Blink?).

Woops, commented on the wrong CL.

[Zhenyao Mo, 2013-10-30 20:03:46]

Adam: revised.  please take another look.

[Ken Russell (switch to Gerrit), 2013-10-30 20:07:08]

LGTM if the new structure works.

[Zhenyao Mo, 2013-10-30 20:07:52]

On 2013/10/30 20:07:08, Ken Russell wrote:
> LGTM if the new structure works.

It works.  I will upload the chromium side CL shortly after, but it's been well
tested.

[abarth-chromium, 2013-10-30 21:41:56]

Why not route this through ContextFeatures like all the rest of the
context-enabled features?  We can land this if you're in a big rush, but it
would be better to route this call the same way all the rest of the
ContextFeatures work,.

https://codereview.chromium.org/51323003/diff/250001/public/web/WebPermission...
File public/web/WebPermissionClient.h (right):

https://codereview.chromium.org/51323003/diff/250001/public/web/WebPermission...
public/web/WebPermissionClient.h:105: virtual bool
allowWebGLDebugRendererInfo(WebFrame*) { return false; }
This should take a const WebDocument& like the other per-context features.

[Zhenyao Mo, 2013-10-30 21:57:10]

Looking ContextFeatures mechanisms, let's avoid going down that path for now. 
This is very temporary (only for during the finch experiment period).  After the
experiment (in a month or so), we will either always enable this, or always
disable this, so all the code here will be removed.

I have one question about WebFrame vs WebDocument.  If there is a good reason to
use WebDocument instead WebFrame, I'll make another revision.  Otherwise I'd
like to get a lgtm from you and land it.

https://codereview.chromium.org/51323003/diff/250001/public/web/WebPermission...
File public/web/WebPermissionClient.h (right):

https://codereview.chromium.org/51323003/diff/250001/public/web/WebPermission...
public/web/WebPermissionClient.h:105: virtual bool
allowWebGLDebugRendererInfo(WebFrame*) { return false; }
On 2013/10/30 21:41:56, abarth wrote:
> This should take a const WebDocument& like the other per-context features.

I am confused.  There are multiple examples using WebFrame (allowStorage,
allowWriteToClipboard) and multiple examples using WebDocument, what's the
difference?

[abarth-chromium, 2013-10-30 22:00:09]

On 2013/10/30 21:57:10, Zhenyao Mo wrote:
> I am confused.  There are multiple examples using WebFrame (allowStorage,
> allowWriteToClipboard) and multiple examples using WebDocument, what's the
> difference?

Some objects use Frame as their context object and some use Document.  In this
case, the context for this feature is an element, which uses a Document as it's
context object.  This only really matters when Document detaches from Frame.  In
this case, the GL canvas gets torn down at that point anyway, so you can't
really tell the difference.

This CL is a bit of a mess, but LGTM I guess.  Please rip this code out soon....

[Zhenyao Mo, 2013-10-30 22:03:17]

On 2013/10/30 22:00:09, abarth wrote:
> On 2013/10/30 21:57:10, Zhenyao Mo wrote:
> > I am confused.  There are multiple examples using WebFrame (allowStorage,
> > allowWriteToClipboard) and multiple examples using WebDocument, what's the
> > difference?
> 
> Some objects use Frame as their context object and some use Document.  In this
> case, the context for this feature is an element, which uses a Document as
it's
> context object.  This only really matters when Document detaches from Frame. 
In
> this case, the GL canvas gets torn down at that point anyway, so you can't
> really tell the difference.
> 
> This CL is a bit of a mess, but LGTM I guess.  Please rip this code out
soon....

Thanks.  Don't worry, this will be removed soon.

[commit-bot: I haz the power, 2013-10-30 22:05:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zmo@chromium.org/51323003/250001

[commit-bot: I haz the power, 2013-10-30 23:10:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-30 23:20:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zmo@chromium.org/51323003/470001

[commit-bot: I haz the power, 2013-10-31 02:27:49]

Change committed as 161008