Bug 60508

Summary: [chromium] Make throttling of WebGL based on webgl frames, not compositor frames
Product: WebKit Reporter: Nat Duca <nduca>
Component: New BugsAssignee: Nat Duca <nduca>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dglazkov, gman, jamesr, kbr, nduca, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Use timer.
none
Make it build.
none
remove comments. none

Nat Duca
Reported 2011-05-09 14:53:21 PDT
[chromium] Make throttling of WebGL based on webgl frames, not compositor frames
Attachments
Patch (15.15 KB, patch)
2011-05-09 15:02 PDT, Nat Duca 		no flags
DetailsFormatted DiffDiff
Use timer. (14.21 KB, patch)
2011-05-09 19:16 PDT, Nat Duca 		no flags
DetailsFormatted DiffDiff
Make it build. (14.19 KB, patch)
2011-05-09 19:48 PDT, Nat Duca 		no flags
DetailsFormatted DiffDiff
remove comments. (14.12 KB, patch)
2011-05-09 20:16 PDT, Nat Duca 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Nat Duca
Comment 1 2011-05-09 15:02:36 PDT
Created attachment 92859 [details] Patch
James Robinson
Comment 2 2011-05-09 15:11:19 PDT
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
Comment 3 2011-05-09 19:16:08 PDT
Created attachment 92908 [details] Use timer.
WebKit Review Bot
Comment 4 2011-05-09 19:42:17 PDT
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
Comment 5 2011-05-09 19:48:23 PDT
Created attachment 92910 [details] Make it build.
James Robinson
Comment 6 2011-05-09 19:53:51 PDT
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
Comment 7 2011-05-09 20:15:33 PDT
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
Comment 8 2011-05-09 20:16:21 PDT
Created attachment 92912 [details] remove comments.
Kenneth Russell
Comment 9 2011-05-11 13:41:55 PDT
Comment on attachment 92912 [details] remove comments. Looks great. r=me
WebKit Commit Bot
Comment 10 2011-05-11 15:47:38 PDT
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: alice.liu@apple.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 11 2011-05-11 15:51:08 PDT
Comment on attachment 92912 [details] remove comments. Clearing flags on attachment: 92912 Committed r86278: <http://trac.webkit.org/changeset/86278>
WebKit Commit Bot
Comment 12 2011-05-11 15:51:12 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.