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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by victorhsieh
Modified:
1 year, 4 months ago
Reviewers:
brettw, Cris Neckar
CC:
chromium-reviews_chromium.org, 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) Lint 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 0 errors 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 ? errors 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 0 errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors Download
D ppapi/proxy/ppb_graphics_2d_proxy.h View 1 2 3 4 5 6 1 chunk +0 lines, -77 lines 0 comments 0 errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 0 errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors Download
Commit:

Messages

Total messages: 50
victorhsieh
1 year, 6 months ago #1
brettw
After looking at this and thinking about it for a while, I'm wondering if it ...
1 year, 6 months ago #2
victorhsieh
Thanks for catching these. Regarding to the hacky Graphics2D bookkeeping, moving that logic to ResourceHost ...
1 year, 6 months ago #3
brettw
The API IDs were needed only by the "old" proxy design. I was hoping to ...
1 year, 6 months ago #4
brettw
I just realized something that came up in another review. The change I asked you ...
1 year, 6 months ago #5
victorhsieh
Tests are all passed. Changes from patch set 2 (last review): - addressed issues you ...
1 year, 6 months ago #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 ...
1 year, 6 months ago #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 ...
1 year, 6 months ago #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() ...
1 year, 6 months ago #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: > ...
1 year, 6 months ago #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 ...
1 year, 6 months ago #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 ...
1 year, 6 months ago #12
victorhsieh
All tests are passed, including paint manager and flash. The change in ~PepperGraphics2DHost is enough ...
1 year, 6 months ago #13
victorhsieh
BTW, this CL depends on a change to have a messaging bug fixed. https://codereview.chromium.org/11112004/
1 year, 6 months ago #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: ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #16
victorhsieh
Brett, would you please submit for me if this is ready? Thanks.
1 year, 5 months ago #17
victorhsieh
BTW, I've ran the same tests as before and all of them passed. Tests include ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #19
brettw
Oh, sorry, I didn't realize you were waiting for me.
1 year, 5 months ago #20
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11053003/77002
1 year, 5 months ago #21
I haz the power (commit-bot)
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
1 year, 5 months ago #22
victorhsieh
ClearAndRun was removed just two days ago. Rebased to upstream and fixed now. Please try ...
1 year, 5 months ago #23
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11053003/86004
1 year, 5 months ago #24
I haz the power (commit-bot)
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
1 year, 5 months ago #25
victorhsieh
Brett, I've fixed chromium-style errors caught by buildbot (just learned that I can turn on ...
1 year, 5 months ago #26
brettw
Ah, sInce you're now using PPB_Graphics2D_Impl outside of the webkit_glue project, you need to export ...
1 year, 5 months ago #27
victorhsieh
Done. Please try again. Sorry for bothering! On 2012/11/07 18:37:43, brettw wrote: > Ah, sInce ...
1 year, 5 months ago #28
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11053003/81043
1 year, 5 months ago #29
I haz the power (commit-bot)
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
1 year, 5 months ago #30
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11053003/91004
1 year, 5 months ago #31
I haz the power (commit-bot)
Presubmit check for 11053003-91004 failed and returned exit status 1. Running presubmit commit checks ...
1 year, 5 months ago #32
victorhsieh
@cdn, would you please take a look at ppapi_messages.h? Thanks. On 2012/11/13 19:12:10, I haz ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #36
Cris Neckar
On 2012/11/14 03:32:04, Cris Neckar wrote: > On 2012/11/14 02:25:51, Victor Hsieh wrote: > > ...
1 year, 5 months ago #37
victorhsieh
Oh I see what you were saying. Fixed, plus a test case. Thanks for catching.
1 year, 5 months ago #38
Cris Neckar
On 2012/11/14 04:38:50, Victor Hsieh wrote: > Oh I see what you were saying. Fixed, ...
1 year, 5 months ago #39
victorhsieh
On 2012/11/14 05:16:23, Cris Neckar wrote: > On 2012/11/14 04:38:50, Victor Hsieh wrote: > > ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #41
Cris Neckar
> Oops, actually we need to fix this in a separate patch so that it ...
1 year, 5 months ago #42
Pam (also PM for reviews)
Drive-by: Drive-by: Please be more complete in your change description. See other drive-by comment, in ...
1 year, 5 months ago #43
victorhsieh
On 2012/11/14 13:07:48, Pam wrote: > Drive-by: Drive-by: Please be more complete in your change ...
1 year, 5 months ago #44
Cris Neckar
The new IPC looks OK but I question whether we actually need to add the ...
1 year, 5 months ago #45
victorhsieh
Unfortunately it's not trivial to not introduce that message. As I mentioned, we are refactoring ...
1 year, 5 months ago #46
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11053003/91008
1 year, 5 months ago #47
I haz the power (commit-bot)
Retried try job too often for step(s) nacl_integration
1 year, 5 months ago #48
Cris Neckar
lgtm for ipc changes
1 year, 4 months ago #49
brettw
1 year, 4 months ago #50
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1275:d14800f88434