Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(447)

Issue 11053003: Migrate Graphics2D to new design. (Closed)

Created:
8 years, 2 months ago by victorhsieh
Modified:
8 years ago
Reviewers:
brettw, Cris Neckar
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Migrate Graphics2D to new design, as part of the whole Pepper resource redesign. New design provides higher performance and involves writing much less code. See the pepper implementation doc for detail. http://www.chromium.org/developers/design-documents/pepper-plugin-implementation TODO: Inline impl to PepperGraphics2DHost BUG=

Patch Set 1 #

Patch Set 2 : #

Total comments: 8

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : test passed (removed some cherrypicks from upstream) #

Total comments: 2

Patch Set 6 : fix DEPS #

Total comments: 26

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Total comments: 2

Patch Set 10 : #

Total comments: 2

Patch Set 11 : #

Patch Set 12 : rebase upstream #

Total comments: 10

Patch Set 13 : #

Patch Set 14 : make content_browsertests and test_shell_tests build #

Patch Set 15 : #

Patch Set 16 : temp dependencies #

Total comments: 1

Patch Set 17 : no ClearAndRun #

Patch Set 18 : warning fix #

Patch Set 19 : webkit export #

Patch Set 20 : fix + rebase #

Patch Set 21 : rebase #

Patch Set 22 : #

Total comments: 2

Patch Set 23 : #

Total comments: 1

Patch Set 24 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+839 lines, -529 lines) Patch
M content/content_renderer.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +2 lines, -0 lines 0 comments Download
M content/public/renderer/renderer_ppapi_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 4 chunks +9 lines, -1 line 0 comments Download
M content/renderer/pepper/content_renderer_pepper_host_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +12 lines, -0 lines 0 comments Download
M content/renderer/pepper/mock_renderer_ppapi_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +3 lines, -0 lines 0 comments Download
M content/renderer/pepper/mock_renderer_ppapi_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +11 lines, -0 lines 0 comments Download
A content/renderer/pepper/pepper_graphics_2d_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +99 lines, -0 lines 0 comments Download
A content/renderer/pepper/pepper_graphics_2d_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +213 lines, -0 lines 0 comments Download
M content/renderer/pepper/pepper_in_process_resource_creation.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/pepper/pepper_in_process_resource_creation.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +10 lines, -0 lines 0 comments Download
M content/renderer/pepper/pepper_plugin_delegate_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/pepper/pepper_plugin_delegate_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +9 lines, -0 lines 0 comments Download
M content/renderer/pepper/renderer_ppapi_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 4 chunks +7 lines, -0 lines 0 comments Download
M content/renderer/pepper/renderer_ppapi_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 5 chunks +21 lines, -0 lines 0 comments Download
M ppapi/cpp/graphics_2d.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 3 chunks +11 lines, -11 lines 0 comments Download
M ppapi/host/ppapi_host.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -3 lines 0 comments Download
M ppapi/host/ppapi_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +2 lines, -2 lines 0 comments Download
M ppapi/host/resource_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +2 lines, -0 lines 0 comments Download
M ppapi/ppapi_proxy.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +2 lines, -2 lines 0 comments Download
A ppapi/proxy/graphics_2d_resource.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +63 lines, -0 lines 0 comments Download
A ppapi/proxy/graphics_2d_resource.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +150 lines, -0 lines 0 comments Download
M ppapi/proxy/interface_list.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +0 lines, -1 line 0 comments Download
M ppapi/proxy/ppapi_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 4 chunks +30 lines, -32 lines 0 comments Download
D ppapi/proxy/ppb_graphics_2d_proxy.h View 1 2 3 4 5 6 1 chunk +0 lines, -77 lines 0 comments Download
D ppapi/proxy/ppb_graphics_2d_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +0 lines, -314 lines 0 comments Download
M ppapi/proxy/ppb_instance_proxy.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -1 line 0 comments Download
M ppapi/proxy/ppb_instance_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 4 chunks +21 lines, -5 lines 0 comments Download
M ppapi/proxy/ppb_testing_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +8 lines, -9 lines 0 comments Download
M ppapi/proxy/resource_creation_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 3 chunks +3 lines, -3 lines 0 comments Download
M ppapi/tests/test_graphics_2d.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -0 lines 0 comments Download
M ppapi/tests/test_graphics_2d.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 3 chunks +41 lines, -8 lines 0 comments Download
M ppapi/thunk/interfaces_ppb_public_stable.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +1 line, -2 lines 0 comments Download
M ppapi/thunk/ppb_graphics_2d_api.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 4 chunks +10 lines, -1 line 0 comments Download
M webkit/plugins/ppapi/mock_plugin_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +2 lines, -0 lines 0 comments Download
M webkit/plugins/ppapi/mock_plugin_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +7 lines, -0 lines 0 comments Download
M webkit/plugins/ppapi/plugin_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 4 chunks +34 lines, -0 lines 0 comments Download
M webkit/plugins/ppapi/plugin_module.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +1 line, -2 lines 0 comments Download
M webkit/plugins/ppapi/ppapi_plugin_instance.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 3 chunks +4 lines, -4 lines 0 comments Download
M webkit/plugins/ppapi/ppapi_plugin_instance.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 9 chunks +22 lines, -32 lines 0 comments Download
M webkit/plugins/ppapi/ppb_graphics_2d_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +14 lines, -8 lines 0 comments Download
M webkit/plugins/ppapi/resource_creation_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +0 lines, -3 lines 0 comments Download
M webkit/plugins/ppapi/resource_creation_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +0 lines, -8 lines 0 comments Download

Messages

Total messages: 50 (0 generated)
victorhsieh
8 years, 2 months ago (2012-10-02 19:31:58 UTC) #1
brettw
After looking at this and thinking about it for a while, I'm wondering if it ...
8 years, 2 months ago (2012-10-02 21:18:57 UTC) #2
victorhsieh
Thanks for catching these. Regarding to the hacky Graphics2D bookkeeping, moving that logic to ResourceHost ...
8 years, 2 months ago (2012-10-02 22:15:03 UTC) #3
brettw
The API IDs were needed only by the "old" proxy design. I was hoping to ...
8 years, 2 months ago (2012-10-02 23:06:21 UTC) #4
brettw
I just realized something that came up in another review. The change I asked you ...
8 years, 2 months ago (2012-10-04 03:32:40 UTC) #5
victorhsieh
Tests are all passed. Changes from patch set 2 (last review): - addressed issues you ...
8 years, 2 months ago (2012-10-05 18:53:44 UTC) #6
victorhsieh
Removed ppapi/proxy from webkit/plugins/ppapi/DEPS. https://codereview.chromium.org/11053003/diff/20002/webkit/plugins/ppapi/ppapi_plugin_instance.cc File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): https://codereview.chromium.org/11053003/diff/20002/webkit/plugins/ppapi/ppapi_plugin_instance.cc#newcode2067 webkit/plugins/ppapi/ppapi_plugin_instance.cc:2067: bound_graphics_ = graphics_2d; On 2012/10/05 ...
8 years, 2 months ago (2012-10-05 20:27:37 UTC) #7
brettw
Looks pretty good so far. https://codereview.chromium.org/11053003/diff/25008/content/public/renderer/renderer_ppapi_host.h File content/public/renderer/renderer_ppapi_host.h (right): https://codereview.chromium.org/11053003/diff/25008/content/public/renderer/renderer_ppapi_host.h#newcode28 content/public/renderer/renderer_ppapi_host.h:28: virtual ppapi::host::PpapiHost* GetPpapiHost() const ...
8 years, 2 months ago (2012-10-05 22:38:06 UTC) #8
victorhsieh
Reply first. Dealing with the remaining. https://codereview.chromium.org/11053003/diff/25008/content/public/renderer/renderer_ppapi_host.h File content/public/renderer/renderer_ppapi_host.h (right): https://codereview.chromium.org/11053003/diff/25008/content/public/renderer/renderer_ppapi_host.h#newcode28 content/public/renderer/renderer_ppapi_host.h:28: virtual ppapi::host::PpapiHost* GetPpapiHost() ...
8 years, 2 months ago (2012-10-05 23:26:57 UTC) #9
victorhsieh
https://codereview.chromium.org/11053003/diff/25008/webkit/plugins/ppapi/ppapi_plugin_instance.cc File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): https://codereview.chromium.org/11053003/diff/25008/webkit/plugins/ppapi/ppapi_plugin_instance.cc#newcode2050 webkit/plugins/ppapi/ppapi_plugin_instance.cc:2050: EnterResourceNoLock<PPB_Graphics2D_API> enter_2d(device, false); On 2012/10/05 22:38:06, brettw wrote: > ...
8 years, 2 months ago (2012-10-08 19:17:02 UTC) #10
brettw
https://codereview.chromium.org/11053003/diff/25008/content/renderer/pepper/pepper_graphics_2d_host.h File content/renderer/pepper/pepper_graphics_2d_host.h (right): https://codereview.chromium.org/11053003/diff/25008/content/renderer/pepper/pepper_graphics_2d_host.h#newcode33 content/renderer/pepper/pepper_graphics_2d_host.h:33: static PepperGraphics2DHost* Create(RendererPpapiHost* host, Yeah, it kind of sucks ...
8 years, 2 months ago (2012-10-09 20:47:13 UTC) #11
brettw
http://codereview.chromium.org/11053003/diff/39002/ppapi/proxy/graphics_2d_resource.cc File ppapi/proxy/graphics_2d_resource.cc (right): http://codereview.chromium.org/11053003/diff/39002/ppapi/proxy/graphics_2d_resource.cc#newcode129 ppapi/proxy/graphics_2d_resource.cc:129: PpapiHostMsg_Graphics2D_Flush(kApiID), Just noticed this, you can actually remove all ...
8 years, 2 months ago (2012-10-10 04:22:39 UTC) #12
victorhsieh
All tests are passed, including paint manager and flash. The change in ~PepperGraphics2DHost is enough ...
8 years, 2 months ago (2012-10-11 22:56:23 UTC) #13
victorhsieh
BTW, this CL depends on a change to have a messaging bug fixed. https://codereview.chromium.org/11112004/
8 years, 2 months ago (2012-10-11 23:08:10 UTC) #14
brettw
LGTM, I'm really sorry for the delay this week. https://codereview.chromium.org/11053003/diff/55016/content/renderer/pepper/pepper_graphics_2d_host.cc File content/renderer/pepper/pepper_graphics_2d_host.cc (right): https://codereview.chromium.org/11053003/diff/55016/content/renderer/pepper/pepper_graphics_2d_host.cc#newcode44 content/renderer/pepper/pepper_graphics_2d_host.cc:44: ...
8 years, 2 months ago (2012-10-19 21:44:13 UTC) #15
victorhsieh
Thanks for reviewing this big change. https://codereview.chromium.org/11053003/diff/55016/content/renderer/pepper/pepper_graphics_2d_host.cc File content/renderer/pepper/pepper_graphics_2d_host.cc (right): https://codereview.chromium.org/11053003/diff/55016/content/renderer/pepper/pepper_graphics_2d_host.cc#newcode44 content/renderer/pepper/pepper_graphics_2d_host.cc:44: // Unbound from ...
8 years, 2 months ago (2012-10-22 02:18:52 UTC) #16
victorhsieh
Brett, would you please submit for me if this is ready? Thanks.
8 years, 1 month ago (2012-11-05 03:32:04 UTC) #17
victorhsieh
BTW, I've ran the same tests as before and all of them passed. Tests include ...
8 years, 1 month ago (2012-11-05 03:35:09 UTC) #18
victorhsieh
https://codereview.chromium.org/11053003/diff/77002/content/content_renderer.gypi File content/content_renderer.gypi (right): https://codereview.chromium.org/11053003/diff/77002/content/content_renderer.gypi#newcode272 content/content_renderer.gypi:272: # TODO: remove following once ppb_graphics_2d_impl is merged to ...
8 years, 1 month ago (2012-11-05 10:34:27 UTC) #19
brettw
Oh, sorry, I didn't realize you were waiting for me.
8 years, 1 month ago (2012-11-06 20:16:33 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11053003/77002
8 years, 1 month ago (2012-11-06 20:17:04 UTC) #21
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
8 years, 1 month ago (2012-11-06 20:32:06 UTC) #22
victorhsieh
ClearAndRun was removed just two days ago. Rebased to upstream and fixed now. Please try ...
8 years, 1 month ago (2012-11-07 03:02:52 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11053003/86004
8 years, 1 month ago (2012-11-07 05:06:18 UTC) #24
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
8 years, 1 month ago (2012-11-07 05:34:09 UTC) #25
victorhsieh
Brett, I've fixed chromium-style errors caught by buildbot (just learned that I can turn on ...
8 years, 1 month ago (2012-11-07 08:18:51 UTC) #26
brettw
Ah, sInce you're now using PPB_Graphics2D_Impl outside of the webkit_glue project, you need to export ...
8 years, 1 month ago (2012-11-07 18:37:43 UTC) #27
victorhsieh
Done. Please try again. Sorry for bothering! On 2012/11/07 18:37:43, brettw wrote: > Ah, sInce ...
8 years, 1 month ago (2012-11-08 02:32:40 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11053003/81043
8 years, 1 month ago (2012-11-08 06:32:52 UTC) #29
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
8 years, 1 month ago (2012-11-08 07:03:47 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11053003/91004
8 years, 1 month ago (2012-11-13 19:11:43 UTC) #31
commit-bot: I haz the power
Presubmit check for 11053003-91004 failed and returned exit status 1. Running presubmit commit checks ...
8 years, 1 month ago (2012-11-13 19:12:10 UTC) #32
victorhsieh
@cdn, would you please take a look at ppapi_messages.h? Thanks. On 2012/11/13 19:12:10, I haz ...
8 years, 1 month ago (2012-11-14 00:59:51 UTC) #33
Cris Neckar
https://codereview.chromium.org/11053003/diff/81096/ppapi/proxy/graphics_2d_resource.cc File ppapi/proxy/graphics_2d_resource.cc (right): https://codereview.chromium.org/11053003/diff/81096/ppapi/proxy/graphics_2d_resource.cc#newcode36 ppapi/proxy/graphics_2d_resource.cc:36: std::numeric_limits<int32>::max(); when width * height * 4 wraps the ...
8 years, 1 month ago (2012-11-14 02:03:46 UTC) #34
victorhsieh
https://codereview.chromium.org/11053003/diff/81096/ppapi/proxy/graphics_2d_resource.cc File ppapi/proxy/graphics_2d_resource.cc (right): https://codereview.chromium.org/11053003/diff/81096/ppapi/proxy/graphics_2d_resource.cc#newcode36 ppapi/proxy/graphics_2d_resource.cc:36: std::numeric_limits<int32>::max(); On 2012/11/14 02:03:46, Cris Neckar wrote: > when ...
8 years, 1 month ago (2012-11-14 02:25:51 UTC) #35
Cris Neckar
On 2012/11/14 02:25:51, Victor Hsieh wrote: > https://codereview.chromium.org/11053003/diff/81096/ppapi/proxy/graphics_2d_resource.cc > File ppapi/proxy/graphics_2d_resource.cc (right): > > https://codereview.chromium.org/11053003/diff/81096/ppapi/proxy/graphics_2d_resource.cc#newcode36 ...
8 years, 1 month ago (2012-11-14 03:32:04 UTC) #36
Cris Neckar
On 2012/11/14 03:32:04, Cris Neckar wrote: > On 2012/11/14 02:25:51, Victor Hsieh wrote: > > ...
8 years, 1 month ago (2012-11-14 03:33:56 UTC) #37
victorhsieh
Oh I see what you were saying. Fixed, plus a test case. Thanks for catching.
8 years, 1 month ago (2012-11-14 04:38:50 UTC) #38
Cris Neckar
On 2012/11/14 04:38:50, Victor Hsieh wrote: > Oh I see what you were saying. Fixed, ...
8 years, 1 month ago (2012-11-14 05:16:23 UTC) #39
victorhsieh
On 2012/11/14 05:16:23, Cris Neckar wrote: > On 2012/11/14 04:38:50, Victor Hsieh wrote: > > ...
8 years, 1 month ago (2012-11-14 06:22:46 UTC) #40
Cris Neckar
https://codereview.chromium.org/11053003/diff/91008/webkit/plugins/ppapi/ppb_image_data_impl.cc File webkit/plugins/ppapi/ppb_image_data_impl.cc (right): https://codereview.chromium.org/11053003/diff/91008/webkit/plugins/ppapi/ppb_image_data_impl.cc#newcode58 webkit/plugins/ppapi/ppb_image_data_impl.cc:58: std::numeric_limits<int32>::max() / 4) Oops, actually we need to fix ...
8 years, 1 month ago (2012-11-14 07:15:56 UTC) #41
Cris Neckar
> Oops, actually we need to fix this in a separate patch so that it ...
8 years, 1 month ago (2012-11-14 07:32:08 UTC) #42
Pam (message me for reviews)
Drive-by: Drive-by: Please be more complete in your change description. See other drive-by comment, in ...
8 years, 1 month ago (2012-11-14 13:07:48 UTC) #43
victorhsieh
On 2012/11/14 13:07:48, Pam wrote: > Drive-by: Drive-by: Please be more complete in your change ...
8 years, 1 month ago (2012-11-14 13:22:49 UTC) #44
Cris Neckar
The new IPC looks OK but I question whether we actually need to add the ...
8 years, 1 month ago (2012-11-14 20:39:18 UTC) #45
victorhsieh
Unfortunately it's not trivial to not introduce that message. As I mentioned, we are refactoring ...
8 years, 1 month ago (2012-11-14 23:17:36 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11053003/91008
8 years, 1 month ago (2012-11-14 23:49:00 UTC) #47
commit-bot: I haz the power
Retried try job too often for step(s) nacl_integration
8 years, 1 month ago (2012-11-15 01:25:33 UTC) #48
Cris Neckar
lgtm for ipc changes
8 years ago (2012-11-26 19:22:09 UTC) #49
brettw
8 years ago (2012-11-26 21:13:13 UTC) #50

Powered by Google App Engine
This is Rietveld 408576698