Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2501013002:
Refactor ArcSupportHost and ArcAuthService part2. (Closed)
|
Created:
4 years, 1 month ago by hidehiko Modified:
4 years, 1 month ago Reviewers:
Luis Héctor Chávez CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, khmel+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, khmel Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor ArcSupportHost and ArcAuthService part2.
This CL moves more UI related implementation from ArcAuthService
to ArcSupportHost, and remove the dependency from ArcSupportHost
to ArcAuthService.
BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.
Committed: https://crrev.com/999d64e15ddfde5a8b6f2332db63caf8234871be
Cr-Commit-Position: refs/heads/master@{#432833}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 25
Patch Set 3 : rebase #Patch Set 4 : Address comments. #Patch Set 5 : rebase #
Messages
Total messages: 21 (10 generated)
hidehiko@chromium.org changed reviewers: + lhchavez@chromium.org
PTAL.
On 2016/11/14 17:42:13, hidehiko wrote: > PTAL. friendly ping?
sorry for the delay. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:591: support_host_->AddObserver(this); I just realized we're not calling ->RemoveObserver(this) anywhere. Please add that in the destructor and possibly before L590 if there's already an instance. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:806: // In case |state_| is ACTIVE, UI page can be ARC_LOADING (which means normal This comment does not seem to agree with the code anymore. Not sure what the right course of action is here. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:189: ui_page_ = ui_page; How about moving this to L191? https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:193: << "before connection to ARC support Chrome app is established."; nit: s/is established/has finished establishing/ ? That better reflects that the connection was pending. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:221: ui_page_ = UIPage::ERROR; Given that you only need these if |!message_host_|, how about moving these to L225? https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:359: CHECK(extension); According to dcheng@, CHECK should never be used outside of base/, and DCHECK should be used instead. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:90: void SetArcManaged(bool is_arc_managed); It seems like this can be set in the constructor. That should reduce the number of moving parts. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:101: // Requests to show the "ARC is loadign" page. nit: s/loadign/loading/ https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:155: Profile* profile_; nit: Profile* const profile_; https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:164: bool app_start_requested_ = false; How about |app_start_pending_|? That better reflects the transient nature of this resettable flag. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:174: // These are valid value iff ui_page_ == ERROR. nit: s/are valid values/have valid values/.
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Thank you for review. PTAL. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:591: support_host_->AddObserver(this); On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > I just realized we're not calling ->RemoveObserver(this) anywhere. Please add > that in the destructor and possibly before L590 if there's already an instance. Addressed in crrev.com/2502243002. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:806: // In case |state_| is ACTIVE, UI page can be ARC_LOADING (which means normal On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > This comment does not seem to agree with the code anymore. Not sure what the > right course of action is here. I agree that the condition looks somewhat weird, but it still seems to have some matching between the comment and the code? I'm happy to work on cleaning up it, but can I on another CL, because this part should be just a mechanical code change and I believe the comment weirdness is not the regression by this CL? https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:189: ui_page_ = ui_page; On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > How about moving this to L191? Because ArcAuthService checks UIPage in some context, it needs to be set always (= even in messag_host_ is non-null case). Clean up is TODO in the header. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:193: << "before connection to ARC support Chrome app is established."; On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > nit: s/is established/has finished establishing/ ? That better reflects that the > connection was pending. Done. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:221: ui_page_ = UIPage::ERROR; On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > Given that you only need these if |!message_host_|, how about moving these to > L225? Ditto. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:359: CHECK(extension); On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > According to dcheng@, CHECK should never be used outside of base/, and DCHECK > should be used instead. Done. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:90: void SetArcManaged(bool is_arc_managed); On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > It seems like this can be set in the constructor. That should reduce the number > of moving parts. ARC managed status can be changed, and OnOptInPreferenceChanged() should be called in that timing, so ctor does not work, IIUC? That's why this setter is introduced. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:101: // Requests to show the "ARC is loadign" page. On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > nit: s/loadign/loading/ Done. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:155: Profile* profile_; On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > nit: Profile* const profile_; Done. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:164: bool app_start_requested_ = false; On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > How about |app_start_pending_|? That better reflects the transient nature of > this resettable flag. Done. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:174: // These are valid value iff ui_page_ == ERROR. On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > nit: s/are valid values/have valid values/. Done.
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.
lgtm https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:806: // In case |state_| is ACTIVE, UI page can be ARC_LOADING (which means normal On 2016/11/16 17:52:21, hidehiko wrote: > On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > > This comment does not seem to agree with the code anymore. Not sure what the > > right course of action is here. > > I agree that the condition looks somewhat weird, but it still seems to have some > matching between the comment and the code? > I'm happy to work on cleaning up it, but can I on another CL, > because this part should be just a mechanical code change and I believe the > comment weirdness is not the regression by this CL? sgtm https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:189: ui_page_ = ui_page; On 2016/11/16 17:52:21, hidehiko wrote: > On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > > How about moving this to L191? > > Because ArcAuthService checks UIPage in some context, it needs to be set always > (= even in messag_host_ is non-null case). Clean up is TODO in the header. Acknowledged. https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2501013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:90: void SetArcManaged(bool is_arc_managed); On 2016/11/16 17:52:21, hidehiko wrote: > On 2016/11/16 02:58:53, Luis Héctor Chávez wrote: > > It seems like this can be set in the constructor. That should reduce the > number > > of moving parts. > > ARC managed status can be changed, and OnOptInPreferenceChanged() should be > called in that timing, so ctor does not work, IIUC? That's why this setter is > introduced. argh, sorry, forgot that was possible.
Thank you for review. Submitting.
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 Link to the patchset: https://codereview.chromium.org/2501013002/#ps80001 (title: "rebase")
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: 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
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Refactor ArcSupportHost and ArcAuthService part2. This CL moves more UI related implementation from ArcAuthService to ArcSupportHost, and remove the dependency from ArcSupportHost to ArcAuthService. BUG=657687 BUG=b/31079732 TEST=Ran on test device. Ran bots. ========== to ========== Refactor ArcSupportHost and ArcAuthService part2. This CL moves more UI related implementation from ArcAuthService to ArcSupportHost, and remove the dependency from ArcSupportHost to ArcAuthService. BUG=657687 BUG=b/31079732 TEST=Ran on test device. Ran bots. Committed: https://crrev.com/999d64e15ddfde5a8b6f2332db63caf8234871be Cr-Commit-Position: refs/heads/master@{#432833} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/999d64e15ddfde5a8b6f2332db63caf8234871be Cr-Commit-Position: refs/heads/master@{#432833} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
