|
|
Created:
4 years, 3 months ago by Ken Rockot(use gerrit already) Modified:
4 years, 2 months ago CC:
Aaron Boodman, abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-style_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, Dirk Pranke, esprehn, jam, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove ViewHostMsg_CreateWindow to mojom
Converts this message, addressing several of its dependencies
in the process:
- Makes content::WindowContainerType an alias for the new
content::mojom::WindowContainerType. Some transitional public
constants have been added to satisfy existing references to
the old enum values. This will be removed in future CLs.
- Adds a mojom definition for blink::mojom::WindowFeatures,
corresponding to blink::WebWindowFeatures. A typemap is
added between these types and is shared by both Blink and
Chromium bindings configurations.
- Adds a blink::mojom::Referrer struct analogous to
content::Renderer. Chromium bindings typemap this new mojom
struct to content::Referrer.
- Adds a mojom blink::mojom::ReferrerPolicy enum with a typemap
to blink::WebReferrerPolicy, shared by Chromium and Blink
bindings. This can be de-duplicated in future CLs.
BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/5c478a7b04b55630c5c491d4d1d2863e965cfa32
Cr-Commit-Position: refs/heads/master@{#421674}
Patch Set 1 #Patch Set 2 : . #
Total comments: 31
Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #Dependent Patchsets: Messages
Total messages: 103 (80 generated)
Description was changed from ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType is an alias for the new content::mojom::WindowContainerType. Some public constants have been added to satisfy existing references to the old enum values. - Adds a mojom definition for WindowFeatures corresponding to WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds mojo::StringTraits<blink::WebString> and mojo::ArrayTraits<blink::WebVector> to support WebWindowFeatures typemap. - content::Referrer now has a mojom equivalent in third_party/WebKit called blink::mojom::Referrer. Chromium bindings typemap this to content::Referrer. - blink::mojom::ReferrerPolicy has a typemap to blink::WebReferrerPolicy shared by Chromium and Blink bindings. BUG=612500 ========== to ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType is an alias for the new content::mojom::WindowContainerType. Some public constants have been added to satisfy existing references to the old enum values. - Adds a mojom definition for WindowFeatures corresponding to WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds mojo::StringTraits<blink::WebString> and mojo::ArrayTraits<blink::WebVector> to support WebWindowFeatures typemap. - content::Referrer now has a mojom equivalent in third_party/WebKit called blink::mojom::Referrer. Chromium bindings typemap this to content::Referrer. - blink::mojom::ReferrerPolicy has a typemap to blink::WebReferrerPolicy shared by Chromium and Blink bindings. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType is an alias for the new content::mojom::WindowContainerType. Some public constants have been added to satisfy existing references to the old enum values. - Adds a mojom definition for WindowFeatures corresponding to WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds mojo::StringTraits<blink::WebString> and mojo::ArrayTraits<blink::WebVector> to support WebWindowFeatures typemap. - content::Referrer now has a mojom equivalent in third_party/WebKit called blink::mojom::Referrer. Chromium bindings typemap this to content::Referrer. - blink::mojom::ReferrerPolicy has a typemap to blink::WebReferrerPolicy shared by Chromium and Blink bindings. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType an alias for the new content::mojom::WindowContainerType. Some public constants have been added to satisfy existing references to the old enum values. - Adds a mojom definition for WindowFeatures corresponding to WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds mojo::StringTraits<blink::WebString> and mojo::ArrayTraits<blink::WebVector> to support WebWindowFeatures typemap. - content::Referrer now has a mojom equivalent in third_party/WebKit called blink::mojom::Referrer. Chromium bindings typemap this to content::Referrer. - blink::mojom::ReferrerPolicy has a typemap to blink::WebReferrerPolicy shared by Chromium and Blink bindings. This can be de-duplicated in future CLs. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by [email protected] to run a CQ dry run
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType an alias for the new content::mojom::WindowContainerType. Some public constants have been added to satisfy existing references to the old enum values. - Adds a mojom definition for WindowFeatures corresponding to WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds mojo::StringTraits<blink::WebString> and mojo::ArrayTraits<blink::WebVector> to support WebWindowFeatures typemap. - content::Referrer now has a mojom equivalent in third_party/WebKit called blink::mojom::Referrer. Chromium bindings typemap this to content::Referrer. - blink::mojom::ReferrerPolicy has a typemap to blink::WebReferrerPolicy shared by Chromium and Blink bindings. This can be de-duplicated in future CLs. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType an alias for the new content::mojom::WindowContainerType. Some transitional public constants have been added to satisfy existing references to the old enum values. This will be removed in future CLs. - Adds a mojom definition for blink::mojom::WindowFeatures, corresponding to blink::WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds Mojo string and array traits for WebString and WebVector to support public blink mojom types WindowFeatures. - Adds a blink::mojom::Referrer struct analogous to content::Renderer. Chromium bindings typemap this new mojom struct to content::Referrer. - Adds a mojom blink::mojom::ReferrerPolicy enum with a typemap to blink::WebReferrerPolicy, shared by Chromium and Blink bindings. This can be de-duplicated in future CLs. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
The CQ bit was checked by [email protected] to run a CQ dry run
Patchset #1 (id:160001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType an alias for the new content::mojom::WindowContainerType. Some transitional public constants have been added to satisfy existing references to the old enum values. This will be removed in future CLs. - Adds a mojom definition for blink::mojom::WindowFeatures, corresponding to blink::WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds Mojo string and array traits for WebString and WebVector to support public blink mojom types WindowFeatures. - Adds a blink::mojom::Referrer struct analogous to content::Renderer. Chromium bindings typemap this new mojom struct to content::Referrer. - Adds a mojom blink::mojom::ReferrerPolicy enum with a typemap to blink::WebReferrerPolicy, shared by Chromium and Blink bindings. This can be de-duplicated in future CLs. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType an alias for the new content::mojom::WindowContainerType. Some transitional public constants have been added to satisfy existing references to the old enum values. This will be removed in future CLs. - Adds a mojom definition for blink::mojom::WindowFeatures, corresponding to blink::WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds a blink::mojom::Referrer struct analogous to content::Renderer. Chromium bindings typemap this new mojom struct to content::Referrer. - Adds a mojom blink::mojom::ReferrerPolicy enum with a typemap to blink::WebReferrerPolicy, shared by Chromium and Blink bindings. This can be de-duplicated in future CLs. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
[email protected] changed reviewers: + [email protected]
[email protected] changed reviewers: + [email protected], [email protected]
Adding nick@, as he has done a lot of work for mojoifying this IPC.
Description was changed from ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType an alias for the new content::mojom::WindowContainerType. Some transitional public constants have been added to satisfy existing references to the old enum values. This will be removed in future CLs. - Adds a mojom definition for blink::mojom::WindowFeatures, corresponding to blink::WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds a blink::mojom::Referrer struct analogous to content::Renderer. Chromium bindings typemap this new mojom struct to content::Referrer. - Adds a mojom blink::mojom::ReferrerPolicy enum with a typemap to blink::WebReferrerPolicy, shared by Chromium and Blink bindings. This can be de-duplicated in future CLs. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType an alias for the new content::mojom::WindowContainerType. Some transitional public constants have been added to satisfy existing references to the old enum values. This will be removed in future CLs. - Adds a mojom definition for blink::mojom::WindowFeatures, corresponding to blink::WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds a blink::mojom::Referrer struct analogous to content::Renderer. Chromium bindings typemap this new mojom struct to content::Referrer. - Adds a mojom blink::mojom::ReferrerPolicy enum with a typemap to blink::WebReferrerPolicy, shared by Chromium and Blink bindings. This can be de-duplicated in future CLs. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
[email protected] changed reviewers: - [email protected]
lgtm
[email protected] changed reviewers: + [email protected]
+dcheng - I notice you're in third_party/WebKit/public OWNERS. Could you please review for that and security in general?
https://codereview.chromium.org/2363573002/diff/200001/content/browser/render... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/2363573002/diff/200001/content/browser/render... content/browser/renderer_host/render_message_filter.cc:301: callback.Run(std::move(reply)); Can we now guarantee that the task posted to the UI thread by |CreateNewWindow| will run before any UI-thread destined mojo message that may be sent from the child process after the reply message is received? https://codereview.chromium.org/2363573002/diff/200001/content/common/render_... File content/common/render_message_filter.mojom (right): https://codereview.chromium.org/2363573002/diff/200001/content/common/render_... content/common/render_message_filter.mojom:58: blink.mojom.WindowFeatures features; I double checked, and removing |additional_features| seems okay. https://codereview.chromium.org/2363573002/diff/200001/content/common/render_... content/common/render_message_filter.mojom:67: int32 main_frame_route_id; Long-term, do you think we'll eventually be returning FrameHost mojom interfaces here (and other places where we mint routing ids)? https://codereview.chromium.org/2363573002/diff/200001/content/public/test/mo... File content/public/test/mock_render_thread.cc (right): https://codereview.chromium.org/2363573002/diff/200001/content/public/test/mo... content/public/test/mock_render_thread.cc:50: // Note that ? https://codereview.chromium.org/2363573002/diff/200001/content/public/test/mo... File content/public/test/mock_render_thread.h (right): https://codereview.chromium.org/2363573002/diff/200001/content/public/test/mo... content/public/test/mock_render_thread.h:45: // ViewMsg_Close : async, send to the Widget. Let's redo this class comment so that it better describes why this class exists. Something like this: // This class provides a mock implementation of RenderThread, which is not // actually backed by a channel to a browser process. Only a few IPC messages // are handled, just enough allow top-level objects (e.g. frames and widgets) // to be created. // // This class is designed for inheritance: embedders of content may subclass // it to handle additional IPCs. https://codereview.chromium.org/2363573002/diff/200001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2363573002/diff/200001/content/renderer/rende... content/renderer/render_view_impl.cc:1556: RenderThreadImpl::current_render_message_filter()->CreateNewWindow( Why use this global and getter? Is it just about avoiding exposing it on RenderThread (which is content/public)? It seems weird to call into RenderThreadImpl:: to get a value that's owned by the MockRenderThread. I'm not totally against this, but curious if there are any alternatives worth considering.
Thanks for looking https://codereview.chromium.org/2363573002/diff/200001/content/browser/render... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/2363573002/diff/200001/content/browser/render... content/browser/renderer_host/render_message_filter.cc:301: callback.Run(std::move(reply)); On 2016/09/27 at 20:32:41, ncarter wrote: > Can we now guarantee that the task posted to the UI thread by |CreateNewWindow| will run before any UI-thread destined mojo message that may be sent from the child process after the reply message is received? Yes. Any interfaces (such as mojom::RenderMessageFilter) which are associated with the IPC::Channel exhibit perfect ordering with respect to Channel on the IO thread and ChannelProxy on the main thread. To put it another way, this conversion has zero impact on the ordering of the message or its reply. https://codereview.chromium.org/2363573002/diff/200001/content/common/render_... File content/common/render_message_filter.mojom (right): https://codereview.chromium.org/2363573002/diff/200001/content/common/render_... content/common/render_message_filter.mojom:58: blink.mojom.WindowFeatures features; On 2016/09/27 at 20:32:41, ncarter wrote: > I double checked, and removing |additional_features| seems okay. Ack. https://codereview.chromium.org/2363573002/diff/200001/content/common/render_... content/common/render_message_filter.mojom:67: int32 main_frame_route_id; On 2016/09/27 at 20:32:41, ncarter wrote: > Long-term, do you think we'll eventually be returning FrameHost mojom interfaces here (and other places where we mint routing ids)? Yes, that's the idea. I may even try to start pairing primordial interfaces with routing IDs sooner rather than later if it means we can avoid routing ID params in other message conversions (i.e. imagine if every message which introduced a new routing ID to its receiver also provided an AssociatedInterfaceProviderPtr). Starting small though, I decided to stick with rote conversion here. https://codereview.chromium.org/2363573002/diff/200001/content/public/test/mo... File content/public/test/mock_render_thread.cc (right): https://codereview.chromium.org/2363573002/diff/200001/content/public/test/mo... content/public/test/mock_render_thread.cc:50: // Note that On 2016/09/27 at 20:32:41, ncarter wrote: > ? Oops. :> Removed - this was going to be the comment which is now in the method above. https://codereview.chromium.org/2363573002/diff/200001/content/public/test/mo... File content/public/test/mock_render_thread.h (right): https://codereview.chromium.org/2363573002/diff/200001/content/public/test/mo... content/public/test/mock_render_thread.h:45: // ViewMsg_Close : async, send to the Widget. On 2016/09/27 at 20:32:41, ncarter wrote: > Let's redo this class comment so that it better describes why this class exists. Something like this: > > // This class provides a mock implementation of RenderThread, which is not > // actually backed by a channel to a browser process. Only a few IPC messages > // are handled, just enough allow top-level objects (e.g. frames and widgets) > // to be created. > // > // This class is designed for inheritance: embedders of content may subclass > // it to handle additional IPCs. SGTM as-is. Done. https://codereview.chromium.org/2363573002/diff/200001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2363573002/diff/200001/content/renderer/rende... content/renderer/render_view_impl.cc:1556: RenderThreadImpl::current_render_message_filter()->CreateNewWindow( On 2016/09/27 at 20:32:41, ncarter wrote: > Why use this global and getter? Is it just about avoiding exposing it on RenderThread (which is content/public)? > > It seems weird to call into RenderThreadImpl:: to get a value that's owned by the MockRenderThread. I'm not totally against this, but curious if there are any alternatives worth considering. Yeah, this was a semi-painful way to avoid exposing the mojom in content/public, which I do think is worth avoiding. I'm not crazy about this either, so I'm happy to entertain alternative ideas. One approach would be to have each RenderViewImpl cache its own RenderMessageFilterAssociatedPtr it gets from RenderThread's GetChannel()->GetRemoteAssociatedInterface(). It's a bit of extra code to have MockRenderThread implement its own mock Channel, but maybe that would be more flexible long-term. WDYT? (happy to prototype it quickly if that helps)
lgtm https://codereview.chromium.org/2363573002/diff/200001/content/browser/render... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/2363573002/diff/200001/content/browser/render... content/browser/renderer_host/render_message_filter.cc:301: callback.Run(std::move(reply)); On 2016/09/27 21:05:41, Ken Rockot wrote: > On 2016/09/27 at 20:32:41, ncarter wrote: > > Can we now guarantee that the task posted to the UI thread by > |CreateNewWindow| will run before any UI-thread destined mojo message that may > be sent from the child process after the reply message is received? > > Yes. Any interfaces (such as mojom::RenderMessageFilter) which are associated > with the IPC::Channel exhibit perfect ordering with respect to Channel on the > IO thread and ChannelProxy on the main thread. > > To put it another way, this conversion has zero impact on the ordering of the > message or its reply. Thanks, and awesome that we have this safety guarantee. I was particularly worried about the ordering once the next message gets mojofied, the one that shows the view, since it comes in (pretty weirdly!) on the opener's interface. https://codereview.chromium.org/2363573002/diff/200001/content/common/render_... File content/common/render_message_filter.mojom (right): https://codereview.chromium.org/2363573002/diff/200001/content/common/render_... content/common/render_message_filter.mojom:67: int32 main_frame_route_id; On 2016/09/27 21:05:41, Ken Rockot wrote: > On 2016/09/27 at 20:32:41, ncarter wrote: > > Long-term, do you think we'll eventually be returning FrameHost mojom > interfaces here (and other places where we mint routing ids)? > > Yes, that's the idea. I may even try to start pairing primordial interfaces with > routing IDs sooner rather than later if it means we can avoid routing ID params > in other message conversions (i.e. imagine if every message which introduced a > new routing ID to its receiver also provided an AssociatedInterfaceProviderPtr). > > Starting small though, I decided to stick with rote conversion here. Looking forward to seeing that happen. I still have on my TODO list to port CreateNewWindow from View to Frame (needed to fix bug 627852). CreateWindow logically belongs on frame, because window creation is an operation on the opener. Your CL here should make that process a lot easier; when I get back to that bug, I'll be sure to check in with you, to avoid conflicts. https://codereview.chromium.org/2363573002/diff/200001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2363573002/diff/200001/content/renderer/rende... content/renderer/render_view_impl.cc:1556: RenderThreadImpl::current_render_message_filter()->CreateNewWindow( On 2016/09/27 21:05:42, Ken Rockot wrote: > On 2016/09/27 at 20:32:41, ncarter wrote: > > Why use this global and getter? Is it just about avoiding exposing it on > RenderThread (which is content/public)? > > > > It seems weird to call into RenderThreadImpl:: to get a value that's owned by > the MockRenderThread. I'm not totally against this, but curious if there are any > alternatives worth considering. > > Yeah, this was a semi-painful way to avoid exposing the mojom in content/public, > which I do think is worth avoiding. I'm not crazy about this either, so I'm > happy to entertain alternative ideas. > > One approach would be to have each RenderViewImpl cache its own > RenderMessageFilterAssociatedPtr it gets from RenderThread's > GetChannel()->GetRemoteAssociatedInterface(). It's a bit of extra code to have > MockRenderThread implement its own mock Channel, but maybe that would be more > flexible long-term. WDYT? (happy to prototype it quickly if that helps) I'd prefer to keep the singleton interfaces looking like singletons; if we had pointers to them from each View, I can see getting confusion around whether the pointer would actually be the same for any two RenderViews -- that seems even more confusing if we apply that pattern to RenderFrameMessageFilter and RenderFrames. It looks like there is an existing, endemic problem; any code that calls RenderThreadImpl::current() is basically untestable from a MockRenderThread-based test. I don't see a clean way to do this other than adding an extra level of inheritance (e.g. a content-private RenderThreadBase, inheriting from RenderThread, which becomes the common base class of RenderThreadImpl and MockRenderThread), or a similar trick using implemented via composition. Curious if jam@ has thoughts here. Your approach makes sense for now. If we see this happening for other interfaces, we can add more structure.
https://codereview.chromium.org/2363573002/diff/200001/content/public/common/... File content/public/common/referrer_struct_traits.h (right): https://codereview.chromium.org/2363573002/diff/200001/content/public/common/... content/public/common/referrer_struct_traits.h:25: return data.ReadUrl(&out->url) && data.ReadPolicy(&out->policy); Let's out-of-line this (it's not clear to me how much code this ends up inlining, but it seems like it could be a reasonable amount) https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/ReferrerPolicyEnumTraits.h (right): https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/ReferrerPolicyEnumTraits.h:16: struct EnumTraits<::blink::mojom::ReferrerPolicy, ::blink::WebReferrerPolicy> { Is it possible to place this in namespace blink and rely on ADL to find this? Then we can lose the ::blink prefix here. https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/referrer.mojom (right): https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/referrer.mojom:27: struct Referrer { It's a little surprising to me that we define this in public/platform, when it's only created and consumed in content. Is there somewhere else it would end up being used in Blink? https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/web/WindowFeaturesStructTraits.h (right): https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/web/WindowFeaturesStructTraits.h:14: struct StructTraits<MojomDataViewType, ::blink::WebWindowFeatures> { How come we can't fully specialize this one? https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/web/WindowFeaturesStructTraits.h:36: out->x = data.x(); Nit: out-of-line Also, I think the Read() portion is actually unused, right? Perhaps I'm missing it somewhere, but I don't think we ever turn the mojo struct containing this back into a WebWindowFeatures struct.
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2363573002/diff/200001/content/public/common/... File content/public/common/referrer_struct_traits.h (right): https://codereview.chromium.org/2363573002/diff/200001/content/public/common/... content/public/common/referrer_struct_traits.h:25: return data.ReadUrl(&out->url) && data.ReadPolicy(&out->policy); On 2016/09/27 at 23:09:42, dcheng wrote: > Let's out-of-line this (it's not clear to me how much code this ends up inlining, but it seems like it could be a reasonable amount) Done https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/ReferrerPolicyEnumTraits.h (right): https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/ReferrerPolicyEnumTraits.h:16: struct EnumTraits<::blink::mojom::ReferrerPolicy, ::blink::WebReferrerPolicy> { On 2016/09/27 at 23:09:42, dcheng wrote: > Is it possible to place this in namespace blink and rely on ADL to find this? Then we can lose the ::blink prefix here. I'm not sure I understand what you mean. Do you mean change the bindings code to rely on ADL everywhere by referring to unqualified StructTraits<U, V>? That seems well beyond the scope of this CL, but we could consider it. I'd feel a little uncomfortable using a name as generic as StructTraits in that case too, so it might be better as MojoStructTraits, MojoEnumTraits, MojoArrayTraits, etc. If this is what you mean, we can always plan this work separately. I think it might be worth doing. https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/referrer.mojom (right): https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/referrer.mojom:27: struct Referrer { On 2016/09/27 at 23:09:42, dcheng wrote: > It's a little surprising to me that we define this in public/platform, when it's only created and consumed in content. Is there somewhere else it would end up being used in Blink? We could define it in content instead if you prefer. I guess my expectation is that many interfaces which use it will eventually be consumed directly from blink code, so this would very likely move to blink anyway. It feels like a web platform concept. https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/web/WindowFeaturesStructTraits.h (right): https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/web/WindowFeaturesStructTraits.h:14: struct StructTraits<MojomDataViewType, ::blink::WebWindowFeatures> { On 2016/09/27 at 23:09:42, dcheng wrote: > How come we can't fully specialize this one? We can, but Chromium and Blink cannot share generated struct definitions (it would be very complex to support), so if we fully specialized it, we'd then have to define the same traits specialization twice: one over blink::mojom::WindowFeatures and one over blink::mojom::blink::WindowFeatures. Note that both Chromium and Blink typemaps map to blink::WebWindowFeatures. This avoids explicit code duplication. https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/web/WindowFeaturesStructTraits.h:36: out->x = data.x(); On 2016/09/27 at 23:09:42, dcheng wrote: > Nit: out-of-line > Can't move it out-of-line unless we fully specialize and duplicate the definition for Blink and Chromium. Neither Chromium nor Blink is allowed to include the other's generated headers, so a single cpp cannot not define full specializations of Read for both of them. We could define the partial specialization in a private header to reduce code duplication and then instantiate it in separate h/cc for separate Chromium and Blink typemaps to the same type, but this seems rather noisy and it's not clear to me that it's worth doing. > Also, I think the Read() portion is actually unused, right? Perhaps I'm missing it somewhere, but I don't think we ever turn the mojo struct containing this back into a WebWindowFeatures struct. We definitely use it. There are references to blink::WebWindowFeatures all around browser code, for better or worse. RenderViewHostImpl receives a CreateNewWindowParams, and its |features| field is typemapped the same as the sending side in blink.
https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/ReferrerPolicyEnumTraits.h (right): https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/ReferrerPolicyEnumTraits.h:16: struct EnumTraits<::blink::mojom::ReferrerPolicy, ::blink::WebReferrerPolicy> { On 2016/09/28 00:20:05, Ken Rockot wrote: > On 2016/09/27 at 23:09:42, dcheng wrote: > > Is it possible to place this in namespace blink and rely on ADL to find this? > Then we can lose the ::blink prefix here. > > I'm not sure I understand what you mean. Do you mean change the bindings code to > rely on ADL everywhere by referring to unqualified StructTraits<U, V>? That > seems well beyond the scope of this CL, but we could consider it. > > I'd feel a little uncomfortable using a name as generic as StructTraits in that > case too, so it might be better as MojoStructTraits, MojoEnumTraits, > MojoArrayTraits, etc. > > If this is what you mean, we can always plan this work separately. I think it > might be worth doing. Sorry, to clarify, can we do: namespace blink { template<> struct EnumTraits<mojom::ReferrerPolicy, WebReferrerPolicy> { // ... stuff }; https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/referrer.mojom (right): https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/referrer.mojom:27: struct Referrer { On 2016/09/28 00:20:05, Ken Rockot wrote: > On 2016/09/27 at 23:09:42, dcheng wrote: > > It's a little surprising to me that we define this in public/platform, when > it's only created and consumed in content. Is there somewhere else it would end > up being used in Blink? > > We could define it in content instead if you prefer. I guess my expectation is > that many interfaces which use it will eventually be consumed directly from > blink code, so this would very likely move to blink anyway. It feels like a web > platform concept. Perhaps, but it would be easy enough to move it into the appropriate layer then. https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/web/WindowFeaturesStructTraits.h (right): https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/web/WindowFeaturesStructTraits.h:14: struct StructTraits<MojomDataViewType, ::blink::WebWindowFeatures> { On 2016/09/28 00:20:05, Ken Rockot wrote: > On 2016/09/27 at 23:09:42, dcheng wrote: > > How come we can't fully specialize this one? > > We can, but Chromium and Blink cannot share generated struct definitions (it > would be very complex to support), so if we fully specialized it, we'd then have > to define the same traits specialization twice: one over > blink::mojom::WindowFeatures and one over blink::mojom::blink::WindowFeatures. > > Note that both Chromium and Blink typemaps map to blink::WebWindowFeatures. > > This avoids explicit code duplication. I'm going to need to think about this, but I thought this was the whole point of https://crbug.com/632061
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
[email protected] changed reviewers: + [email protected]
https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/ReferrerPolicyEnumTraits.h (right): https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/ReferrerPolicyEnumTraits.h:16: struct EnumTraits<::blink::mojom::ReferrerPolicy, ::blink::WebReferrerPolicy> { On 2016/09/28 at 00:30:51, dcheng wrote: > On 2016/09/28 00:20:05, Ken Rockot wrote: > > On 2016/09/27 at 23:09:42, dcheng wrote: > > > Is it possible to place this in namespace blink and rely on ADL to find this? > > Then we can lose the ::blink prefix here. > > > > I'm not sure I understand what you mean. Do you mean change the bindings code to > > rely on ADL everywhere by referring to unqualified StructTraits<U, V>? That > > seems well beyond the scope of this CL, but we could consider it. > > > > I'd feel a little uncomfortable using a name as generic as StructTraits in that > > case too, so it might be better as MojoStructTraits, MojoEnumTraits, > > MojoArrayTraits, etc. > > > > If this is what you mean, we can always plan this work separately. I think it > > might be worth doing. > > Sorry, to clarify, can we do: > > namespace blink { > > template<> > struct EnumTraits<mojom::ReferrerPolicy, WebReferrerPolicy> { > // ... stuff > }; As discussed off thread, this is a no-go since ADL only applies to function calls, not struct references. https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/referrer.mojom (right): https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/referrer.mojom:27: struct Referrer { On 2016/09/28 at 00:30:51, dcheng wrote: > On 2016/09/28 00:20:05, Ken Rockot wrote: > > On 2016/09/27 at 23:09:42, dcheng wrote: > > > It's a little surprising to me that we define this in public/platform, when > > it's only created and consumed in content. Is there somewhere else it would end > > up being used in Blink? > > > > We could define it in content instead if you prefer. I guess my expectation is > > that many interfaces which use it will eventually be consumed directly from > > blink code, so this would very likely move to blink anyway. It feels like a web > > platform concept. > > Perhaps, but it would be easy enough to move it into the appropriate layer then. As discussed off thread, leaving this here for now since it's kind of weird no matter where it goes (splitting mojom Referrer from mojom ReferrerPolicy is weird, but splitting mojom ReferrerPolicy for WebReferrerPolicy is also weird). +esprehn for opinion here https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/web/WindowFeaturesStructTraits.h (right): https://codereview.chromium.org/2363573002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/web/WindowFeaturesStructTraits.h:14: struct StructTraits<MojomDataViewType, ::blink::WebWindowFeatures> { On 2016/09/28 at 00:30:51, dcheng wrote: > On 2016/09/28 00:20:05, Ken Rockot wrote: > > On 2016/09/27 at 23:09:42, dcheng wrote: > > > How come we can't fully specialize this one? > > > > We can, but Chromium and Blink cannot share generated struct definitions (it > > would be very complex to support), so if we fully specialized it, we'd then have > > to define the same traits specialization twice: one over > > blink::mojom::WindowFeatures and one over blink::mojom::blink::WindowFeatures. > > > > Note that both Chromium and Blink typemaps map to blink::WebWindowFeatures. > > > > This avoids explicit code duplication. > > I'm going to need to think about this, but I thought this was the whole point of https://crbug.com/632061 Indeed this was exactly the point of that work! Done.
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hmm, adding mojoms to the web/ layer is something new for sure, I think it's okay, but I'm also trying to make the web layer smaller in general and get as much of the code as possible moved out of there.
On Sep 27, 2016 9:14 PM, <[email protected]> wrote: > > Hmm, adding mojoms to the web/ layer is something new for sure, I think it's > okay, but I'm also trying to make the web layer smaller in general and get as > much of the code as possible moved out of there. Sure, that makes sense. I would see any mojom introduced here as a transitional phase until it can move to platform once content is no longer using the corresponding web/ type. > > https://codereview.chromium.org/2363573002/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
On Sep 27, 2016 9:14 PM, <[email protected]> wrote: > > Hmm, adding mojoms to the web/ layer is something new for sure, I think it's > okay, but I'm also trying to make the web layer smaller in general and get as > much of the code as possible moved out of there. Sure, that makes sense. I would see any mojom introduced here as a transitional phase until it can move to platform once content is no longer using the corresponding web/ type. > > https://codereview.chromium.org/2363573002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: This issue passed the CQ dry run.
So are we OK with landing this as-is?
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected], [email protected] Link to the patchset: https://codereview.chromium.org/2363573002/#ps280001 (title: ".")
The CQ bit was unchecked by [email protected]
mojo lgtm
And blink?
mojo *and* blink lgtm sorry =P
Thanks :p On Sep 28, 2016 4:05 PM, <[email protected]> wrote: > mojo *and* blink lgtm > > sorry =P > > https://codereview.chromium.org/2363573002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
Thanks :p On Sep 28, 2016 4:05 PM, <[email protected]> wrote: > mojo *and* blink lgtm > > sorry =P > > https://codereview.chromium.org/2363573002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
The CQ bit was checked by [email protected]
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType an alias for the new content::mojom::WindowContainerType. Some transitional public constants have been added to satisfy existing references to the old enum values. This will be removed in future CLs. - Adds a mojom definition for blink::mojom::WindowFeatures, corresponding to blink::WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds a blink::mojom::Referrer struct analogous to content::Renderer. Chromium bindings typemap this new mojom struct to content::Referrer. - Adds a mojom blink::mojom::ReferrerPolicy enum with a typemap to blink::WebReferrerPolicy, shared by Chromium and Blink bindings. This can be de-duplicated in future CLs. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType an alias for the new content::mojom::WindowContainerType. Some transitional public constants have been added to satisfy existing references to the old enum values. This will be removed in future CLs. - Adds a mojom definition for blink::mojom::WindowFeatures, corresponding to blink::WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds a blink::mojom::Referrer struct analogous to content::Renderer. Chromium bindings typemap this new mojom struct to content::Referrer. - Adds a mojom blink::mojom::ReferrerPolicy enum with a typemap to blink::WebReferrerPolicy, shared by Chromium and Blink bindings. This can be de-duplicated in future CLs. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType an alias for the new content::mojom::WindowContainerType. Some transitional public constants have been added to satisfy existing references to the old enum values. This will be removed in future CLs. - Adds a mojom definition for blink::mojom::WindowFeatures, corresponding to blink::WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds a blink::mojom::Referrer struct analogous to content::Renderer. Chromium bindings typemap this new mojom struct to content::Referrer. - Adds a mojom blink::mojom::ReferrerPolicy enum with a typemap to blink::WebReferrerPolicy, shared by Chromium and Blink bindings. This can be de-duplicated in future CLs. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move ViewHostMsg_CreateWindow to mojom Converts this message, addressing several of its dependencies in the process: - Makes content::WindowContainerType an alias for the new content::mojom::WindowContainerType. Some transitional public constants have been added to satisfy existing references to the old enum values. This will be removed in future CLs. - Adds a mojom definition for blink::mojom::WindowFeatures, corresponding to blink::WebWindowFeatures. A typemap is added between these types and is shared by both Blink and Chromium bindings configurations. - Adds a blink::mojom::Referrer struct analogous to content::Renderer. Chromium bindings typemap this new mojom struct to content::Referrer. - Adds a mojom blink::mojom::ReferrerPolicy enum with a typemap to blink::WebReferrerPolicy, shared by Chromium and Blink bindings. This can be de-duplicated in future CLs. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/5c478a7b04b55630c5c491d4d1d2863e965cfa32 Cr-Commit-Position: refs/heads/master@{#421674} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5c478a7b04b55630c5c491d4d1d2863e965cfa32 Cr-Commit-Position: refs/heads/master@{#421674} |