Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529783002:
arc: Implement uninstall confirmation dialog for Arc app. (Closed)
|
Created:
4 years ago by lgcheng Modified:
4 years ago Reviewers:
msw CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, tfarina, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, Matt Giuca Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Implement uninstall confirmation dialog for Arc app.
Add ArcAppDialogView for arc app uninstall confirmation. Add Test coverage.
BUG=661076
Test=Add browsertest. Tests passed.
Test=Manual Test.
Committed: https://crrev.com/296586cffbb9baa109722179c0af2e3534316873
Cr-Commit-Position: refs/heads/master@{#435641}
Patch Set 1 #Patch Set 2 : Test coverage. #Patch Set 3 : Clean up. #
Total comments: 73
Patch Set 4 : Address Mike's comments. Run git cl format. #
Total comments: 10
Patch Set 5 : Address minor comments. Rebase. #Messages
Total messages: 36 (27 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== arc: Implement uninstall confirmation dialog for Arc app. Move view layout implementation from extension_uninstall_dialog_view.cc to uninstall_dialog_delegate_view.cc for reuse. Add uninstall_dialog for Arc app. Add Test coverage. BUG=661076 Test=Add browsertest. Tests passed. ========== to ========== arc: Implement uninstall confirmation dialog for Arc app. Add ArcAppDialogView for arc app uninstall confirmation. Add Test coverage. BUG=661076 Test=Add browsertest. Tests passed. ==========
The CQ bit was checked by lgcheng@google.com 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_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by lgcheng@google.com 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.
lgcheng@google.com changed reviewers: + msw@chromium.org
Hi Mike, Hope you had a great holiday. I create this new CL with your suggestion. Would you PTAL when have a moment? Thanks!
This is *so* much better; thank you!!! Just a few major comments; please know that I always give lots of nits :) https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_utils.cc:13: #include "chrome/browser/profiles/profile.h" optional nit: this probably isn't needed (forward decl should suffice) https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_utils.cc:353: // for shortcut we just remove the shortcut instead of the package nit: trailing period; optionally move above conditional to nix curlies. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/arc_app_dialog_view.cc (right): https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:10: #include "chrome/browser/profiles/profile.h" ditto nit: just use forward decl. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:32: const int kIconSourceSize = 32; Can we just ask the loader for 64px and let it deal with resizing as needed? It seems like specifying 32px might ignore some larger (48px or maybe even 64px) icons that would be better to display at 64px. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:41: const base::string16& head_text, nit: |heading_text| here and elsewhere https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:47: // Public method to start dialog process. nit: "Start loading the icon; the dialog will be shown when loading completes." https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:51: void SelectOptionForTest(bool confirm); optional nit: |ConfirmOrCancelForTest| https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:55: base::string16 GetWindowTitle() const override { return window_title_; } nit: define these out-of-line, to be consistent with other member functions. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:72: // Popups the constrained window of confirmation dialog. nit: "Constructs and shows the modal dialog widget." https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:76: gfx::ImageSkia icon_ = gfx::ImageSkia(); You don't need to explicitly invoke the default ctor here. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:78: views::ImageView* icon_view_; nit: = nullptr; https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:79: views::Label* heading_view_; nit: = nullptr; https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:124: gfx::Size size(gfx::Size(kIconSize, kIconSize)); nit: "gfx::Size size(kIconSize, kIconSize);" or just inline "icon_view_->SetImageSize(gfx::Size(kIconSize, kIconSize));" below https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:137: g_current_arc_app_dialog_view = nullptr; nit: DCHECK_EQ(this, g_current_arc_app_dialog_view); above this https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:141: icon_loader_.reset(new ArcAppIconLoader(profile_, kIconSourceSize, this)); optional nit: inline this in the constructor? https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:153: if (controller_) { optional nit: remove curlies. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:156: delete this; optional nit: call DialogDelegateView::DeleteDelegate(); instead. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:159: gfx::Size ArcAppDialogView::GetPreferredSize() const { nit: Can you use a LayoutManager instead of doing manual layout and sizing? It seems like BoxLayout might suffice if you call heading_view_->SizeToFit(kRightColumnWidth), or GridLayout can be used if you need something more specific/complex. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:202: bool ArcAppDialogView::Cancel() { Not needed; DialogDelegateView::Cancel already returns true. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:215: if (image.isNull()) { nit: remove curlies https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:216: icon_ = extensions::util::GetDefaultAppIcon(); Remove the |icon_| class member and just do: icon_view_->SetImage(image.isNull() ? extensions::util::GetDefaultAppIcon() : image); https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:224: Show(); So we only create and show the dialog once icon loading completes. It's okay to do this asynchronously, but I'm a little concerned that we might leak the ArcAppDialogView if this is never called (icon loading hangs indefinitely or fails or similar). Are there any reasonable ways to safeguard against that? One option might be to set g_current_arc_app_dialog_view in the ctor and fire a timer to cancel icon loading and destroy the delegate instance if a timeout is reached. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:232: Cancel(); Does |this| ArcAppDialogView leak here? Perhaps it should delete itself here? https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:243: void ShowDialogImpl(Profile* profile, optional nit: inline this in ShowArcAppUninstallDialog? https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:262: ArcAppConfirmCallback confirm_callback) { nit: bind arc::UninstallArcApp in this function, nix this param? https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:32: chromeos::FakeChromeUserManager* GetUserManager() { optional nit: inline and cache a ptr below. (I see this is a copy-paste of arc_app_test.cc and it'd be nice to fix both, or optional to keep both as-is...) https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:55: void SetUpAppInstance() { It would be nice if there was a common utility function or shared arc browser_tests base class to do this for these tests and other tests. It seems like a fair amount of the setup code is shared with other test suites. Can you create an ArcAppTest instance in this class and call its SetUp function to remove some redundancy? https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:67: chromeos::switches::kEnableArc); nit: indentation (run "git cl format") https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:72: if (!arc_app_list_pref_) { Why is this conditional? It seems like we should know in advance whether an instance exists? https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:127: ArcAppListPrefs* arc_app_list_pref_; nit: = nullptr; https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:129: Profile* profile_; nit: = nullptr; https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:139: IN_PROC_BROWSER_TEST_F(ArcAppUninstallDialogViewBrowserTest, Nice test fixture; should we have a similar one for shortcut removal? https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:153: base::Bind(UninstallArcApp)); nit: indentation (run "git cl format") https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:164: base::Bind(UninstallArcApp)); nit: indentation (run "git cl format")
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Hi Mike, Thanks for such a careful review! Address your moments in the update. Would you PTAL when having a moment? Two highlights. 1. Still keep the manual layout in view. 2. Not using ArcAppTest suite in browser_test. Detailed reasons are listed in the reply to your comments. Thanks! https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_utils.cc:13: #include "chrome/browser/profiles/profile.h" On 2016/11/29 20:47:36, msw wrote: > optional nit: this probably isn't needed (forward decl should suffice) It seems I need this header file. static ArcAppListPrefs* Get(content::BrowserContext* context); I need profile.h so that compile knows Profile is subclass of content::BrowserContext https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_utils.cc:353: // for shortcut we just remove the shortcut instead of the package On 2016/11/29 20:47:36, msw wrote: > nit: trailing period; optionally move above conditional to nix curlies. Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/arc_app_dialog_view.cc (right): https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:10: #include "chrome/browser/profiles/profile.h" On 2016/11/29 20:47:36, msw wrote: > ditto nit: just use forward decl. Need this so that compile knows Profile is subclass of content::BrowserContext. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:32: const int kIconSourceSize = 32; On 2016/11/29 20:47:37, msw wrote: > Can we just ask the loader for 64px and let it deal with resizing as needed? It > seems like specifying 32px might ignore some larger (48px or maybe even 64px) > icons that would be better to display at 64px. Make loader handles the resizing. Remove kIconSourceSize Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:41: const base::string16& head_text, On 2016/11/29 20:47:36, msw wrote: > nit: |heading_text| here and elsewhere Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:47: // Public method to start dialog process. On 2016/11/29 20:47:36, msw wrote: > nit: "Start loading the icon; the dialog will be shown when loading completes." Remove this method. Inline in constructor. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:51: void SelectOptionForTest(bool confirm); On 2016/11/29 20:47:37, msw wrote: > optional nit: |ConfirmOrCancelForTest| Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:55: base::string16 GetWindowTitle() const override { return window_title_; } On 2016/11/29 20:47:37, msw wrote: > nit: define these out-of-line, to be consistent with other member functions. Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:72: // Popups the constrained window of confirmation dialog. On 2016/11/29 20:47:37, msw wrote: > nit: "Constructs and shows the modal dialog widget." Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:76: gfx::ImageSkia icon_ = gfx::ImageSkia(); On 2016/11/29 20:47:37, msw wrote: > You don't need to explicitly invoke the default ctor here. Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:78: views::ImageView* icon_view_; On 2016/11/29 20:47:36, msw wrote: > nit: = nullptr; Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:79: views::Label* heading_view_; On 2016/11/29 20:47:37, msw wrote: > nit: = nullptr; Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:124: gfx::Size size(gfx::Size(kIconSize, kIconSize)); On 2016/11/29 20:47:36, msw wrote: > nit: "gfx::Size size(kIconSize, kIconSize);" or just inline > "icon_view_->SetImageSize(gfx::Size(kIconSize, kIconSize));" below Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:137: g_current_arc_app_dialog_view = nullptr; On 2016/11/29 20:47:37, msw wrote: > nit: DCHECK_EQ(this, g_current_arc_app_dialog_view); above this Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:141: icon_loader_.reset(new ArcAppIconLoader(profile_, kIconSourceSize, this)); On 2016/11/29 20:47:37, msw wrote: > optional nit: inline this in the constructor? Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:153: if (controller_) { On 2016/11/29 20:47:37, msw wrote: > optional nit: remove curlies. Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:156: delete this; On 2016/11/29 20:47:37, msw wrote: > optional nit: call DialogDelegateView::DeleteDelegate(); instead. Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:159: gfx::Size ArcAppDialogView::GetPreferredSize() const { On 2016/11/29 20:47:36, msw wrote: > nit: Can you use a LayoutManager instead of doing manual layout and sizing? It > seems like BoxLayout might suffice if you call > heading_view_->SizeToFit(kRightColumnWidth), or GridLayout can be used if you > need something more specific/complex. This part is identical with extension uninstall dialog. I'd make them stay the same in this CL. If we want to change to better implementation, I wonder we need update both sides(here and extension uninstall dialog) to avoid any potential UI/UX inconsistency between extension uninstall dialog and arc app uninstall dialog since I am not super familiar with layout code. WDYT? https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:202: bool ArcAppDialogView::Cancel() { On 2016/11/29 20:47:37, msw wrote: > Not needed; DialogDelegateView::Cancel already returns true. Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:215: if (image.isNull()) { On 2016/11/29 20:47:36, msw wrote: > nit: remove curlies Redundant check removed. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:216: icon_ = extensions::util::GetDefaultAppIcon(); On 2016/11/29 20:47:37, msw wrote: > Remove the |icon_| class member and just do: > icon_view_->SetImage(image.isNull() ? extensions::util::GetDefaultAppIcon() : > image); Redundant image nullity check removed. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:224: Show(); On 2016/11/29 20:47:36, msw wrote: > So we only create and show the dialog once icon loading completes. It's okay to > do this asynchronously, but I'm a little concerned that we might leak the > ArcAppDialogView if this is never called (icon loading hangs indefinitely or > fails or similar). Are there any reasonable ways to safeguard against that? > > One option might be to set g_current_arc_app_dialog_view in the ctor and fire a > timer to cancel icon loading and destroy the delegate instance if a timeout is > reached. There is safeguard in ArcAppIconLoader ensures that it will at least update with default extension Icon. Remove my redundant image nullity check above. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:232: Cancel(); On 2016/11/29 20:47:37, msw wrote: > Does |this| ArcAppDialogView leak here? Perhaps it should delete itself here? Yes, there is a potential here. delete this added. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:243: void ShowDialogImpl(Profile* profile, On 2016/11/29 20:47:36, msw wrote: > optional nit: inline this in ShowArcAppUninstallDialog? Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:262: ArcAppConfirmCallback confirm_callback) { On 2016/11/29 20:47:37, msw wrote: > nit: bind arc::UninstallArcApp in this function, nix this param? Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:32: chromeos::FakeChromeUserManager* GetUserManager() { On 2016/11/29 20:47:37, msw wrote: > optional nit: inline and cache a ptr below. (I see this is a copy-paste of > arc_app_test.cc and it'd be nice to fix both, or optional to keep both as-is...) These lines turn out to be not needed for browser_test. Removed. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:55: void SetUpAppInstance() { On 2016/11/29 20:47:37, msw wrote: > It would be nice if there was a common utility function or shared arc > browser_tests base class to do this for these tests and other tests. It seems > like a fair amount of the setup code is shared with other test suites. Can you > create an ArcAppTest instance in this class and call its SetUp function to > remove some redundancy? ArcAppTest::Setup() is very different from what is here although they share a lot of Arc enable code. ArcAppTest is designed as a unit_test helper, where ArcBridgeService and ArcSessionManager are not initialized so ArcAppTest creates FakeArcBridgeService and then create ArcSessionManager and manages their lifetime. In browser_test ArcBridgeService and ArcSessionManager are pre initialized and their life time is also managed by browser_test base. Initializing a ArcAppTest would result a lot of test teardown error. Good news is that after reexamination, a lot of setup code needed in unit_test is not needed in browser_test. Remove unnecessary code. The code which remains to Enable Arc does not look that too much now. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:67: chromeos::switches::kEnableArc); On 2016/11/29 20:47:37, msw wrote: > nit: indentation (run "git cl format") Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:72: if (!arc_app_list_pref_) { On 2016/11/29 20:47:37, msw wrote: > Why is this conditional? It seems like we should know in advance whether an > instance exists? The browser_test env try to call ArcAppListPrefs::Get(profile_); and keep the mapping [profile* -> nullptr] in the key_factory map before I turn on the system switch. The mapping of [profile* -> nullptr] stays in the map and prevents future ArcAppListPrefs::Get(profile_) call to create the ArcAppListPrefs. In any cases other than tests, availability of Arc never changes without user logging out. I think this is current workaround for Arc environment in browser tests that the availability of Arc changes. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:127: ArcAppListPrefs* arc_app_list_pref_; On 2016/11/29 20:47:37, msw wrote: > nit: = nullptr; Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:129: Profile* profile_; On 2016/11/29 20:47:37, msw wrote: > nit: = nullptr; Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:139: IN_PROC_BROWSER_TEST_F(ArcAppUninstallDialogViewBrowserTest, On 2016/11/29 20:47:37, msw wrote: > Nice test fixture; should we have a similar one for shortcut removal? Shortcut removal test added. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:153: base::Bind(UninstallArcApp)); On 2016/11/29 20:47:37, msw wrote: > nit: indentation (run "git cl format") Done. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:164: base::Bind(UninstallArcApp)); On 2016/11/29 20:47:37, msw wrote: > nit: indentation (run "git cl format") Done.
Nice, I have just a few minor comments left. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_utils.cc:13: #include "chrome/browser/profiles/profile.h" On 2016/11/30 19:28:46, lgcheng wrote: > On 2016/11/29 20:47:36, msw wrote: > > optional nit: this probably isn't needed (forward decl should suffice) > > It seems I need this header file. > static ArcAppListPrefs* Get(content::BrowserContext* context); > > I need profile.h so that compile knows Profile is subclass of > content::BrowserContext Acknowledged. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/arc_app_dialog_view.cc (right): https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:159: gfx::Size ArcAppDialogView::GetPreferredSize() const { On 2016/11/30 19:28:47, lgcheng wrote: > On 2016/11/29 20:47:36, msw wrote: > > nit: Can you use a LayoutManager instead of doing manual layout and sizing? It > > seems like BoxLayout might suffice if you call > > heading_view_->SizeToFit(kRightColumnWidth), or GridLayout can be used if you > > need something more specific/complex. > > This part is identical with extension uninstall dialog. I'd make them stay the > same in this CL. If we want to change to better implementation, I wonder we need > update both sides(here and extension uninstall dialog) to avoid any potential > UI/UX inconsistency between extension uninstall dialog and arc app uninstall > dialog since I am not super familiar with layout code. > > WDYT? I see. Please file a bug to update both in a followup CL and CC me. Add a comment here that this is a copy of ExtensionUninstallDialogDelegateView sizing and layout code, and cite the bug. It should be fairly easy to use BoxLayout or some other LayoutManager in both cases, the codebase has many examples. I can help as needed. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:224: Show(); On 2016/11/30 19:28:46, lgcheng wrote: > On 2016/11/29 20:47:36, msw wrote: > > So we only create and show the dialog once icon loading completes. It's okay > to > > do this asynchronously, but I'm a little concerned that we might leak the > > ArcAppDialogView if this is never called (icon loading hangs indefinitely or > > fails or similar). Are there any reasonable ways to safeguard against that? > > > > One option might be to set g_current_arc_app_dialog_view in the ctor and fire > a > > timer to cancel icon loading and destroy the delegate instance if a timeout is > > reached. > > There is safeguard in ArcAppIconLoader ensures that it will at least update with > default extension Icon. Remove my redundant image nullity check above. It would be even better to have a safeguard here, but that may suffice. https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:72: if (!arc_app_list_pref_) { On 2016/11/30 19:28:47, lgcheng wrote: > On 2016/11/29 20:47:37, msw wrote: > > Why is this conditional? It seems like we should know in advance whether an > > instance exists? > > The browser_test env try to call ArcAppListPrefs::Get(profile_); and keep the > mapping [profile* -> nullptr] in the key_factory map before I turn on the system > switch. The mapping of [profile* -> nullptr] stays in the map and prevents > future ArcAppListPrefs::Get(profile_) call to create the ArcAppListPrefs. > > In any cases other than tests, availability of Arc never changes without user > logging out. I think this is current workaround for Arc environment in browser > tests that the availability of Arc changes. Hmm, okay. https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/arc_app_dialog_view.cc (right): https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/arc_app_dialog_view.cc:134: icon_loader_.reset(new ArcAppIconLoader(profile_, kIconSize, this)); nit: add a comment like "The dialog will show once the icon is loaded." https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/arc_app_dialog_view.cc:148: this->GetWidget()->Close(); nit: is this needed? if so, can you remove "this->"? https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/arc_app_dialog_view.cc:217: icon_view_->SetImage(image); nit: DCHECK(!image.isNull()); above. https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/arc_app_dialog_view.cc:226: // Parent window is killed before icon is loaded. nit: "The parent window was killed before the icon was loaded." https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:149: // uninstalled when the short cut is removed. nit: shortcut
Description was changed from ========== arc: Implement uninstall confirmation dialog for Arc app. Add ArcAppDialogView for arc app uninstall confirmation. Add Test coverage. BUG=661076 Test=Add browsertest. Tests passed. ========== to ========== arc: Implement uninstall confirmation dialog for Arc app. Add ArcAppDialogView for arc app uninstall confirmation. Add Test coverage. BUG=661076 Test=Add browsertest. Tests passed. Test=Manual Test. ==========
The CQ bit was checked by lgcheng@google.com 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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #5 (id:140001) has been deleted
The CQ bit was checked by lgcheng@google.com 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.
Comments addressed. Would you PTAL when having a moment? Thanks! https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/arc_app_dialog_view.cc (right): https://codereview.chromium.org/2529783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/arc_app_dialog_view.cc:159: gfx::Size ArcAppDialogView::GetPreferredSize() const { On 2016/11/30 23:26:57, msw wrote: > On 2016/11/30 19:28:47, lgcheng wrote: > > On 2016/11/29 20:47:36, msw wrote: > > > nit: Can you use a LayoutManager instead of doing manual layout and sizing? > It > > > seems like BoxLayout might suffice if you call > > > heading_view_->SizeToFit(kRightColumnWidth), or GridLayout can be used if > you > > > need something more specific/complex. > > > > This part is identical with extension uninstall dialog. I'd make them stay the > > same in this CL. If we want to change to better implementation, I wonder we > need > > update both sides(here and extension uninstall dialog) to avoid any potential > > UI/UX inconsistency between extension uninstall dialog and arc app uninstall > > dialog since I am not super familiar with layout code. > > > > WDYT? > > I see. Please file a bug to update both in a followup CL and CC me. Add a > comment here that this is a copy of ExtensionUninstallDialogDelegateView sizing > and layout code, and cite the bug. It should be fairly easy to use BoxLayout or > some other LayoutManager in both cases, the codebase has many examples. I can > help as needed. Done. https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/arc_app_dialog_view.cc (right): https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/arc_app_dialog_view.cc:134: icon_loader_.reset(new ArcAppIconLoader(profile_, kIconSize, this)); On 2016/11/30 23:26:57, msw wrote: > nit: add a comment like "The dialog will show once the icon is loaded." Done. https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/arc_app_dialog_view.cc:148: this->GetWidget()->Close(); On 2016/11/30 23:26:57, msw wrote: > nit: is this needed? if so, can you remove "this->"? This is needed in test. Calling Accept() or Cancel() would not close the window. The object is actually released in test teardown time and would lead to DCHECK_EQ(this, g_current_arc_app_dialog_view) fail in deconstructor if I create more than one ArcAppDialog in a single test. Manual confirm from UI works correctly and object is deconstructed correctly while I observe the logging in manual test. this-> removed. https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/arc_app_dialog_view.cc:217: icon_view_->SetImage(image); On 2016/11/30 23:26:57, msw wrote: > nit: DCHECK(!image.isNull()); above. Done. https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/arc_app_dialog_view.cc:226: // Parent window is killed before icon is loaded. On 2016/11/30 23:26:57, msw wrote: > nit: "The parent window was killed before the icon was loaded." Done. https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2529783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:149: // uninstalled when the short cut is removed. On 2016/11/30 23:26:57, msw wrote: > nit: shortcut Done.
lgtm
The CQ bit was checked by lgcheng@google.com
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": 160001, "attempt_start_ts": 1480613778946450,
"parent_rev": "5aaf469fb3f35401a261fef617804c62f93f1dd0", "commit_rev":
"e1ea2344ad028b7b9aeb0a186c2af76d4aa6cc53"}
Message was sent while issue was closed.
Description was changed from ========== arc: Implement uninstall confirmation dialog for Arc app. Add ArcAppDialogView for arc app uninstall confirmation. Add Test coverage. BUG=661076 Test=Add browsertest. Tests passed. Test=Manual Test. ========== to ========== arc: Implement uninstall confirmation dialog for Arc app. Add ArcAppDialogView for arc app uninstall confirmation. Add Test coverage. BUG=661076 Test=Add browsertest. Tests passed. Test=Manual Test. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== arc: Implement uninstall confirmation dialog for Arc app. Add ArcAppDialogView for arc app uninstall confirmation. Add Test coverage. BUG=661076 Test=Add browsertest. Tests passed. Test=Manual Test. ========== to ========== arc: Implement uninstall confirmation dialog for Arc app. Add ArcAppDialogView for arc app uninstall confirmation. Add Test coverage. BUG=661076 Test=Add browsertest. Tests passed. Test=Manual Test. Committed: https://crrev.com/296586cffbb9baa109722179c0af2e3534316873 Cr-Commit-Position: refs/heads/master@{#435641} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/296586cffbb9baa109722179c0af2e3534316873 Cr-Commit-Position: refs/heads/master@{#435641} |
