Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(27)

Issue 2643063002: Refactor Blink's ServiceConnector and add ability to mock in layout tests (Closed)

Created:
3 years, 8 months ago by blundell
Modified:
3 years, 6 months ago
CC:
Aaron Boodman, abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, dcheng, dglazkov+blink, esprehn, haraken, ke.he, kinuko+watch, leonhsl(Using Gerrit), mlamouri+watch-content_chromium.org, mlamouri+watch-blink_chromium.org, nasko+codewatch_chromium.org, ortuno, qsr+mojo_chromium.org, timvolodine, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Refactor Blink's ServiceConnector and add ability to mock in layout tests This CL refactors the implementation of Blink's ServiceConnector so that layout tests can mock remote interfaces of arbitrary services in JS. Specifically, it changes the organization to be roughly parallel to Platform::interfaceProvider(). - Blink has a Connector interface. - Platform has a connector() method. - //content/renderer has a BlinkConnectorImpl backed directly by //content/renderer's Connector. - BlinkConnectorImpl also has the ability to override interfaces with local implementations, similar to service_manager::InterfaceProvider. - We add a JS wrapper of BlinkConnectorImpl to expose that ability to JS. Review-Url: https://codereview.chromium.org/2643063002 Cr-Commit-Position: refs/heads/master@{#459064} Committed: https://chromium.googlesource.com/chromium/src/+/43dd7b3dd9c675d5f9df67feae7662ee196517fa

Patch Set 1 #

Patch Set 2 : Rebase, clean up, add unit tests #

Patch Set 3 : Rebase, satisfy gn check #

Patch Set 4 : Rebase + test fix #

Patch Set 5 : Remove bindServiceConnector() #

Patch Set 6 : Rebase #

Patch Set 7 : Rebase #

Patch Set 8 : Rebase #

Patch Set 9 : Rebase #

Total comments: 11

Patch Set 10 : Rebase #

Patch Set 11 : Response to review #

Patch Set 12 : fix compile #

Total comments: 9

Patch Set 13 : Response to review #

Patch Set 14 : Fix build #

Unified diffs Side-by-side diffs Delta from patch set Stats (+620 lines, -123 lines) Patch
M content/child/blink_platform_impl.h View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -3 lines 0 comments Download
M content/child/blink_platform_impl.cc View 1 2 3 4 5 6 7 8 9 2 chunks +0 lines, -15 lines 0 comments Download
M content/renderer/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -0 lines 0 comments Download
A content/renderer/mojo/blink_connector_impl.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +73 lines, -0 lines 0 comments Download
A content/renderer/mojo/blink_connector_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +90 lines, -0 lines 0 comments Download
A content/renderer/mojo/blink_connector_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +158 lines, -0 lines 0 comments Download
A content/renderer/mojo/blink_connector_js_wrapper.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +73 lines, -0 lines 0 comments Download
A content/renderer/mojo/blink_connector_js_wrapper.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +104 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +9 lines, -0 lines 0 comments Download
M content/renderer/renderer_blink_platform_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +5 lines, -0 lines 0 comments Download
M content/renderer/renderer_blink_platform_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +11 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/resources/mojo-helpers.js View 1 2 3 1 chunk +33 lines, -27 lines 0 comments Download
M third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.cpp View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +0 lines, -1 line 0 comments Download
D third_party/WebKit/Source/platform/ServiceConnector.h View 1 chunk +0 lines, -60 lines 0 comments Download
M third_party/WebKit/Source/platform/exported/Platform.cpp View 1 2 3 4 5 6 7 8 9 3 chunks +7 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/exported/ServiceRegistry.cpp View 1 2 3 4 5 6 7 8 9 10 2 chunks +12 lines, -0 lines 0 comments Download
M third_party/WebKit/public/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
A third_party/WebKit/public/platform/Connector.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +32 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/Platform.h View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -9 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 77 (37 generated)
blundell
esprehn@: For review (whole CL). haraken@, dcheng@, rockot@, Ke He: FYI. I verified on a ...
3 years, 8 months ago (2017-01-20 14:40:36 UTC) #16
blundell
Friendly ping.
3 years, 7 months ago (2017-01-24 11:37:40 UTC) #17
Ken Rockot(use gerrit already)
Ping? It would be nice to get this CL landed.
3 years, 7 months ago (2017-01-25 16:37:59 UTC) #19
esprehn
So the main thread specific stuff I see all over here is a big red ...
3 years, 7 months ago (2017-02-01 06:02:23 UTC) #21
dcheng
https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/blink_connector_impl.cc File content/renderer/mojo/blink_connector_impl.cc (right): https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/blink_connector_impl.cc#newcode29 content/renderer/mojo/blink_connector_impl.cc:29: if (!connector_.get()) Nit: no .get(). Also... it would be ...
3 years, 7 months ago (2017-02-01 07:45:16 UTC) #22
dcheng
Sorry for the delay on this review =( I'll mostly defer to esprehn@ since he's ...
3 years, 7 months ago (2017-02-01 07:46:06 UTC) #23
Ken Rockot(use gerrit already)
We certainly want to stick to using mojom-generated bindings for Service Manager APIs, which means ...
3 years, 7 months ago (2017-02-01 19:16:00 UTC) #24
Ken Rockot(use gerrit already)
On 2017/02/01 at 19:16:00, Ken Rockot wrote: > We certainly want to stick to using ...
3 years, 7 months ago (2017-02-01 19:16:33 UTC) #25
esprehn
So where are we on this?
3 years, 7 months ago (2017-02-15 02:10:17 UTC) #26
leonhsl(Using Gerrit)
3 years, 7 months ago (2017-02-15 03:28:31 UTC) #27
blundell
On 2017/02/15 02:10:17, esprehn wrote: > So where are we on this? I'm curious for ...
3 years, 7 months ago (2017-02-20 16:10:54 UTC) #28
blundell
dcheng@: ping :).
3 years, 6 months ago (2017-03-14 12:16:58 UTC) #29
blundell
On 2017/03/14 12:16:58, blundell wrote: > dcheng@: ping :). bump :)
3 years, 6 months ago (2017-03-17 20:48:17 UTC) #30
esprehn
3 years, 6 months ago (2017-03-20 23:17:34 UTC) #33
dglazkov
On 2017/02/20 at 16:10:54, blundell wrote: > On 2017/02/15 02:10:17, esprehn wrote: > > So ...
3 years, 6 months ago (2017-03-21 14:42:41 UTC) #35
blundell
On 2017/03/21 14:42:41, dglazkov wrote: > On 2017/02/20 at 16:10:54, blundell wrote: > > On ...
3 years, 6 months ago (2017-03-21 14:52:39 UTC) #36
Ken Rockot(use gerrit already)
One final comment now that this has been resurrected: I would prefer we called the ...
3 years, 6 months ago (2017-03-21 15:09:21 UTC) #37
blundell
s/getInterface/bindInterface done. Also changed a mistaken usage of std::map::insert() that leon.han@intel.com helpfully pointed out. Dimitri, ...
3 years, 6 months ago (2017-03-21 16:34:21 UTC) #40
dglazkov
On 2017/03/21 at 16:34:21, blundell wrote: > s/getInterface/bindInterface done. Also changed a mistaken usage of ...
3 years, 6 months ago (2017-03-21 17:31:04 UTC) #47
dglazkov
lgtm
3 years, 6 months ago (2017-03-21 17:39:04 UTC) #48
dcheng
LGTM (Sorry for the long review latency) https://codereview.chromium.org/2643063002/diff/240001/content/renderer/mojo/blink_connector_impl.cc File content/renderer/mojo/blink_connector_impl.cc (right): https://codereview.chromium.org/2643063002/diff/240001/content/renderer/mojo/blink_connector_impl.cc#newcode29 content/renderer/mojo/blink_connector_impl.cc:29: if (!connector_.get()) ...
3 years, 6 months ago (2017-03-21 18:56:18 UTC) #51
jam
Can you explain why we have BlinkConnectorImpl which wraps service_manager::Connector, instead of simply exposing the ...
3 years, 6 months ago (2017-03-21 22:02:17 UTC) #52
blundell
On 2017/03/21 22:02:17, jam wrote: > Can you explain why we have BlinkConnectorImpl which wraps ...
3 years, 6 months ago (2017-03-22 11:33:15 UTC) #53
jam
On 2017/03/22 11:33:15, blundell wrote: > On 2017/03/21 22:02:17, jam wrote: > > Can you ...
3 years, 6 months ago (2017-03-22 15:15:52 UTC) #54
Ken Rockot(use gerrit already)
On Wed, Mar 22, 2017 at 8:15 AM, <jam@chromium.org> wrote: > On 2017/03/22 11:33:15, blundell ...
3 years, 6 months ago (2017-03-22 15:29:22 UTC) #55
Ken Rockot(use gerrit already)
On Wed, Mar 22, 2017 at 8:15 AM, <jam@chromium.org> wrote: > On 2017/03/22 11:33:15, blundell ...
3 years, 6 months ago (2017-03-22 15:29:22 UTC) #56
blundell
On 2017/03/22 15:15:52, jam wrote: > On 2017/03/22 11:33:15, blundell wrote: > > On 2017/03/21 ...
3 years, 6 months ago (2017-03-22 15:30:49 UTC) #57
Ken Rockot(use gerrit already)
On Wed, Mar 22, 2017 at 8:30 AM, <blundell@chromium.org> wrote: > On 2017/03/22 15:15:52, jam ...
3 years, 6 months ago (2017-03-22 15:35:03 UTC) #58
Ken Rockot(use gerrit already)
On Wed, Mar 22, 2017 at 8:30 AM, <blundell@chromium.org> wrote: > On 2017/03/22 15:15:52, jam ...
3 years, 6 months ago (2017-03-22 15:35:05 UTC) #59
jam
On 2017/03/22 15:35:05, Ken Rockot wrote: > On Wed, Mar 22, 2017 at 8:30 AM, ...
3 years, 6 months ago (2017-03-22 15:44:25 UTC) #60
Ken Rockot(use gerrit already)
On Wed, Mar 22, 2017 at 8:44 AM, <jam@chromium.org> wrote: > On 2017/03/22 15:35:05, Ken ...
3 years, 6 months ago (2017-03-22 15:46:53 UTC) #61
Ken Rockot(use gerrit already)
On Wed, Mar 22, 2017 at 8:44 AM, <jam@chromium.org> wrote: > On 2017/03/22 15:35:05, Ken ...
3 years, 6 months ago (2017-03-22 15:46:53 UTC) #62
jam
lgtm Colin/Ken/I chatted, and since this change blocks other cls and Dimitri is gone for ...
3 years, 6 months ago (2017-03-22 17:21:35 UTC) #63
Ken Rockot(use gerrit already)
https://codereview.chromium.org/2643063002/diff/240001/third_party/WebKit/LayoutTests/resources/mojo-helpers.js File third_party/WebKit/LayoutTests/resources/mojo-helpers.js (right): https://codereview.chromium.org/2643063002/diff/240001/third_party/WebKit/LayoutTests/resources/mojo-helpers.js#newcode45 third_party/WebKit/LayoutTests/resources/mojo-helpers.js:45: connector.clearInterfaceOverridesForTesting(); On 2017/03/21 at 18:56:18, dcheng wrote: > Do ...
3 years, 6 months ago (2017-03-23 04:56:04 UTC) #64
blundell
On 2017/03/21 17:31:04, dglazkov OOO till March 27 wrote: > On 2017/03/21 at 16:34:21, blundell ...
3 years, 6 months ago (2017-03-23 09:43:50 UTC) #65
blundell
Thanks, everyone! https://codereview.chromium.org/2643063002/diff/240001/content/renderer/mojo/blink_connector_impl.cc File content/renderer/mojo/blink_connector_impl.cc (right): https://codereview.chromium.org/2643063002/diff/240001/content/renderer/mojo/blink_connector_impl.cc#newcode29 content/renderer/mojo/blink_connector_impl.cc:29: if (!connector_.get()) On 2017/03/21 18:56:18, dcheng wrote: ...
3 years, 6 months ago (2017-03-23 10:40:34 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2643063002/260001
3 years, 6 months ago (2017-03-23 10:40:57 UTC) #69
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng/builds/38086)
3 years, 6 months ago (2017-03-23 10:50:19 UTC) #71
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2643063002/280001
3 years, 6 months ago (2017-03-23 11:48:52 UTC) #74
commit-bot: I haz the power
3 years, 6 months ago (2017-03-23 13:26:20 UTC) #77
Message was sent while issue was closed.
Committed patchset #14 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/43dd7b3dd9c675d5f9df67feae76...

Powered by Google App Engine
This is Rietveld 408576698