arc: Fix crash on accessing app info for secondary user.

chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc

Index: chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
diff --git a/chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc b/chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
index 1654dc0405fd0654f7c8ce94d6d3b1e9c5960e8e..320e6ac13636dcc44a269e3b302b29395009672e 100644
--- a/chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
+++ b/chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
@@ -35,6 +35,8 @@
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/arc/arc_session_manager.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
 #include "chrome/browser/ui/views/apps/app_info_dialog/arc_app_info_links_panel.h"
 #endif
 
@@ -110,12 +112,7 @@ void ShowAppInfoInNativeDialog(content::WebContents* web_contents,
 AppInfoDialog::AppInfoDialog(gfx::NativeWindow parent_window,
                              Profile* profile,
                              const extensions::Extension* app)
-    : dialog_header_(NULL),
-      dialog_body_(NULL),
-      dialog_footer_(NULL),
-      profile_(profile),
-      app_id_(app->id()),
-      extension_registry_(NULL) {
+    : profile_(profile), app_id_(app->id()) {
   views::BoxLayout* layout =
       new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0);
   SetLayoutManager(layout);
@@ -146,10 +143,17 @@ AppInfoDialog::AppInfoDialog(gfx::NativeWindow parent_window,
   dialog_body_contents->AddChildView(new AppInfoPermissionsPanel(profile, app));
 
 #if defined(OS_CHROMEOS)
-  // When ARC is enabled, show the "Manage supported links" link for Chrome.
-  if (arc::ArcSessionManager::Get()->IsArcEnabled() &&
-      app->id() == extension_misc::kChromeAppId)
-    dialog_body_contents->AddChildView(new ArcAppInfoLinksPanel(profile, app));
+  // When ARC is enabled and Settings app is available, show the

[msw, 2017/02/15 21:48:28]

nit: 'the Settings app'

[khmel, 2017/02/16 15:47:51]

Done.

+  // "Manage supported links" link for Chrome.

[khmel, 2017/02/15 16:38:45]

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.

[msw, 2017/02/15 21:48:28]

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)

[khmel, 2017/02/16 15:47:51]

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...

[msw, 2017/02/16 18:55:14]

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)

[khmel, 2017/02/16 20:33:29]

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.

+  if (app->id() == extension_misc::kChromeAppId &&
+      arc::ArcSessionManager::Get()->IsArcEnabled()) {
+    const ArcAppListPrefs* arc_app_list_prefs = ArcAppListPrefs::Get(profile);
+    if (arc_app_list_prefs &&
+        arc_app_list_prefs->IsRegistered(arc::kSettingsAppId)) {
+      arc_app_info_links_ = new ArcAppInfoLinksPanel(profile, app);

[msw, 2017/02/15 21:48:28]

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)

[khmel, 2017/02/16 15:47:51]

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) :)

+      dialog_body_contents->AddChildView(arc_app_info_links_);
+    }
+  }
 #endif
 
   // Clip the scrollable view so that the scrollbar appears. As long as this