Bug 60508 – [chromium] Make throttling of WebGL based on webgl frames, not compositor frames

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nat Duca]

[chromium] Make throttling of WebGL based on webgl frames, not compositor frames

[Nat Duca]

Created attachment 92859 [details]
Patch

[James Robinson]

Comment on attachment 92859 [details]
Patch

WebCore code normally uses a WebCore::Timer() with a delay of 0 to do this sort of callback - will this work in this case?  I believe this will take care of your layer lifetime issues.

For an example see Document::m_styleRecalcTimer

[Nat Duca]

Created attachment 92908 [details]
Use timer.

[WebKit Review Bot]

Comment on attachment 92908 [details]
Use timer.

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

[Nat Duca]

Created attachment 92910 [details]
Make it build.

[James Robinson]

Comment on attachment 92910 [details]
Make it build.

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

Some nits for now.  I think it looks OK but think that Ken should do the final review.

> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:146
> +    // Check if the context is gone.

WebKit style frowns on this sort of comment since it should be obvious what the check is doing just by reading the conditional.

> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:150
> +    // Actually do the rate limiting.

WK style isn't too fond of this either.

> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:152
> +    extensions->rateLimitOffscreenContextCHROMIUM();

What if we lose the context while the task is pending?  Does anything special happen?

[Nat Duca]

This extension should behave similarly to other GL APIs upon context lost: it will fail, but it shouldn't explode. Our Chromium-side implementation of the extension should hold to that standard.

I verified that this is the case with the chromium-side of the implementation.

[Nat Duca]

Created attachment 92912 [details]
remove comments.

[Kenneth Russell]

Comment on attachment 92912 [details]
remove comments.

Looks great. r=me

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 92912 [details]:

http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 92912 [details]
remove comments.

Clearing flags on attachment: 92912

Committed r86278: <http://trac.webkit.org/changeset/86278>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.