arc: Prevent showing Opt-in UI when it is not expected.

arc: Prevent showing Opt-in UI when it is not expected.

BUG=606097
TEST=Manually on device. Start OptIn workflow and pin
     (before pin disabled for ArcSupport window)  OptIn
     window into the shelf. Once OptIn workflow is done
     click on shelf icon. There is no more poping window.
TEST=Manually on device. No pin menu for Arc Support app.
TEST=unit_tests passes.

Committed: https://crrev.com/4b24e4d7ca3181499787ae2dfc4f97ea48e0326c
Cr-Commit-Position: refs/heads/master@{#390312}

[khmel, 2016-04-23 00:34:39]

khmel@chromium.org changed reviewers:
+ xiyuan@chromium.org

[khmel, 2016-04-23 00:34:40]

PTAL

https://codereview.chromium.org/1919593003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/1919593003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:413: if
(!disable_ui_for_testing)
I have one more question. There is an idea to prevent pinning this app at all.
Do you know if any legacy functionality for this? I tried to find but seems it
is not available. If not I would say it is too hack-y to implement special for
this case.

[khmel, 2016-04-23 00:34:41]

khmel@chromium.org changed reviewers:
+ xiyuan@chromium.org

[khmel, 2016-04-23 00:34:41]

PTAL

[xiyuan, 2016-04-25 21:14:35]

https://codereview.chromium.org/1919593003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/1919593003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:413: if
(!disable_ui_for_testing)
On 2016/04/23 00:34:40, khmel wrote:
> I have one more question. There is an idea to prevent pinning this app at all.
> Do you know if any legacy functionality for this? I tried to find but seems it
> is not available. If not I would say it is too hack-y to implement special for
> this case. 

Have you checked AppListControllerDelegateAsh::GetPinnable[1]? Maybe we should
check also check Extension::ShouldDisplayInAppLauncher and returns NO_PIN if the
exntension does not display in launcher?

[1]:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[khmel, 2016-04-26 16:37:17]

PTAL

https://codereview.chromium.org/1919593003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/1919593003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:413: if
(!disable_ui_for_testing)
On 2016/04/25 21:14:35, xiyuan wrote:
> On 2016/04/23 00:34:40, khmel wrote:
> > I have one more question. There is an idea to prevent pinning this app at
all.
> > Do you know if any legacy functionality for this? I tried to find but seems
it
> > is not available. If not I would say it is too hack-y to implement special
for
> > this case. 
> 
> Have you checked AppListControllerDelegateAsh::GetPinnable[1]? Maybe we should
> check also check Extension::ShouldDisplayInAppLauncher and returns NO_PIN if
the
> exntension does not display in launcher?
> 
> [1]:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

This might be good point to insert this check.

[xiyuan, 2016-04-26 17:15:25]

https://codereview.chromium.org/1919593003/diff/20001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.cc (right):

https://codereview.chromium.org/1919593003/diff/20001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:27: bool
ShouldDisplayInAppLauncher(const std::string& extension_id) {
nit: ShouldDisplayInAppLauncher -> CanPin since it checks whether the extension
is not allowed to be pinned rather than check the exteions's
ShouldDisplayInAppLauncher.

https://codereview.chromium.org/1919593003/diff/20001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:76: return
ChromeLauncherController::instance()->CanPin(extension_id)
I wonder whether we should change ChromeLauncherController::CanPin to GetPinable
and move the blacklist check there. And this function can just return
ChromeLauncherController::GetPinnable().

LauncherContextMenu holds a reference to ChromeLauncherController. The CL now
adds a reference to AppListControllerDelegate, call it to check the blacklist
then call back to ChromeLauncherController. It feels a bit unnecessary.

WDYT?

[khmel, 2016-04-26 20:35:39]

Description was changed from

==========
arc: Prevent showing Opt-in UI when it is not expected.

BUG=606097
TEST=Manually on device. Start OptIn workflow and pin OptIn
     window into the shelf. Once OptIn workflow is done click
     on shelf icon. There is no more poping window.
==========

to

==========
arc: Prevent showing Opt-in UI when it is not expected.

BUG=606097
TEST=Manually on device. Start OptIn workflow and pin
     (before pin disabled for ArcSupport window)  OptIn
     window into the shelf. Once OptIn workflow is done
     click on shelf icon. There is no more poping window.
TEST=Manually on device. No pin menu for Arc Support app.
TEST=unit_tests passes.
==========

[khmel, 2016-04-26 20:38:37]

Refactore to use GetPinnable()

PTAL

https://codereview.chromium.org/1919593003/diff/20001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.cc (right):

https://codereview.chromium.org/1919593003/diff/20001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:27: bool
ShouldDisplayInAppLauncher(const std::string& extension_id) {
On 2016/04/26 17:15:25, xiyuan wrote:
> nit: ShouldDisplayInAppLauncher -> CanPin since it checks whether the
extension
> is not allowed to be pinned rather than check the exteions's
> ShouldDisplayInAppLauncher.

Done.

https://codereview.chromium.org/1919593003/diff/20001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:76: return
ChromeLauncherController::instance()->CanPin(extension_id)
On 2016/04/26 17:15:25, xiyuan wrote:
> I wonder whether we should change ChromeLauncherController::CanPin to
GetPinable
> and move the blacklist check there. And this function can just return
> ChromeLauncherController::GetPinnable().
> 
> LauncherContextMenu holds a reference to ChromeLauncherController. The CL now
> adds a reference to AppListControllerDelegate, call it to check the blacklist
> then call back to ChromeLauncherController. It feels a bit unnecessary.
> 
> WDYT?

Yes, I had the same feeling :)

[xiyuan, 2016-04-26 21:00:23]

lgtm

Cool. I like this version.

https://codereview.chromium.org/1919593003/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/1919593003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:385:
AppListControllerDelegate::Pinnable GetPinnable(const std::string& app_id);
nit: We might want find a better home for Pinnable. It is a bit strange like
this. Maybe add a TODO for that?

[khmel, 2016-04-26 21:11:30]

khmel@chromium.org changed reviewers:
+ sashab@chromium.org, skuhne@chromium.org

[khmel, 2016-04-26 21:11:31]

Thanks Xiyuan for review,

Adding Stefan for ash/launcher
and Sasha for app_info_dialog

PTAL

https://codereview.chromium.org/1919593003/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/1919593003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:385:
AppListControllerDelegate::Pinnable GetPinnable(const std::string& app_id);
On 2016/04/26 21:00:23, xiyuan wrote:
> nit: We might want find a better home for Pinnable. It is a bit strange like
> this. Maybe add a TODO for that?

Right, it looks a little odd to have it here.

[Mr4D (OOO till 08-26), 2016-04-26 21:38:56]

lgtm

[khmel, 2016-04-27 18:00:17]

khmel@chromium.org changed reviewers:
+ benwells@chromium.org

[khmel, 2016-04-27 18:00:17]

Hi Ben,

Could you please take a look into app_info_dialog code?

PTAL

BTW,

I heard that Sasha is no longer working on this stuff. Would it make sense to
update chrome/browser/ui/views/apps/app_info_dialog/OWNERS?

[benwells, 2016-04-28 03:45:22]

lgtm

[khmel, 2016-04-28 05:14:41]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-04-28 05:14:42]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/1919593003/#ps60001
(title: "TODO added")

[commit-bot: I haz the power, 2016-04-28 05:14:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1919593003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1919593003/60001

[commit-bot: I haz the power, 2016-04-28 05:56:55]

Description was changed from

==========
arc: Prevent showing Opt-in UI when it is not expected.

BUG=606097
TEST=Manually on device. Start OptIn workflow and pin
     (before pin disabled for ArcSupport window)  OptIn
     window into the shelf. Once OptIn workflow is done
     click on shelf icon. There is no more poping window.
TEST=Manually on device. No pin menu for Arc Support app.
TEST=unit_tests passes.
==========

to

==========
arc: Prevent showing Opt-in UI when it is not expected.

BUG=606097
TEST=Manually on device. Start OptIn workflow and pin
     (before pin disabled for ArcSupport window)  OptIn
     window into the shelf. Once OptIn workflow is done
     click on shelf icon. There is no more poping window.
TEST=Manually on device. No pin menu for Arc Support app.
TEST=unit_tests passes.
==========

[commit-bot: I haz the power, 2016-04-28 05:56:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-30 17:16:33]

Patchset 4 (id:??) landed as
https://crrev.com/4b24e4d7ca3181499787ae2dfc4f97ea48e0326c
Cr-Commit-Position: refs/heads/master@{#390312}

[commit-bot: I haz the power, 2016-04-30 17:54:25]

Description was changed from

==========
arc: Prevent showing Opt-in UI when it is not expected.

BUG=606097
TEST=Manually on device. Start OptIn workflow and pin
     (before pin disabled for ArcSupport window)  OptIn
     window into the shelf. Once OptIn workflow is done
     click on shelf icon. There is no more poping window.
TEST=Manually on device. No pin menu for Arc Support app.
TEST=unit_tests passes.
==========

to

==========
arc: Prevent showing Opt-in UI when it is not expected.

BUG=606097
TEST=Manually on device. Start OptIn workflow and pin
     (before pin disabled for ArcSupport window)  OptIn
     window into the shelf. Once OptIn workflow is done
     click on shelf icon. There is no more poping window.
TEST=Manually on device. No pin menu for Arc Support app.
TEST=unit_tests passes.

Committed: https://crrev.com/4b24e4d7ca3181499787ae2dfc4f97ea48e0326c
Cr-Commit-Position: refs/heads/master@{#390312}
==========