Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
490393002:
Simplify IOSurface CoreAnimation code: Part 2 (Closed)
|
Created:
6 years, 4 months ago by ccameron Modified:
6 years, 3 months ago Reviewers:
Ken Russell (switch to Gerrit) CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionSimplify IOSurface CoreAnimation code: Part 2
It isn't necessary to mess around with sharing CGLContextObjs anymore,
they were necessary only to share data between the frame subscriber
and the RenderWidgetHostViewMac. Just give each IOSurfaceLayer its
own CGLContextObj, and draw using immediate mode (using shaders
resulted in an increase in startup time).
BUG=314190
TBR=pfeldman@chromium.org
R=kbr@chromium.org
Committed: https://crrev.com/672e23e9a1f1e11cb5f62c46ab8f687e3ca9d3da
Cr-Commit-Position: refs/heads/master@{#293103}
Patch Set 1 #Patch Set 2 : Lower similarity #
Total comments: 2
Patch Set 3 : Incorporate review feedback #Patch Set 4 : Missed colon #
Total comments: 1
Patch Set 5 : Rebase #Patch Set 6 : Second half #Patch Set 7 : Report acutal pixel differences #Patch Set 8 : Test where the bug is coming in #Patch Set 9 : Add more time #Patch Set 10 : Smaller scope #Patch Set 11 : Try with pixel format bit #Patch Set 12 : Pixel format bit #Patch Set 13 : Final version #Patch Set 14 : Without the extra bit #
Total comments: 1
Messages
Total messages: 46 (2 generated)
ping
Excellent cleanup and simplification. LGTM https://codereview.chromium.org/490393002/diff/20001/content/browser/composit... File content/browser/compositor/io_surface_layer_mac.mm (right): https://codereview.chromium.org/490393002/diff/20001/content/browser/composit... content/browser/compositor/io_surface_layer_mac.mm:256: // Open the IOSurface into this texture, if the underlying IOSurface has been "Associate the IOSurface with this texture, ...". The terminology "Open the IOSurface" is used above to refer to the IOSurfaceLookup call.
Thanks! https://codereview.chromium.org/490393002/diff/20001/content/browser/composit... File content/browser/compositor/io_surface_layer_mac.mm (right): https://codereview.chromium.org/490393002/diff/20001/content/browser/composit... content/browser/compositor/io_surface_layer_mac.mm:256: // Open the IOSurface into this texture, if the underlying IOSurface has been On 2014/08/26 21:14:38, Ken Russell wrote: > "Associate the IOSurface with this texture, ...". The terminology "Open the > IOSurface" is used above to refer to the IOSurfaceLookup call. Done.
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/490393002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_gpu_retina_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2014/08/27 00:24:32, I haz the power (commit-bot) wrote: > FYI, CQ is re-trying this CL (attempt #1). > The failing builders are: > mac_gpu_retina_triggered_tests on tryserver.chromium.gpu > (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...) > mac_chromium_rel_swarming on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) The mac_gpu_retina_triggered_tests failures contain: ** Unknown exception behavior: 0 I think this indicates the browser crashed. Chris, can you reproduce this failure locally? Let me know if you have trouble figuring out how to run the tests.
https://codereview.chromium.org/490393002/diff/60001/content/browser/composit... File content/browser/compositor/io_surface_layer_mac.mm (right): https://codereview.chromium.org/490393002/diff/60001/content/browser/composit... content/browser/compositor/io_surface_layer_mac.mm:88: if ([self respondsToSelector:(@selector(contentsScale))]) The case of the extraneous colon.
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/490393002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/490393002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/490393002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/490393002/80001
On 2014/08/27 19:37:13, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/ccameron@chromium.org/490393002/80001 The failure in CaptureScreenShot looks like the sort of thing that could fail with this patch (same area of code), but doesn't repro locally (gpu/no-gpu, lodpi/hidpi)
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/490393002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2014/08/29 09:00:12, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_chromium_rel_swarming on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) This is getting ridiculous -- I cut the CL in half, and just tried it at https://codereview.chromium.org/521503002/, and it had no issues. I'll keep salami-slicing this patch to see when I get a failure.
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/490393002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/490393002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as a83898264f64ad83bb98139e5e0dcb2c77db1997
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/528403002/ by aiolos@chromium.org. The reason for reverting is: crbug.com/410360 (Browser crashing on IOSurfaceLayer release in tab_switching.typical_25).
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/672e23e9a1f1e11cb5f62c46ab8f687e3ca9d3da Cr-Commit-Position: refs/heads/master@{#293103}
Message was sent while issue was closed.
Adding a comment for something that needs to be fixed when reverting this. https://codereview.chromium.org/490393002/diff/260001/content/browser/composi... File content/browser/compositor/io_surface_layer_mac.mm (left): https://codereview.chromium.org/490393002/diff/260001/content/browser/composi... content/browser/compositor/io_surface_layer_mac.mm:149: SetNeedsDisplayAndDisplayAndAck(); *** this is wrong -- it needs to be DisplayIfNeededAndAck. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
