Bug 107218 - Move WebGLErrorsToConsole page setting to window.internals.settings
Summary: Move WebGLErrorsToConsole page setting to window.internals.settings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: arno.
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-17 19:45 PST by Kenneth Russell
Modified: 2013-01-25 11:21 PST (History)
12 users (show)

See Also:


Attachments
patch proposal (4.37 KB, patch)
2013-01-18 18:09 PST, arno.
no flags Details | Formatted Diff | Diff
updated patch (13.99 KB, patch)
2013-01-21 14:55 PST, arno.
buildbot: commit-queue-
Details | Formatted Diff | Diff
updated patch (15.67 KB, patch)
2013-01-23 11:08 PST, arno.
no flags Details | Formatted Diff | Diff
updated patch (39.99 KB, patch)
2013-01-24 11:16 PST, arno.
no flags Details | Formatted Diff | Diff
updated patch (remove spurious change) (39.47 KB, patch)
2013-01-24 18:12 PST, arno.
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 2013-01-17 19:45:28 PST
Per Bug 107035 the page setting WebGLErrorsToConsole should be moved to window.internals.settings. It shouldn't be hard to make all the WebGL tests set this to false, preventing spew by the test harness, while making it default to true (in browsers).
Comment 1 Tony Chang 2013-01-18 10:18:16 PST
Since you used Settings.in to define the property, you get this for free!

We automatically generate setters on window.internals.settings for bool preferences defined in Settings.in. window.internals.settings.setWebGLEnabled should already exist.
Comment 2 arno. 2013-01-18 18:09:57 PST
Created attachment 183587 [details]
patch proposal
Comment 3 Build Bot 2013-01-19 09:41:53 PST
Comment on attachment 183587 [details]
patch proposal

Attachment 183587 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15966212

New failing tests:
inspector/profiler/webgl/webgl-profiler-api-changes.html
inspector/profiler/webgl/webgl-profiler-get-error.html
Comment 4 arno. 2013-01-21 14:55:20 PST
Created attachment 183833 [details]
updated patch
Comment 5 Build Bot 2013-01-21 21:45:02 PST
Comment on attachment 183833 [details]
updated patch

Attachment 183833 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16037385

New failing tests:
compositing/z-order/negative-z-index.html
http/tests/security/window-events-clear-domain.html
Comment 6 WebKit Review Bot 2013-01-22 15:54:50 PST
Comment on attachment 183833 [details]
updated patch

Attachment 183833 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15991037

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
http/tests/security/window-events-clear-domain.html
http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html
Comment 7 Kenneth Russell 2013-01-22 16:24:45 PST
Comment on attachment 183833 [details]
updated patch

Arno, have you gone through the layout test failures reported by the EWS? A couple look like they could be related to this patch.
Comment 8 arno. 2013-01-23 11:08:03 PST
Created attachment 184266 [details]
updated patch

I think this patch covers all tests.
But the problem is: forceLostContext(SyntheticLostContext) is called every time a document is unloaded. So when working with webgl, the console receives a lot of unuseful WebGL: CONTEXT_LOST_WEBGL: loseContext: context lost messages.
What about sending lost context messages to console only if mode is RealLostContext ?
Comment 9 Kenneth Russell 2013-01-23 12:52:40 PST
(In reply to comment #8)
> What about sending lost context messages to console only if mode is RealLostContext ?

No objections, but I think you will find this difficult to implement. The CONTEXT_LOST_WEBGL error needs to be synthesized both for real and synthetic lost contexts, and by the time it's queried, the fact that the original lost context was synthetic is lost.
Comment 10 arno. 2013-01-24 11:16:24 PST
Created attachment 184537 [details]
updated patch

updated patch: do not display context lost errors on console if it is a synthetic lost. Also, instead of setting setWebGLErrorsToConsoleEnabled to false for all webgl tests, only do that for tests which trigger some errors. So, it will be easier to check if a test starts triggering an unexpected error
Comment 11 Kenneth Russell 2013-01-24 18:07:21 PST
Comment on attachment 184537 [details]
updated patch

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

Thanks for persisting. This patch looks good. Did you want it reviewed and cq'd? Please mark r? and cq? in Details of patch if so.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1885
> +            && (!state.bufferBinding || !state.bufferBinding->object())) {

Spurious change?
Comment 12 arno. 2013-01-24 18:12:16 PST
Created attachment 184626 [details]
updated patch (remove spurious change)
Comment 13 Kenneth Russell 2013-01-24 18:40:14 PST
Comment on attachment 184626 [details]
updated patch (remove spurious change)

Looks good. r=me
Comment 14 WebKit Review Bot 2013-01-25 11:21:39 PST
Comment on attachment 184626 [details]
updated patch (remove spurious change)

Clearing flags on attachment: 184626

Committed r140851: <http://trac.webkit.org/changeset/140851>
Comment 15 WebKit Review Bot 2013-01-25 11:21:43 PST
All reviewed patches have been landed.  Closing bug.