Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2724933002:
Convert SetVisuallyDeemphasized IPC to mojo (Closed)
|
Created:
3 years, 9 months ago by catmullings Modified:
3 years, 9 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert SetVisuallyDeemphasized IPC to mojo
Convert SetVisuallyDeemphasized to a mojo message while moving its
definition and declaration out of chrome/ code into extensions/ code.
BUG=697613
Review-Url: https://codereview.chromium.org/2724933002
Cr-Commit-Position: refs/heads/master@{#455788}
Committed: https://chromium.googlesource.com/chromium/src/+/cf1efee24a5d9363e8bed7d45950ead20eff2cda
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addressed patch 2 comments #
Total comments: 6
Patch Set 3 : Addressed Devlin's comments #
Total comments: 6
Patch Set 4 : Addressed Ken's comments #Messages
Total messages: 63 (45 generated)
Description was changed from ========== Convert SetVisuallyDeemphasized IPC to mojo Convert SetVisuallyDeemphasized to a mojo message while moving its definition and declaration out of chrome/ code into extensions/ code. BUG= ========== to ========== Convert SetVisuallyDeemphasized IPC to mojo Convert SetVisuallyDeemphasized to a mojo message while moving its definition and declaration out of chrome/ code into extensions/ code. BUG=577685 ==========
The CQ bit was checked by catmullings@chromium.org 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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by catmullings@chromium.org 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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Convert SetVisuallyDeemphasized IPC to mojo Convert SetVisuallyDeemphasized to a mojo message while moving its definition and declaration out of chrome/ code into extensions/ code. BUG=577685 ========== to ========== Convert SetVisuallyDeemphasized IPC to mojo Convert SetVisuallyDeemphasized to a mojo message while moving its definition and declaration out of chrome/ code into extensions/ code. BUG=697613 ==========
The CQ bit was checked by catmullings@chromium.org 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 checked by catmullings@chromium.org 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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by catmullings@chromium.org 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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
catmullings@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Nice! Some dependency issues, and a few other nits, but overall this looks pretty good! :) https://codereview.chromium.org/2724933002/diff/60001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2724933002/diff/60001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:1184: "//extensions/renderer:mojo", This is a layering violation - chrome/browser cannot depend on renderer/ knowledge. https://codereview.chromium.org/2724933002/diff/60001/chrome/browser/ui/DEPS File chrome/browser/ui/DEPS (right): https://codereview.chromium.org/2724933002/diff/60001/chrome/browser/ui/DEPS#... chrome/browser/ui/DEPS:10: "+extensions/renderer/mojo", ditto https://codereview.chromium.org/2724933002/diff/60001/chrome/browser/ui/apps/... File chrome/browser/ui/apps/chrome_app_delegate.cc (right): https://codereview.chromium.org/2724933002/diff/60001/chrome/browser/ui/apps/... chrome/browser/ui/apps/chrome_app_delegate.cc:313: &extensions_frame_observer_); This re-gets the interface each time we send the message. Seems like we should be either caching the interface in a local variable, rather than a member variable, or else checking whether or not its initialized before trying to get it. I think the former makes more sense here, since it appears the ChromeAppDelegate can be used across multiple WebContents/RenderFrameHosts. https://codereview.chromium.org/2724933002/diff/60001/extensions/renderer/ext... File extensions/renderer/extensions_render_frame_observer.h (right): https://codereview.chromium.org/2724933002/diff/60001/extensions/renderer/ext... extensions/renderer/extensions_render_frame_observer.h:23: void Create(mojom::FrameObserverRequest request); "Create()" is a little strange here, since usually Foo::Create() would be a method to create a new Foo. Maybe AssociateBinding()? Also, it looks like maybe this method can be private? https://codereview.chromium.org/2724933002/diff/60001/extensions/renderer/ext... extensions/renderer/extensions_render_frame_observer.h:30: #if BUILDFLAG(ENABLE_EXTENSIONS) Since we're moving this to an extensions/ file, we can know that extensions are enabled. https://codereview.chromium.org/2724933002/diff/60001/extensions/renderer/moj... File extensions/renderer/mojo/frame_observer.mojom (right): https://codereview.chromium.org/2724933002/diff/60001/extensions/renderer/moj... extensions/renderer/mojo/frame_observer.mojom:7: interface FrameObserver { This isn't really a FrameObserver, per se (beyond the fact that the renderer-side binding for it hangs off a RenderFrameObserver). Since this message is for app windows, maybe something like AppWindowHandler, or similar? https://codereview.chromium.org/2724933002/diff/60001/extensions/renderer/moj... extensions/renderer/mojo/frame_observer.mojom:8: SetVisuallyDeemphasized(bool deemphasized); Since this is a shared topic between browser and renderer, we should put it in extensions/common (similarly to how the previous message was in chrome/common). That will solve the layering violations. :)
The CQ bit was checked by catmullings@chromium.org 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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2724933002/diff/60001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2724933002/diff/60001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:1184: "//extensions/renderer:mojo", On 2017/03/04 02:19:41, Devlin wrote: > This is a layering violation - chrome/browser cannot depend on renderer/ > knowledge. Done. https://codereview.chromium.org/2724933002/diff/60001/chrome/browser/ui/DEPS File chrome/browser/ui/DEPS (right): https://codereview.chromium.org/2724933002/diff/60001/chrome/browser/ui/DEPS#... chrome/browser/ui/DEPS:10: "+extensions/renderer/mojo", On 2017/03/04 02:19:41, Devlin wrote: > ditto Done. https://codereview.chromium.org/2724933002/diff/60001/chrome/browser/ui/apps/... File chrome/browser/ui/apps/chrome_app_delegate.cc (right): https://codereview.chromium.org/2724933002/diff/60001/chrome/browser/ui/apps/... chrome/browser/ui/apps/chrome_app_delegate.cc:313: &extensions_frame_observer_); On 2017/03/04 02:19:41, Devlin wrote: > This re-gets the interface each time we send the message. Seems like we should > be either caching the interface in a local variable, rather than a member > variable, or else checking whether or not its initialized before trying to get > it. I think the former makes more sense here, since it appears the > ChromeAppDelegate can be used across multiple WebContents/RenderFrameHosts. Done. https://codereview.chromium.org/2724933002/diff/60001/extensions/renderer/ext... File extensions/renderer/extensions_render_frame_observer.h (right): https://codereview.chromium.org/2724933002/diff/60001/extensions/renderer/ext... extensions/renderer/extensions_render_frame_observer.h:23: void Create(mojom::FrameObserverRequest request); On 2017/03/04 02:19:41, Devlin wrote: > "Create()" is a little strange here, since usually Foo::Create() would be a > method to create a new Foo. Maybe AssociateBinding()? Also, it looks like > maybe this method can be private? Done. https://codereview.chromium.org/2724933002/diff/60001/extensions/renderer/ext... extensions/renderer/extensions_render_frame_observer.h:30: #if BUILDFLAG(ENABLE_EXTENSIONS) On 2017/03/04 02:19:41, Devlin wrote: > Since we're moving this to an extensions/ file, we can know that extensions are > enabled. Done. https://codereview.chromium.org/2724933002/diff/60001/extensions/renderer/moj... File extensions/renderer/mojo/frame_observer.mojom (right): https://codereview.chromium.org/2724933002/diff/60001/extensions/renderer/moj... extensions/renderer/mojo/frame_observer.mojom:7: interface FrameObserver { On 2017/03/04 02:19:41, Devlin wrote: > This isn't really a FrameObserver, per se (beyond the fact that the > renderer-side binding for it hangs off a RenderFrameObserver). Since this > message is for app windows, maybe something like AppWindowHandler, or similar? Done. https://codereview.chromium.org/2724933002/diff/60001/extensions/renderer/moj... extensions/renderer/mojo/frame_observer.mojom:8: SetVisuallyDeemphasized(bool deemphasized); On 2017/03/04 02:19:41, Devlin wrote: > Since this is a shared topic between browser and renderer, we should put it in > extensions/common (similarly to how the previous message was in chrome/common). > That will solve the layering violations. :) Done.
rdevlin.cronin@chromium.org changed reviewers: + rockot@chromium.org
Nice! A few last nits, but otherwise lgtm. +Ken for mojo expertise. https://codereview.chromium.org/2724933002/diff/80001/chrome/browser/ui/DEPS File chrome/browser/ui/DEPS (right): https://codereview.chromium.org/2724933002/diff/80001/chrome/browser/ui/DEPS#... chrome/browser/ui/DEPS:10: "+extensions/common/mojo", Do we need this? https://codereview.chromium.org/2724933002/diff/80001/extensions/renderer/ext... File extensions/renderer/extensions_render_frame_observer.cc (right): https://codereview.chromium.org/2724933002/diff/80001/extensions/renderer/ext... extensions/renderer/extensions_render_frame_observer.cc:109: SK_ColorTRANSPARENT); I realize this was just copy-paste, so not your code, but while you're here... :) Let's make this a little simpler by doing SkColor color = deemphasized ? SkColorSetARGB(178, 0, 0, 0) : SK_ColorTRANSPARENT; render_frame()->GetRenderView()->GetWebView()->SetPageOverlayColor(color); https://codereview.chromium.org/2724933002/diff/80001/extensions/renderer/ext... File extensions/renderer/extensions_render_frame_observer.h (right): https://codereview.chromium.org/2724933002/diff/80001/extensions/renderer/ext... extensions/renderer/extensions_render_frame_observer.h:13: #include "extensions/features/features.h" Needed?
The CQ bit was checked by catmullings@chromium.org 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 #3 (id:100001) has been deleted
The CQ bit was checked by catmullings@chromium.org 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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Addressed Devlin's comments. https://codereview.chromium.org/2724933002/diff/80001/chrome/browser/ui/DEPS File chrome/browser/ui/DEPS (right): https://codereview.chromium.org/2724933002/diff/80001/chrome/browser/ui/DEPS#... chrome/browser/ui/DEPS:10: "+extensions/common/mojo", On 2017/03/07 19:39:05, Devlin wrote: > Do we need this? Done. https://codereview.chromium.org/2724933002/diff/80001/extensions/renderer/ext... File extensions/renderer/extensions_render_frame_observer.cc (right): https://codereview.chromium.org/2724933002/diff/80001/extensions/renderer/ext... extensions/renderer/extensions_render_frame_observer.cc:109: SK_ColorTRANSPARENT); On 2017/03/07 19:39:05, Devlin wrote: > I realize this was just copy-paste, so not your code, but while you're here... > :) > > Let's make this a little simpler by doing > SkColor color = > deemphasized ? SkColorSetARGB(178, 0, 0, 0) : SK_ColorTRANSPARENT; > render_frame()->GetRenderView()->GetWebView()->SetPageOverlayColor(color); Done. https://codereview.chromium.org/2724933002/diff/80001/extensions/renderer/ext... File extensions/renderer/extensions_render_frame_observer.h (right): https://codereview.chromium.org/2724933002/diff/80001/extensions/renderer/ext... extensions/renderer/extensions_render_frame_observer.h:13: #include "extensions/features/features.h" On 2017/03/07 19:39:05, Devlin wrote: > Needed? Done.
lgtm https://codereview.chromium.org/2724933002/diff/120001/extensions/common/mojo... File extensions/common/mojo/app_window_handler.mojom (right): https://codereview.chromium.org/2724933002/diff/120001/extensions/common/mojo... extensions/common/mojo/app_window_handler.mojom:7: interface AppWindowHandler { nit: Maybe "Handler" is redundant for an interface name. Could it just be the "AppWindow" interface? https://codereview.chromium.org/2724933002/diff/120001/extensions/renderer/ex... File extensions/renderer/extensions_render_frame_observer.cc (right): https://codereview.chromium.org/2724933002/diff/120001/extensions/renderer/ex... extensions/renderer/extensions_render_frame_observer.cc:92: void ExtensionsRenderFrameObserver::AssociateBindings( nit: This is a pretty weird method name for what it does. Maybe just call it BindAppWindowHandler or BindAppWindowHandlerRequest? https://codereview.chromium.org/2724933002/diff/120001/extensions/renderer/ex... File extensions/renderer/extensions_render_frame_observer.h (right): https://codereview.chromium.org/2724933002/diff/120001/extensions/renderer/ex... extensions/renderer/extensions_render_frame_observer.h:42: bool webview_visually_deemphasized_; optional (personal preference) nit: you could initialize this here instead of in the constructor definition.
The CQ bit was checked by catmullings@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, rockot@chromium.org Link to the patchset: https://codereview.chromium.org/2724933002/#ps140001 (title: "Addressed Ken's comments")
Addressed Ken's comments. https://codereview.chromium.org/2724933002/diff/120001/extensions/common/mojo... File extensions/common/mojo/app_window_handler.mojom (right): https://codereview.chromium.org/2724933002/diff/120001/extensions/common/mojo... extensions/common/mojo/app_window_handler.mojom:7: interface AppWindowHandler { On 2017/03/08 17:31:38, Ken Rockot wrote: > nit: Maybe "Handler" is redundant for an interface name. Could it just be the > "AppWindow" interface? Done. https://codereview.chromium.org/2724933002/diff/120001/extensions/renderer/ex... File extensions/renderer/extensions_render_frame_observer.cc (right): https://codereview.chromium.org/2724933002/diff/120001/extensions/renderer/ex... extensions/renderer/extensions_render_frame_observer.cc:92: void ExtensionsRenderFrameObserver::AssociateBindings( On 2017/03/08 17:31:38, Ken Rockot wrote: > nit: This is a pretty weird method name for what it does. Maybe just call it > BindAppWindowHandler or BindAppWindowHandlerRequest? Done. https://codereview.chromium.org/2724933002/diff/120001/extensions/renderer/ex... File extensions/renderer/extensions_render_frame_observer.h (right): https://codereview.chromium.org/2724933002/diff/120001/extensions/renderer/ex... extensions/renderer/extensions_render_frame_observer.h:42: bool webview_visually_deemphasized_; On 2017/03/08 17:31:38, Ken Rockot wrote: > optional (personal preference) nit: you could initialize this here instead of in > the constructor definition. I'll leave this as it is.
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 commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
catmullings@chromium.org changed reviewers: + benwells@chromium.org, meacer@chromium.org, wfh@chromium.org
PTAL request for OWNERS of the following files: +benwells for chrome/browser/ui/apps/chrome_app_delegate.cc +wfh for chrome/renderer/chrome_render_view_observer.* +meacer for IPC Security on extensions/common/mojo/app_window.mojom, chrome/common/extensions/chrome_extension_messages.h
hi I think you'll need a chrome/ owner for chrome/renderer/chrome_render_view_observer.* not me... so I'll remove, let me know if I have this wrong.
wfh@chromium.org changed reviewers: - wfh@chromium.org
Ipc lgtm
catmullings@chromium.org changed reviewers: + jochen@chromium.org
+jochen as chrome/ OWNER, request to review chrome/renderer/chrome_render_view_observer.*
c/b/ui/apps lgtm
c/b/ui/apps lgtm
lgtm
The CQ bit was checked by catmullings@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489081312631040,
"parent_rev": "31b966ba7c5468ada998bb46e605a4d733b69202", "commit_rev":
"cf1efee24a5d9363e8bed7d45950ead20eff2cda"}
Message was sent while issue was closed.
Description was changed from ========== Convert SetVisuallyDeemphasized IPC to mojo Convert SetVisuallyDeemphasized to a mojo message while moving its definition and declaration out of chrome/ code into extensions/ code. BUG=697613 ========== to ========== Convert SetVisuallyDeemphasized IPC to mojo Convert SetVisuallyDeemphasized to a mojo message while moving its definition and declaration out of chrome/ code into extensions/ code. BUG=697613 Review-Url: https://codereview.chromium.org/2724933002 Cr-Commit-Position: refs/heads/master@{#455788} Committed: https://chromium.googlesource.com/chromium/src/+/cf1efee24a5d9363e8bed7d45950... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001) as https://chromium.googlesource.com/chromium/src/+/cf1efee24a5d9363e8bed7d45950... |
