Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2436903003:
Extract ArcSupportMessageHost. (Closed)
|
Created:
4 years, 2 months ago by hidehiko Modified:
4 years, 1 month ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, extensions-reviews_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, khmel+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, davemoore+watch_chromium.org, khmel Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtract ArcSupportMessageHost.
Currently ArcSupportHost is the ARC's implementation of
NativeMessageHost. Because of the system structure, the instance
is created and managed by the module outside of ARC.
This CL extracts the implementation of that part into
ArcSupportMessageHost so that the ArcSupportHost, which will
be the main interface to show UI things for ARC, can be managed
along with the ARC's life-time.
There is a plan to extract non authorization related part from
ArcAuthSerivce. Meanwhile, the instance is managed by
ArcAuthService, temporarily.
BUG=657687, 636218, 633258
, b/31079732
TEST=Ran on test device. Ran trybots.
Committed: https://crrev.com/e0eb7942096fb4945f5d5d3ed23632148d5348bd
Cr-Commit-Position: refs/heads/master@{#427277}
Patch Set 1 #Patch Set 2 : Fix tests #
Total comments: 33
Patch Set 3 : rebase #Patch Set 4 : Address comments. #
Total comments: 5
Patch Set 5 : Address comments. #Patch Set 6 : Rebase #Messages
Total messages: 38 (19 generated)
The CQ bit was checked by hidehiko@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_...)
The CQ bit was checked by hidehiko@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...
hidehiko@chromium.org changed reviewers: + asargent@chromium.org, lhchavez@chromium.org
PTAL. lhchavez@: All. asargent@: chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc as an OWNER. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:94: if (g_browser_process->local_state()) { Note: No service is running on unittest, if stmt is workaround. This will be cleaned, when the logic is extracted. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:132: {"com.google.chrome.test.echo", // ScopedTestNativeMessagingHost::kHostName Note: this is the result of "git cl format".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
CL description typo: "managed by the moudle" - I think you probably meant "managed by the module" Also, a question: why is ArcSupportMessageHost defined in a "/ui/" directory? It seems like the NativeMessageHost interface deals mainly with details of interprocess communication and doesn't immediately make me think of user-visible UI code, so seeing that in the path was unexpected.
Description was changed from ========== Extract ArcSupportMessageHost. Currently ArcSupportHost is the ARC's implementation of NativeMessageHost. Because of the system structure, the instance is created and managed by the moudle outside of ARC. This CL extracts the implementation of that part into ArcSupportMessageHost so that the ArcSupportHost, which will be the main interface to show UI things for ARC, can be managed along with the ARC's life-time. There is a plan to extract non authorization related part from ArcAuthSerivce. Meanwhile, the instance is managed by ArcAuthService, temporarily. BUG=657687, 636218, 633258, b/31079732 TEST=Ran on test device. Ran trybots. ========== to ========== Extract ArcSupportMessageHost. Currently ArcSupportHost is the ARC's implementation of NativeMessageHost. Because of the system structure, the instance is created and managed by the module outside of ARC. This CL extracts the implementation of that part into ArcSupportMessageHost so that the ArcSupportHost, which will be the main interface to show UI things for ARC, can be managed along with the ARC's life-time. There is a plan to extract non authorization related part from ArcAuthSerivce. Meanwhile, the instance is managed by ArcAuthService, temporarily. BUG=657687, 636218, 633258, b/31079732 TEST=Ran on test device. Ran trybots. ==========
On 2016/10/20 19:42:37, Antony Sargent wrote: > CL description typo: "managed by the moudle" - I think you probably meant > "managed by the module" Thank you. Fixed. > > Also, a question: why is ArcSupportMessageHost defined in a "/ui/" directory? It > seems like the NativeMessageHost interface deals mainly with details of > interprocess communication and doesn't immediately make me think of user-visible > UI code, so seeing that in the path was unexpected. Good point. Luis, how about then /arc_support/ directory? I followed your subdirectory CL's name, but /ui/ is now renamed to intent_helper so I think there is no directory with suitable name for now? WDYT?
first round https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:94: if (g_browser_process->local_state()) { On 2016/10/20 18:36:17, hidehiko wrote: > Note: No service is running on unittest, if stmt is workaround. > This will be cleaned, when the logic is extracted. Can you add a TODO? https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:127: // delivered to here. nit: s/delivered to here/delivered here/ https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:143: void ArcSupportHost::SetMessageHost(arc::ArcSupportMessageHost* message_host) { if (message_host_ == message_host) return; for symmetry? https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:172: display::Screen::GetScreen()->RemoveObserver(this); DCHECK(message_host_); ? https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/ui/arc_support_message_host.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/ui/arc_support_message_host.cc:53: if (auth_service) This looks racy. If |auth_service| is null, then ArcAuthService will never be able to get the MessageHost. What guarantees do we have that this will never happen? Please comment about that and change this if to a DCHECK.
khmel@chromium.org changed reviewers: + khmel@chromium.org
Nice work! my 5 cents. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:472: support_host_.reset(new ArcSupportHost()); I would recommend to create it on demand. In most cases, UI is not shown. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:679: if (support_host_) In my previous comment I recommend to create host on demand. If this is not possible, IMHO this check is redundant and we can change to DCHECK() https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:134: return; IIUC this should never happen. May be LOG(ERROR)? https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/ui/arc_support_message_host.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/ui/arc_support_message_host.cc:45: observer_ = observer; nit: DCHECK(!observer_), meaning we cannot override previous observer https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/ui/arc_support_message_host.cc:53: if (auth_service) On 2016/10/21 02:24:08, Luis Héctor Chávez wrote: > This looks racy. If |auth_service| is null, then ArcAuthService will never be > able to get the MessageHost. What guarantees do we have that this will never > happen? Please comment about that and change this if to a DCHECK. It seems ArcAuthService::Get() cannot be nullptr in this case. DCHECK(auth_service)? Previous realization had DCHECK() https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/ui/arc_support_message_host.h (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/ui/arc_support_message_host.h:40: // Register (or unregister if nullptr) the observer. Currently this class nit: Register(s) unregister(s) https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/ui/arc_support_message_host.h:48: // Overrides NativeMessageHost: nit: not sure, but // extensions::NativeMessageHost: ?
The CQ bit was checked by hidehiko@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...
Thank you for review. PTAL. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:472: support_host_.reset(new ArcSupportHost()); On 2016/10/21 03:34:49, khmel wrote: > I would recommend to create it on demand. In most cases, UI is not shown. Ah, yes it can be in theory, but the current code complexity is making it difficult. Specifically, I'm moving more UI related code to ArcSupportHost so that the timing gets more and more complicated. So, let me keep it as is for now, and let's revisit here after more refactoring, so that life-time management can be easier, I think. Added TODO. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:679: if (support_host_) On 2016/10/21 03:34:49, khmel wrote: > In my previous comment I recommend to create host on demand. > If this is not possible, IMHO this check is redundant and we can change to > DCHECK() Unfortunately, it cannot, specifically for CloseUI(), which is called in Shutdown() called in OnPrimaryUserProfilePrepared(). I do want to be consistent SetUIPage() and CloseUI(), so let me keep this as is now. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:94: if (g_browser_process->local_state()) { On 2016/10/21 02:24:08, Luis Héctor Chávez wrote: > On 2016/10/20 18:36:17, hidehiko wrote: > > Note: No service is running on unittest, if stmt is workaround. > > This will be cleaned, when the logic is extracted. > > Can you add a TODO? Done. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:134: return; On 2016/10/21 03:34:49, khmel wrote: > IIUC this should never happen. May be LOG(ERROR)? This happens. Because ArcSupportMessageHost creation is async, when this is called, it is no guarantee that message_host_ is ready. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:143: void ArcSupportHost::SetMessageHost(arc::ArcSupportMessageHost* message_host) { On 2016/10/21 02:24:07, Luis Héctor Chávez wrote: > if (message_host_ == message_host) > return; > > for symmetry? Done. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:172: display::Screen::GetScreen()->RemoveObserver(this); On 2016/10/21 02:24:07, Luis Héctor Chávez wrote: > DCHECK(message_host_); ? Done. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/ui/arc_support_message_host.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/ui/arc_support_message_host.cc:45: observer_ = observer; On 2016/10/21 03:34:49, khmel wrote: > nit: DCHECK(!observer_), meaning we cannot override previous observer This also can be used to detach the observer. Added DCHECK but with different condition. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/ui/arc_support_message_host.cc:53: if (auth_service) On 2016/10/21 03:34:49, khmel wrote: > On 2016/10/21 02:24:08, Luis Héctor Chávez wrote: > > This looks racy. If |auth_service| is null, then ArcAuthService will never be > > able to get the MessageHost. What guarantees do we have that this will never > > happen? Please comment about that and change this if to a DCHECK. > > It seems ArcAuthService::Get() cannot be nullptr in this case. > DCHECK(auth_service)? > Previous realization had DCHECK() Indeed, here DCHECK is appropriate, but in dtor, if check is needed. Added comment in dtor, and replaced this by DCHECK. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/ui/arc_support_message_host.h (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/ui/arc_support_message_host.h:40: // Register (or unregister if nullptr) the observer. Currently this class On 2016/10/21 03:34:49, khmel wrote: > nit: Register(s) unregister(s) Good catch. Done in a new file. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/ui/arc_support_message_host.h:48: // Overrides NativeMessageHost: On 2016/10/21 03:34:49, khmel wrote: > nit: not sure, but > // extensions::NativeMessageHost: > ? Hmm. There seem various patterns... // $classname: // Overrides $classname: // $classname overrides: // $classname override: // Overridden $classname: etc. How about standardize "// $classname overrides:" then? I haven't touched to other ones, but will do in a separate CL.
lgtm, defer to khmel@. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/ui/arc_support_message_host.h (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/ui/arc_support_message_host.h:48: // Overrides NativeMessageHost: On 2016/10/21 07:40:27, hidehiko wrote: > On 2016/10/21 03:34:49, khmel wrote: > > nit: not sure, but > > // extensions::NativeMessageHost: > > ? > > Hmm. There seem various patterns... > // $classname: > // Overrides $classname: > // $classname overrides: > // $classname override: > // Overridden $classname: > > etc. > How about standardize "// $classname overrides:" then? I haven't touched to > other ones, but will do in a separate CL. > I'd consult the chromium-dev mailing list if this has already been discussed. Otherwise, use the good old "whichever pattern has more matches with ag | wc -l trick". https://codereview.chromium.org/2436903003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2436903003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/BUILD.gn:241: "arc/extensions/arc_support_message_host.h", nit: s/extensions/extension/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for review. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/ui/arc_support_message_host.h (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/ui/arc_support_message_host.h:48: // Overrides NativeMessageHost: On 2016/10/21 07:50:48, Luis Héctor Chávez wrote: > On 2016/10/21 07:40:27, hidehiko wrote: > > On 2016/10/21 03:34:49, khmel wrote: > > > nit: not sure, but > > > // extensions::NativeMessageHost: > > > ? > > > > Hmm. There seem various patterns... > > // $classname: > > // Overrides $classname: > > // $classname overrides: > > // $classname override: > > // Overridden $classname: > > > > etc. > > How about standardize "// $classname overrides:" then? I haven't touched to > > other ones, but will do in a separate CL. > > > > I'd consult the chromium-dev mailing list if this has already been discussed. > Otherwise, use the good old "whichever pattern has more matches with ag | wc -l > trick". As long as this is not standardized, I'd like to make this off-topic. I'm happy to change to any of above, and happy to have a discussion for standardizing if necessary, but I would like to avoid the discussion in this CL. https://codereview.chromium.org/2436903003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2436903003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/BUILD.gn:241: "arc/extensions/arc_support_message_host.h", On 2016/10/21 07:50:48, Luis Héctor Chávez wrote: > nit: s/extensions/extension/ Let me keep as is, since in chrome repo, extensions looks more common.
Thank you for update. I have following recommendation: arc_support_host.h arc_support_message_host.c are quite small classes. arc_support_host can do nothing without message_host. In this CL we have not trivial functionality that handles settings message_host and checking if it exists or not. As I see, everything might be implement simpler. How about: Join these classes as before. Start this Platform App when required. On initialization you just register: ArcAuthService::Get()->SetSupportHost(this) On close: ArcAuthService::Get()->SetSupportHost(null) Thus we get rid of complicated logic by handling setting and accessing message_host This is up to you but IMHO it could be easier. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:134: return; On 2016/10/21 07:40:27, hidehiko wrote: > On 2016/10/21 03:34:49, khmel wrote: > > IIUC this should never happen. May be LOG(ERROR)? > > This happens. Because ArcSupportMessageHost creation is async, when this is > called, it is no guarantee that message_host_ is ready. I have concern about losing data for user (not shown error or other important information is bad). Normally, IIUC, when this happens that means page is creating in the background and will be shown soon. But if not, and page could not be created? In this case in log we would have nothing. Not sure how to handle this better. May be set flag that indicates we have pending request to show the page and on closing to analyze that we reset it.
https://codereview.chromium.org/2436903003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2436903003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:560: const extensions::AppWindowRegistry* const app_window_registry = One more request, can you please move logic of showing platform app to arc_support_host? I think, it more logical to have it there. For example: static void ShowForProfile(Profile* profile) { ... }
Thank you for review. > I have following recommendation: > arc_support_host.h arc_support_message_host.c are quite small classes. > arc_support_host can do nothing without message_host. In this CL we > have not trivial functionality that handles settings message_host and > checking if it exists or not. As I see, everything might be implement > simpler. > How about: > Join these classes as before. Start this Platform App when required. > On initialization you just register: ArcAuthService::Get()->SetSupportHost(this) > On close: ArcAuthService::Get()->SetSupportHost(null) > Thus we get rid of complicated logic by handling setting and accessing > message_host > > This is up to you but IMHO it could be easier. Let me keep two classes split. The goal of this CL and its follows is to split the UI code from ArcAuthService. For that perspective, we need two classes; 1) Managed by ARC. It will provide APIs to control extension UI, and these can be called from other ARC modules. 2) Managed by extension system. This will be exchanging messages between browser process and the extension. In this CL, 1) is ArcSupportHost and 2) is ArcSupportMessageHost, which inherits NativeMessageHost. The reason why we need two classes is because the current system is designed that instances of NativeMessageHost needs to be created in extension system. However, it's better to have a class managed by ARC system, otherwise, as you pointed in an inline comment, some messages/UI operations could be lost due to async operations. Merging will require clients to manage async operations, as ArcAuthService does now, which is not a part of authorization and actually what I'm cleaning up. From coding perspective, I just moved async layer for delegate set up from (ArcAuthService <-> ArcSupportHost) to (ArcSupportHost <-> ArcSupportMessageHost), so no regression I think in this perspective. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:134: return; On 2016/10/21 14:11:17, khmel wrote: > On 2016/10/21 07:40:27, hidehiko wrote: > > On 2016/10/21 03:34:49, khmel wrote: > > > IIUC this should never happen. May be LOG(ERROR)? > > > > This happens. Because ArcSupportMessageHost creation is async, when this is > > called, it is no guarantee that message_host_ is ready. > > I have concern about losing data for user (not shown error or other important > information is bad). Normally, IIUC, when this happens that means page is > creating in the background and will be shown soon. But if not, and page could > not be created? In this case in log we would have nothing. Not sure how to > handle this better. May be set flag that indicates we have pending request to > show the page and on closing to analyze that we reset it. Currently, no data loss is ensured in the ArcAuthService. If the page show event is fired in ArcAuthService before the actual window creation (or, in more precise, before ArcSupportMessageHost is set to ArcSupportHost), the ArcAuthService's fields remember it, and Initialize() sends them to the extension. It is just because the existing code is so, and I think it is not good design. I'll send another CL to move the responsibility to this class immediately after this CL ges landed. Then, client won't be necessary to take care about such a timing issue. https://codereview.chromium.org/2436903003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2436903003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:560: const extensions::AppWindowRegistry* const app_window_registry = On 2016/10/21 14:27:23, khmel wrote: > One more request, can you please move logic of showing platform app to > arc_support_host? I think, it more logical to have it there. > > For example: > > static void ShowForProfile(Profile* profile) { > ... > } Sure I will, but let me work on it in a separate CL. It is a part of TODO: // TODO(hidehiko): Move more implementation for the UI control from // ArcAuthService to this class. in arc_support_host.h.
lgtm with comments https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:472: support_host_.reset(new ArcSupportHost()); On 2016/10/21 07:40:27, hidehiko wrote: > On 2016/10/21 03:34:49, khmel wrote: > > I would recommend to create it on demand. In most cases, UI is not shown. > > Ah, yes it can be in theory, but the current code complexity is making it > difficult. Specifically, I'm moving more UI related code to ArcSupportHost so > that the timing gets more and more complicated. > > So, let me keep it as is for now, and let's revisit here after more refactoring, > so that life-time management can be easier, I think. Added TODO. Took another look. arc_support_host is not trivial, it register pref observers. Having this functionality on for all cases is too risky and wasteful. How about creating support host on demand for external consumers? ArcSupportHost* GetSupportHost() { if (!support_host) { .... } return support_host; } IMHO You can get rid from additional TODO // TODO(hidehiko): This if statement is only for testing. Clean up this. if (g_browser_process->local_state()) ... https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:679: if (support_host_) On 2016/10/21 07:40:27, hidehiko wrote: > On 2016/10/21 03:34:49, khmel wrote: > > In my previous comment I recommend to create host on demand. > > If this is not possible, IMHO this check is redundant and we can change to > > DCHECK() > > Unfortunately, it cannot, specifically for CloseUI(), which is called in > Shutdown() called in OnPrimaryUserProfilePrepared(). I do want to be consistent > SetUIPage() and CloseUI(), so let me keep this as is now. Acknowledged, however it would be nice to have LOG(ERROR) if we try to show something and cannot dur missing the host. At least we would have log record that will help to analyze the problem. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:134: return; On 2016/10/24 06:03:26, hidehiko wrote: > On 2016/10/21 14:11:17, khmel wrote: > > On 2016/10/21 07:40:27, hidehiko wrote: > > > On 2016/10/21 03:34:49, khmel wrote: > > > > IIUC this should never happen. May be LOG(ERROR)? > > > > > > This happens. Because ArcSupportMessageHost creation is async, when this is > > > called, it is no guarantee that message_host_ is ready. > > > > I have concern about losing data for user (not shown error or other important > > information is bad). Normally, IIUC, when this happens that means page is > > creating in the background and will be shown soon. But if not, and page could > > not be created? In this case in log we would have nothing. Not sure how to > > handle this better. May be set flag that indicates we have pending request to > > show the page and on closing to analyze that we reset it. > > Currently, no data loss is ensured in the ArcAuthService. > If the page show event is fired in ArcAuthService before the actual window > creation (or, in more precise, before ArcSupportMessageHost is set to > ArcSupportHost), the ArcAuthService's fields remember it, and Initialize() sends > them to the extension. > It is just because the existing code is so, and I think it is not good design. > I'll send another CL to move the responsibility to this class immediately after > this CL ges landed. Then, client won't be necessary to take care about such a > timing issue. Acknowledged. https://codereview.chromium.org/2436903003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2436903003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:560: const extensions::AppWindowRegistry* const app_window_registry = On 2016/10/24 06:03:26, hidehiko wrote: > On 2016/10/21 14:27:23, khmel wrote: > > One more request, can you please move logic of showing platform app to > > arc_support_host? I think, it more logical to have it there. > > > > For example: > > > > static void ShowForProfile(Profile* profile) { > > ... > > } > > Sure I will, but let me work on it in a separate CL. It is a part of TODO: > > // TODO(hidehiko): Move more implementation for the UI control from > // ArcAuthService to this class. > > in arc_support_host.h. Acknowledged.
chrome/browser/extensions/ lgtm
Thank you for review. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:472: support_host_.reset(new ArcSupportHost()); On 2016/10/24 15:05:36, khmel wrote: > On 2016/10/21 07:40:27, hidehiko wrote: > > On 2016/10/21 03:34:49, khmel wrote: > > > I would recommend to create it on demand. In most cases, UI is not shown. > > > > Ah, yes it can be in theory, but the current code complexity is making it > > difficult. Specifically, I'm moving more UI related code to ArcSupportHost so > > that the timing gets more and more complicated. > > > > So, let me keep it as is for now, and let's revisit here after more > refactoring, > > so that life-time management can be easier, I think. Added TODO. > > Took another look. arc_support_host is not trivial, it register pref observers. > Having this functionality on for all cases is too risky and wasteful. How about > creating support host on demand for external consumers? > > ArcSupportHost* GetSupportHost() { > if (!support_host) { > .... > } > return support_host; > } > > IMHO You can get rid from additional TODO > > // TODO(hidehiko): This if statement is only for testing. Clean up this. > if (g_browser_process->local_state()) > ... > > > I agree that observing pref can be wasteful, but it looks small enough? Also, could you explain what's the risk there? Anyway, I agreed that observing pref in ArcSupportHost is not a good way. Actually, it will be removed from ArcSupportHost with your pending CL ;-). So, the TODO will be addressed in near future. Considering the code change size, let's work on it separately. Also, introducing GetSupportHost() needs singleton-ish mechanism, which Luis and I agreed that we should avoid. Or, we need a new UI code in ArcAuthService, which I'm actually removing. So, I'll avoid it. https://codereview.chromium.org/2436903003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:679: if (support_host_) On 2016/10/24 15:05:36, khmel wrote: > On 2016/10/21 07:40:27, hidehiko wrote: > > On 2016/10/21 03:34:49, khmel wrote: > > > In my previous comment I recommend to create host on demand. > > > If this is not possible, IMHO this check is redundant and we can change to > > > DCHECK() > > > > Unfortunately, it cannot, specifically for CloseUI(), which is called in > > Shutdown() called in OnPrimaryUserProfilePrepared(). I do want to be > consistent > > SetUIPage() and CloseUI(), so let me keep this as is now. > > Acknowledged, however it would be nice to have LOG(ERROR) if we try to show > something and cannot dur missing the host. At least we would have log record > that will help to analyze the problem. Adding log makes sense. But, as I said, it is not an error so LOG(ERROR) sounds too strong. Let's use VLOG(2) instead, which is used for normal code path in c/b/c/arc.
Rebased and submitting. Yury, my understanding is that you're ok to land the latest one, but please let me know if you have more things. Thank you for review, all.
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, asargent@chromium.org, khmel@chromium.org Link to the patchset: https://codereview.chromium.org/2436903003/#ps100001 (title: "Rebase")
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 ========== Extract ArcSupportMessageHost. Currently ArcSupportHost is the ARC's implementation of NativeMessageHost. Because of the system structure, the instance is created and managed by the module outside of ARC. This CL extracts the implementation of that part into ArcSupportMessageHost so that the ArcSupportHost, which will be the main interface to show UI things for ARC, can be managed along with the ARC's life-time. There is a plan to extract non authorization related part from ArcAuthSerivce. Meanwhile, the instance is managed by ArcAuthService, temporarily. BUG=657687, 636218, 633258, b/31079732 TEST=Ran on test device. Ran trybots. ========== to ========== Extract ArcSupportMessageHost. Currently ArcSupportHost is the ARC's implementation of NativeMessageHost. Because of the system structure, the instance is created and managed by the module outside of ARC. This CL extracts the implementation of that part into ArcSupportMessageHost so that the ArcSupportHost, which will be the main interface to show UI things for ARC, can be managed along with the ARC's life-time. There is a plan to extract non authorization related part from ArcAuthSerivce. Meanwhile, the instance is managed by ArcAuthService, temporarily. BUG=657687, 636218, 633258, b/31079732 TEST=Ran on test device. Ran trybots. ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Extract ArcSupportMessageHost. Currently ArcSupportHost is the ARC's implementation of NativeMessageHost. Because of the system structure, the instance is created and managed by the module outside of ARC. This CL extracts the implementation of that part into ArcSupportMessageHost so that the ArcSupportHost, which will be the main interface to show UI things for ARC, can be managed along with the ARC's life-time. There is a plan to extract non authorization related part from ArcAuthSerivce. Meanwhile, the instance is managed by ArcAuthService, temporarily. BUG=657687, 636218, 633258, b/31079732 TEST=Ran on test device. Ran trybots. ========== to ========== Extract ArcSupportMessageHost. Currently ArcSupportHost is the ARC's implementation of NativeMessageHost. Because of the system structure, the instance is created and managed by the module outside of ARC. This CL extracts the implementation of that part into ArcSupportMessageHost so that the ArcSupportHost, which will be the main interface to show UI things for ARC, can be managed along with the ARC's life-time. There is a plan to extract non authorization related part from ArcAuthSerivce. Meanwhile, the instance is managed by ArcAuthService, temporarily. BUG=657687, 636218, 633258, b/31079732 TEST=Ran on test device. Ran trybots. Committed: https://crrev.com/e0eb7942096fb4945f5d5d3ed23632148d5348bd Cr-Commit-Position: refs/heads/master@{#427277} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e0eb7942096fb4945f5d5d3ed23632148d5348bd Cr-Commit-Position: refs/heads/master@{#427277} |
