|
|
Descriptionarc: Fix crash on accessing app info for secondary user.
This fix the crash whem user activates Chrome app info page
for secondary user profile. This alse adds unit test that
cover this case and generic functionality.
TEST=Manually, unit tests extended
BUG=689263
Review-Url: https://codereview.chromium.org/2700523002
Cr-Commit-Position: refs/heads/master@{#451118}
Committed: https://chromium.googlesource.com/chromium/src/+/b756297e51407c3b602c6d65746a8f18d835d71b
Patch Set 1 #Patch Set 2 : clean up #
Total comments: 25
Patch Set 3 : rebase + comments addressed #
Total comments: 2
Patch Set 4 : minor comment update #
Messages
Total messages: 12 (5 generated)
khmel@chromium.org changed reviewers: + msw@chromium.org
Hi Mike, PTAL https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc (right): https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:147: // "Manage supported links" link for Chrome. Problem was we asked for IsArcEnabled but that was associated with primary profile. Crash fix could be made by asking IsAllowedForProfile additionally but there is one more case when Settings app is not available at this moment and showing "links" section has no sense. So combined solution is to ask ArcAppListPrefs for settings app that also covers secondary user profile case. https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc (left): https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:72: arc::ArcSessionManager::DisableUIForTesting(); This actually does not start ARC (we need extra preparation for flags and setting primary profile). Use better method for this https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:83: dialog_ = new AppInfoDialog(widget_->GetNativeWindow(), Move creation and closing to separate methods Show, ShowForProfile, Close in order to be able to call showing multiple times.
https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc (right): https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:146: // When ARC is enabled and Settings app is available, show the nit: 'the Settings app' https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:147: // "Manage supported links" link for Chrome. On 2017/02/15 16:38:45, khmel wrote: > Problem was we asked for IsArcEnabled but that was associated with primary > profile. Crash fix could be made by asking IsAllowedForProfile additionally but > there is one more case when Settings app is not available at this moment and > showing "links" section has no sense. So combined solution is to ask > ArcAppListPrefs for settings app that also covers secondary user profile case. Wouldn't the IsArcEnabled check here be incorrectly relying on Arc being enabled for a different profile in the multi-profile case? (ie. current user has arc enabled, but the 'primary'/other user has arc disabled, then this check won't pass but it should?) If my guess is correct, please add a test case for that. Otherwise, if that's not the case, it might be slightly redundant, but perhaps it still makes sense to explicitly check IsArcAllowedForProfile, instead on relying on the settings app check to do that under the hood; wdyt? (aside: update two unrelated comment mentions of IsAllowedForProfile to IsArcAllowedForProfile) https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:153: arc_app_info_links_ = new ArcAppInfoLinksPanel(profile, app); optional nit: consider (1) removing this member and letting the tests infer the presence of the links from the number of children held by this view? (perhaps that's a little odd though...). Otherwise, consider (2) adding a public accessor arc_app_info_links_for_test() and removing the friend test fixture declaration or (3) adding a comment on the member declaration like "Checked by unit tests." (it's just odd to see a private member set here and never checked in the class) https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc (left): https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:72: arc::ArcSessionManager::DisableUIForTesting(); On 2017/02/15 16:38:45, khmel wrote: > This actually does not start ARC (we need extra preparation for flags and > setting primary profile). Use better method for this Acknowledged. https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:83: dialog_ = new AppInfoDialog(widget_->GetNativeWindow(), On 2017/02/15 16:38:45, khmel wrote: > Move creation and closing to separate methods Show, ShowForProfile, Close in > order to be able to call showing multiple times. Acknowledged. https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc (right): https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:37: std::vector<arc::mojom::AppInfoPtr> GetArcSettingsAppMojoInfo() { It's a little odd to return a vector for one item, but I guess that's what the caller needs... nit: GetArcSettingsAppInfo https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:47: } // namespace nit: add a blank line above https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:167: bool widget_destroyed_ = true; optional nit: invert this member as |widget_exists_ = false;| (or optionally remove it, since it seems like |widget_| serves the same purpose) https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:291: // Re-show App Info but for non-primary profie. nit: 'profile' https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:293: std::unique_ptr<TestingProfile> other_profile(new TestingProfile); nit: base::MakeUnique https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:303: EXPECT_FALSE(dialog_->arc_app_info_links_); nit: add some explanation as to why the links aren't available here, ie. "The ARC App info links are not available to the secondary profile with ARC disabled."
Thank you for taking a look. Please find updated version. Thanks! https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc (right): https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:146: // When ARC is enabled and Settings app is available, show the On 2017/02/15 21:48:28, msw wrote: > nit: 'the Settings app' Done. https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:147: // "Manage supported links" link for Chrome. On 2017/02/15 21:48:28, msw wrote: > On 2017/02/15 16:38:45, khmel wrote: > > Problem was we asked for IsArcEnabled but that was associated with primary > > profile. Crash fix could be made by asking IsAllowedForProfile additionally > but > > there is one more case when Settings app is not available at this moment and > > showing "links" section has no sense. So combined solution is to ask > > ArcAppListPrefs for settings app that also covers secondary user profile case. > > Wouldn't the IsArcEnabled check here be incorrectly relying on Arc being enabled > for a different profile in the multi-profile case? (ie. current user has arc > enabled, but the 'primary'/other user has arc disabled, then this check won't > pass but it should?) If my guess is correct, please add a test case for that. > > Otherwise, if that's not the case, it might be slightly redundant, but perhaps > it still makes sense to explicitly check IsArcAllowedForProfile, instead on > relying on the settings app check to do that under the hood; wdyt? (aside: > update two unrelated comment mentions of IsAllowedForProfile to > IsArcAllowedForProfile) We don't allow ARC for secondary profile, so we have simpler case here. "IsArcAllowedForProfile" would fix the crash, but does not solve the problem when the Settings app is yet not available. (User test case: OptIn Arc, open Chrome app info and see the ARC link which does not work). The Settings app appears only after ARC is booted for first time fully. In case of secondary user arc_app_list_prefs is null and we check it here. https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_l... https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:153: arc_app_info_links_ = new ArcAppInfoLinksPanel(profile, app); On 2017/02/15 21:48:28, msw wrote: > optional nit: consider (1) removing this member and letting the tests infer the > presence of the links from the number of children held by this view? (perhaps > that's a little odd though...). Otherwise, consider (2) adding a public accessor > arc_app_info_links_for_test() and removing the friend test fixture declaration > or (3) adding a comment on the member declaration like "Checked by unit tests." > (it's just odd to see a private member set here and never checked in the class) All ideas look reasonable and I don't have any preferable. I was thinking about 1 and 2 during the impl but found that we have friend test declaration and followed that way. So, due small preference will do 2) :) https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc (right): https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:37: std::vector<arc::mojom::AppInfoPtr> GetArcSettingsAppMojoInfo() { On 2017/02/15 21:48:28, msw wrote: > It's a little odd to return a vector for one item, but I guess that's what the > caller needs... nit: GetArcSettingsAppInfo Done. https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:47: } // namespace On 2017/02/15 21:48:28, msw wrote: > nit: add a blank line above Done. https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:167: bool widget_destroyed_ = true; On 2017/02/15 21:48:28, msw wrote: > optional nit: invert this member as |widget_exists_ = false;| (or optionally > remove it, since it seems like |widget_| serves the same purpose) Good catch. removed. https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:291: // Re-show App Info but for non-primary profie. On 2017/02/15 21:48:28, msw wrote: > nit: 'profile' Done. https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:293: std::unique_ptr<TestingProfile> other_profile(new TestingProfile); On 2017/02/15 21:48:28, msw wrote: > nit: base::MakeUnique Done. https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:303: EXPECT_FALSE(dialog_->arc_app_info_links_); On 2017/02/15 21:48:28, msw wrote: > nit: add some explanation as to why the links aren't available here, ie. "The > ARC App info links are not available to the secondary profile with ARC > disabled." Thanks for ready-to-use proposal :)
lgtm with a nit and a minor comment. https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc (right): https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:147: // "Manage supported links" link for Chrome. On 2017/02/16 15:47:51, khmel wrote: > On 2017/02/15 21:48:28, msw wrote: > > On 2017/02/15 16:38:45, khmel wrote: > > > Problem was we asked for IsArcEnabled but that was associated with primary > > > profile. Crash fix could be made by asking IsAllowedForProfile additionally > > but > > > there is one more case when Settings app is not available at this moment and > > > showing "links" section has no sense. So combined solution is to ask > > > ArcAppListPrefs for settings app that also covers secondary user profile > case. > > > > Wouldn't the IsArcEnabled check here be incorrectly relying on Arc being > enabled > > for a different profile in the multi-profile case? (ie. current user has arc > > enabled, but the 'primary'/other user has arc disabled, then this check won't > > pass but it should?) If my guess is correct, please add a test case for that. > > > > Otherwise, if that's not the case, it might be slightly redundant, but perhaps > > it still makes sense to explicitly check IsArcAllowedForProfile, instead on > > relying on the settings app check to do that under the hood; wdyt? (aside: > > update two unrelated comment mentions of IsAllowedForProfile to > > IsArcAllowedForProfile) > > We don't allow ARC for secondary profile, so we have simpler case here. > "IsArcAllowedForProfile" would fix the crash, but does not solve the problem > when the Settings app is yet not available. (User test case: OptIn Arc, > open Chrome app info and see the ARC link which does not work). The Settings > app appears only after ARC is booted for first time fully. > In case of secondary user arc_app_list_prefs is null and we check it here. > https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_l... > Mightn't we someday allow Arc on either profile? I have a slight preference to *also* check IsArcAllowedForProfile, in addition to settings app availability (but perhaps instead of checking the profile-agnostic arc::ArcSessionManager::Get()->IsArcPlayStoreEnabled()), but either way seems okay for now, I guess. (It's more a question of how much work it will someday be to retrofit support for Arc on any profile... and making the intent of the check clear here) https://codereview.chromium.org/2700523002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc (right): https://codereview.chromium.org/2700523002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:298: // The ARC App info links are not available to the ARC is not allowed for nit: Fix the grammar here "to the ARC" -> "if ARC".
Thank you for review! https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc (right): https://codereview.chromium.org/2700523002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:147: // "Manage supported links" link for Chrome. On 2017/02/16 18:55:14, msw wrote: > On 2017/02/16 15:47:51, khmel wrote: > > On 2017/02/15 21:48:28, msw wrote: > > > On 2017/02/15 16:38:45, khmel wrote: > > > > Problem was we asked for IsArcEnabled but that was associated with primary > > > > profile. Crash fix could be made by asking IsAllowedForProfile > additionally > > > but > > > > there is one more case when Settings app is not available at this moment > and > > > > showing "links" section has no sense. So combined solution is to ask > > > > ArcAppListPrefs for settings app that also covers secondary user profile > > case. > > > > > > Wouldn't the IsArcEnabled check here be incorrectly relying on Arc being > > enabled > > > for a different profile in the multi-profile case? (ie. current user has arc > > > enabled, but the 'primary'/other user has arc disabled, then this check > won't > > > pass but it should?) If my guess is correct, please add a test case for > that. > > > > > > Otherwise, if that's not the case, it might be slightly redundant, but > perhaps > > > it still makes sense to explicitly check IsArcAllowedForProfile, instead on > > > relying on the settings app check to do that under the hood; wdyt? (aside: > > > update two unrelated comment mentions of IsAllowedForProfile to > > > IsArcAllowedForProfile) > > > > We don't allow ARC for secondary profile, so we have simpler case here. > > "IsArcAllowedForProfile" would fix the crash, but does not solve the problem > > when the Settings app is yet not available. (User test case: OptIn Arc, > > open Chrome app info and see the ARC link which does not work). The Settings > > app appears only after ARC is booted for first time fully. > > In case of secondary user arc_app_list_prefs is null and we check it here. > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_l... > > > > Mightn't we someday allow Arc on either profile? I have a slight preference to > *also* check IsArcAllowedForProfile, in addition to settings app availability > (but perhaps instead of checking the profile-agnostic > arc::ArcSessionManager::Get()->IsArcPlayStoreEnabled()), but either way seems > okay for now, I guess. (It's more a question of how much work it will someday be > to retrofit support for Arc on any profile... and making the intent of the check > clear here) Actually there is currently a project (probably after M60) to make ARC running always in order to get access to system Android app (user will still need OptIn ARC in order to get access to Play Store apps). In these terms IsArcAllowedForProfile will be deprecated and probably changed to IsPlayStoreAllowedForProfile. However there is no clear design so far. From this perspective working using ArcAppListPrefs is preferable because all complex logic will be hidden in that class. Technically, once that project is done we might have workable solution here automatically :) assuming that the Settings app is system app which does not require Play Store. https://codereview.chromium.org/2700523002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc (right): https://codereview.chromium.org/2700523002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:298: // The ARC App info links are not available to the ARC is not allowed for On 2017/02/16 18:55:14, msw wrote: > nit: Fix the grammar here "to the ARC" -> "if ARC". Done.
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2700523002/#ps60001 (title: "minor comment update")
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": 60001, "attempt_start_ts": 1487277285674990, "parent_rev": "e9cf581d97cce0939cc6ce4f1a60767a8a28602f", "commit_rev": "b756297e51407c3b602c6d65746a8f18d835d71b"}
Message was sent while issue was closed.
Description was changed from ========== arc: Fix crash on accessing app info for secondary user. This fix the crash whem user activates Chrome app info page for secondary user profile. This alse adds unit test that cover this case and generic functionality. TEST=Manually, unit tests extended BUG=689263 ========== to ========== arc: Fix crash on accessing app info for secondary user. This fix the crash whem user activates Chrome app info page for secondary user profile. This alse adds unit test that cover this case and generic functionality. TEST=Manually, unit tests extended BUG=689263 Review-Url: https://codereview.chromium.org/2700523002 Cr-Commit-Position: refs/heads/master@{#451118} Committed: https://chromium.googlesource.com/chromium/src/+/b756297e51407c3b602c6d65746a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b756297e51407c3b602c6d65746a... |