Bug 84822

Summary: [chromium] REGRESSION(112286) Compositor initialization blocks for program compilation / linking
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, enne, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

James Robinson
Reported 2012-04-24 20:09:09 PDT
[chromium] REGRESSION(112286) Compositor initialization blocks for program compilation / linking
Attachments
Patch (8.48 KB, patch)
2012-04-24 20:25 PDT, James Robinson 		no flags
DetailsFormatted DiffDiff
Patch (9.29 KB, patch)
2012-04-24 23:48 PDT, James Robinson 		no flags
DetailsFormatted DiffDiff
Patch for landing (10.21 KB, patch)
2012-04-25 21:11 PDT, James Robinson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
James Robinson
Comment 1 2012-04-24 20:25:49 PDT
Created attachment 138727 [details] Patch
James Robinson
Comment 2 2012-04-24 23:48:46 PDT
Created attachment 138744 [details] Patch
Dana Jansens
Comment 3 2012-04-25 09:34:04 PDT
Comment on attachment 138744 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138744&action=review > Source/WebCore/platform/graphics/chromium/TextureCopier.cpp:82 > + if (m_blitProgram->initialized()) is that supposed to be if (!initialized) ?
James Robinson
Comment 4 2012-04-25 10:52:18 PDT
Oh, yes (interesting that no layout tests break)
Adrienne Walker
Comment 5 2012-04-25 18:03:52 PDT
(In reply to comment #4) > Oh, yes (interesting that no layout tests break) It'd just re-get the uniforms on ever ycopy, so it's inefficient but still correct. Can I trouble you to add an assert to ProgramBinding::initialize() to make sure we don't do this?
Adrienne Walker
Comment 6 2012-04-25 18:05:31 PDT
Comment on attachment 138744 [details] Patch Other than than the initialized() check, looks great. Thanks for writing a test. :)
James Robinson
Comment 7 2012-04-25 21:10:24 PDT
(In reply to comment #5) > (In reply to comment #4) > > Oh, yes (interesting that no layout tests break) > > It'd just re-get the uniforms on ever ycopy, so it's inefficient but still correct. Can I trouble you to add an assert to ProgramBinding::initialize() to make sure we don't do this? No, since i'm not calling initialize() unless initialized() is true, I think it will never get the uniforms at all. The only uniform on the program is the s_texture on the fragment shader. So I have no idea WTF it was doing. Anyway, added ASSERT()s for both ways - calling initialize() too much or calling program() before initialize() and it seems good now.
James Robinson
Comment 8 2012-04-25 21:11:40 PDT
Created attachment 138928 [details] Patch for landing
WebKit Review Bot
Comment 9 2012-04-25 21:49:42 PDT
Comment on attachment 138928 [details] Patch for landing Clearing flags on attachment: 138928 Committed r115285: <http://trac.webkit.org/changeset/115285>
WebKit Review Bot
Comment 10 2012-04-25 21:49:47 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.