Chromium Code Reviews
Help | Chromium Project | Sign in
(32)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 2 months ago by victorhsieh
Modified:
4 years ago
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
Commit queue not available (can’t edit this change).

Messages

Total messages: 50 (0 generated)
victorhsieh
4 years, 2 months ago (2012-10-02 19:31:58 UTC) #1
brettw (ping after 24h)
After looking at this and thinking about it for a while, I'm wondering if it ...
4 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 ...
4 years, 2 months ago (2012-10-02 22:15:03 UTC) #3
brettw (ping after 24h)
The API IDs were needed only by the "old" proxy design. I was hoping to ...
4 years, 2 months ago (2012-10-02 23:06:21 UTC) #4
brettw (ping after 24h)
I just realized something that came up in another review. The change I asked you ...
4 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 ...
4 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 ...
4 years, 2 months ago (2012-10-05 20:27:37 UTC) #7
brettw (ping after 24h)
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 ...
4 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() ...
4 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: > ...
4 years, 2 months ago (2012-10-08 19:17:02 UTC) #10
brettw (ping after 24h)
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 ...
4 years, 2 months ago (2012-10-09 20:47:13 UTC) #11
brettw (ping after 24h)
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 ...
4 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 ...
4 years, 1 month 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/
4 years, 1 month ago (2012-10-11 23:08:10 UTC) #14
brettw (ping after 24h)
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: ...
4 years, 1 month 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 ...
4 years, 1 month ago (2012-10-22 02:18:52 UTC) #16
victorhsieh
Brett, would you please submit for me if this is ready? Thanks.
4 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 ...
4 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 ...
4 years, 1 month ago (2012-11-05 10:34:27 UTC) #19
brettw (ping after 24h)
Oh, sorry, I didn't realize you were waiting for me.
4 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
4 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 ...
4 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 ...
4 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
4 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 ...
4 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 ...
4 years, 1 month ago (2012-11-07 08:18:51 UTC) #26
brettw (ping after 24h)
Ah, sInce you're now using PPB_Graphics2D_Impl outside of the webkit_glue project, you need to export ...
4 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 ...
4 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
4 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 ...
4 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
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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: > > ...
4 years 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.
4 years 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, ...
4 years 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: > > ...
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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
4 years 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
4 years ago (2012-11-15 01:25:33 UTC) #48
Cris Neckar
lgtm for ipc changes
4 years ago (2012-11-26 19:22:09 UTC) #49
brettw (ping after 24h)
4 years ago (2012-11-26 21:13:13 UTC) #50
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd