Bug 103793 - Associate URLs with GraphicsContext3D instances created for WebGL
Summary: Associate URLs with GraphicsContext3D instances created for WebGL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on: 101826
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-30 17:47 PST by Kenneth Russell
Modified: 2012-12-06 17:26 PST (History)
12 users (show)

See Also:


Attachments
Patch (4.73 KB, patch)
2012-11-30 17:55 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (7.37 KB, patch)
2012-12-03 21:11 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch for landing (7.39 KB, patch)
2012-12-04 18:47 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Revised patch using WebString (5.21 KB, patch)
2012-12-06 14:32 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2012-11-30 17:47:30 PST
In order to improve the quality of crash reports and lost context notifications, a URL needs to be associated with the GraphicsContext3D instances created on behalf of WebGL.

As of the fix for Bug 101826, the FrameLoaderClient gets the opportunity to veto WebGL context creation, but the context creation code itself does not have enough information to determine the URL of the page which is creating the context. This information is most easily passed down from WebCore to the platform.
Comment 1 Kenneth Russell 2012-11-30 17:55:54 PST
Created attachment 177069 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-30 18:01:57 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adam Barth 2012-12-01 16:16:13 PST
Comment on attachment 177069 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177069&action=review

> Source/Platform/chromium/public/WebGraphicsContext3D.h:112
> +        WebString urlOfCreatingDocument;

Why a WebString an not a WebURL?
Comment 4 Kenneth Russell 2012-12-03 21:04:22 PST
(In reply to comment #3)
> (From update of attachment 177069 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177069&action=review
> 
> > Source/Platform/chromium/public/WebGraphicsContext3D.h:112
> > +        WebString urlOfCreatingDocument;
> 
> Why a WebString an not a WebURL?

No good reason. It turns out there was a technical problem using WebURL here, but it wasn't difficult to fix in the gyp files. New patch coming up.
Comment 5 Kenneth Russell 2012-12-03 21:11:32 PST
Created attachment 177411 [details]
Patch
Comment 6 Adam Barth 2012-12-03 23:34:13 PST
Comment on attachment 177411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177411&action=review

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:428
> +    attributes.topDocumentURL = document->frame()->tree()->top()->document()->url();

document->topDocument() will save you a lot of typing.
Comment 7 Kenneth Russell 2012-12-04 18:47:07 PST
Created attachment 177643 [details]
Patch for landing
Comment 8 Kenneth Russell 2012-12-04 18:47:31 PST
Comment on attachment 177411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177411&action=review

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:428
>> +    attributes.topDocumentURL = document->frame()->tree()->top()->document()->url();
> 
> document->topDocument() will save you a lot of typing.

Fixed, thanks.
Comment 9 Kenneth Russell 2012-12-04 18:50:09 PST
Committed r136609: <http://trac.webkit.org/changeset/136609>
Comment 10 Stephen White 2012-12-05 12:29:05 PST
Reverted r136609 for reason:

Breaking Chrome/Mac Debug builds.

Committed r136734: <http://trac.webkit.org/changeset/136734>
Comment 12 Kenneth Russell 2012-12-05 14:30:58 PST
@thakis indicates this bot is using the shared library build.
Comment 13 Kenneth Russell 2012-12-05 17:21:02 PST
I tried, but it is infeasible to pass an instance of WebURL (not a reference) across the WebKit boundary, in particular in the component build. The problem is that WebURL.h has inlined constructors which call out-of-line code in the googleurl library, and there is just no way using GYP to express that dependents of targets in WebKit.gyp need to depend on (i.e., include on their link line) the googleurl library. Neither export_dependent_settings nor direct_dependent_settings do this; note for example that the icuuc library (currently in the export_dependent_settings of WebKit.gyp:webkit) does not show up on the link line of targets that depend on webkit, such as src/webkit/gpu/webkit_gpu.gypi:webkit_gpu.

If I try to use a WebURL here then some (but not all) downstream targets of the WebKit API will have to mysteriously add a googleurl dependency in the component build. I found at least two targets (src/webkit/gpu/webkit_gpu.gypi:webkit_gpu and src/ui/compositor/compositor.gyp:compositor) which need it, but I'm not going to pursue this further as it would just add a time bomb to the code base.

Tomorrow I'll recast this patch to use WebString. abarth suggested adding a FIXME indicating why it doesn't use WebURL; this comment documents the reason.
Comment 14 Kenneth Russell 2012-12-06 14:32:35 PST
Created attachment 178083 [details]
Revised patch using WebString
Comment 15 Kenneth Russell 2012-12-06 16:23:51 PST
abarth doesn't seem to be around today, but yesterday indicated offline that the change to WebString is OK. I can't afford another delay of this patch and it looks good on the EWS so am landing it by hand now.
Comment 16 Kenneth Russell 2012-12-06 16:27:56 PST
Committed r136907: <http://trac.webkit.org/changeset/136907>
Comment 17 Adam Barth 2012-12-06 17:26:47 PST
Comment on attachment 178083 [details]
Revised patch using WebString

Delayed LGTM