Show notification instead of opening app launcher after extension installation

Show notification instead of opening app launcher after extension installation

This patch suppresses opening the launcher and show app-installed notifications instead after user installs apps from CWS.

Even with this patch, the app launcher is opened to show the installation progress. It'll be suppressed in the separated patch,

BUG=606843
TEST=manual tested

Committed: https://crrev.com/fd0c9080b93a56883be8eda1de4f09420422966a
Cr-Commit-Position: refs/heads/master@{#397081}

[yoshiki, 2016-05-19 20:04:47]

Patchset #1 (id:1) has been deleted

[yoshiki, 2016-05-19 20:04:54]

Patchset #1 (id:20001) has been deleted

[yoshiki, 2016-05-19 20:08:23]

Description was changed from

==========
.

BUG=
==========

to

==========
Show notification instead of opening app launcher after extension installation

This patch suppresses opening the launcher and show app-installed notifications
instead after user installs apps from CWS.

Even with this patch, the app launcher is opened to show the installation
progress. It'll be suppressed in the separated patch,

BUG=606843
TEST=manual tested
==========

[yoshiki, 2016-05-19 20:08:23]

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

[yoshiki, 2016-05-19 20:08:43]

Benwells, PTAL. Thanks.

[benwells, 2016-05-20 00:17:29]

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_installed_notification.cc (right):

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_installed_notification.cc:26: const char*
kNotificationOriginUrl = "http://origin/";
What is this all about? This seems really weird.

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_installed_notification.cc:39: // by
ancestor NotificationDelegate, or be released soon by myself.
You probably shouldn't use 'myself' in a comment.

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_installed_notification.cc:46: 
Nit: no blank line here

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_installed_notification.cc:55:
ash::system_notifier::kNotifierScreenshot),
What's this got to do with screenshots?

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_installed_notification.cc:69:
NEW_FOREGROUND_TAB, extensions::SOURCE_TEST);
why SOURCE_TEST?

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_installed_notification.h (right):

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_installed_notification.h:31:
scoped_refptr<const extensions::Extension> extension_;
I think holding on to a refptr to extension is a bad idea. While it is ref
counted, most things don't use it that way.

A safer approach is to grab the data you need off the extension pointer in
construction (maybe just id) and then look up the information later.

[yoshiki, 2016-05-24 16:13:01]

Patchset #2 (id:60001) has been deleted

[yoshiki, 2016-05-24 16:23:18]

Patchset #2 (id:80001) has been deleted

[yoshiki, 2016-05-24 16:25:18]

Sorry I forgot to upload the latest patchset so some tentative code remained. I
fixed and addressed your comment. PTAL again. Thanks.

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_installed_notification.cc (right):

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_installed_notification.cc:26: const char*
kNotificationOriginUrl = "http://origin/";
On 2016/05/20 00:17:29, benwells wrote:
> What is this all about? This seems really weird.

Sorry I forgot to upload the latest patchset. Fixed.

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_installed_notification.cc:39: // by
ancestor NotificationDelegate, or be released soon by myself.
On 2016/05/20 00:17:29, benwells wrote:
> You probably shouldn't use 'myself' in a comment.

Done.

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_installed_notification.cc:46: 
On 2016/05/20 00:17:29, benwells wrote:
> Nit: no blank line here

Done.

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_installed_notification.cc:55:
ash::system_notifier::kNotifierScreenshot),
On 2016/05/20 00:17:29, benwells wrote:
> What's this got to do with screenshots?

Fixed.

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_installed_notification.cc:69:
NEW_FOREGROUND_TAB, extensions::SOURCE_TEST);
On 2016/05/20 00:17:29, benwells wrote:
> why SOURCE_TEST?

Fixed.

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_installed_notification.h (right):

https://codereview.chromium.org/1993583002/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_installed_notification.h:31:
scoped_refptr<const extensions::Extension> extension_;
On 2016/05/20 00:17:29, benwells wrote:
> I think holding on to a refptr to extension is a bad idea. While it is ref
> counted, most things don't use it that way.
> 
> A safer approach is to grab the data you need off the extension pointer in
> construction (maybe just id) and then look up the information later.

Thank you for guidance. Done.

[benwells, 2016-05-25 05:31:31]

https://codereview.chromium.org/1993583002/diff/100001/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/extension_installed_notification.cc (right):

https://codereview.chromium.org/1993583002/diff/100001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_installed_notification.cc:22: #include
"ui/message_center/notification.h"
Nit: This is a lot of includes, are these all needed?

https://codereview.chromium.org/1993583002/diff/100001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_installed_notification.cc:27: }  //
anonymouse namespace
Nit: no rodents please!

https://codereview.chromium.org/1993583002/diff/100001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_installed_notification.cc:38: // itself
(ancestor NotificationDelegate actually).
Nit: Please rephrase this as: "It's lifetime is managed by the parent class
NotificationDelegate." (or something more grammatically correct).

https://codereview.chromium.org/1993583002/diff/100001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_installed_notification.cc:66: const
extensions::Extension* extension =
Nit: IWYU, please add relevant headers for this.

[yoshiki, 2016-05-27 05:42:47]

Patchset #4 (id:140001) has been deleted

[yoshiki, 2016-05-27 05:43:04]

Patchset #3 (id:120001) has been deleted

[yoshiki, 2016-05-30 06:56:01]

benwells@, PTAL. Thanks.

https://codereview.chromium.org/1993583002/diff/100001/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/extension_installed_notification.cc (right):

https://codereview.chromium.org/1993583002/diff/100001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_installed_notification.cc:22: #include
"ui/message_center/notification.h"
On 2016/05/25 05:31:31, benwells wrote:
> Nit: This is a lot of includes, are these all needed?

I checked the includes and removed/replaced unnecessary ones.

https://codereview.chromium.org/1993583002/diff/100001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_installed_notification.cc:27: }  //
anonymouse namespace
On 2016/05/25 05:31:31, benwells wrote:
> Nit: no rodents please!

Ahh, Fixed.

https://codereview.chromium.org/1993583002/diff/100001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_installed_notification.cc:38: // itself
(ancestor NotificationDelegate actually).
On 2016/05/25 05:31:31, benwells wrote:
> Nit: Please rephrase this as: "It's lifetime is managed by the parent class
> NotificationDelegate." (or something more grammatically correct).

Thank you for guidance. Done.

https://codereview.chromium.org/1993583002/diff/100001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_installed_notification.cc:66: const
extensions::Extension* extension =
On 2016/05/25 05:31:31, benwells wrote:
> Nit: IWYU, please add relevant headers for this.

Done.

[benwells, 2016-05-30 07:19:04]

lgtm

[yoshiki, 2016-05-30 07:27:29]

yoshiki@chromium.org changed reviewers:
+ asvitkine@google.com, oshima@chromium.org

[yoshiki, 2016-05-30 07:27:30]

oshima@, PTAL at added resources in chrome/app/theme/?

asvitkine@, PTAL at the histograms.xml change in metrics?

[Alexei Svitkine (slow), 2016-05-30 14:41:03]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-05-30 14:41:04]

histograms lgtm

[oshima, 2016-05-31 23:31:37]

lgtm

If you can use vector icon,that'd be better though.

[yoshiki, 2016-06-01 06:46:39]

On 2016/05/31 23:31:37, oshima wrote:
> lgtm
> 
> If you can use vector icon,that'd be better though.

Let me ask the UX and do it in separate CL. Thanks.

[yoshiki, 2016-06-01 06:46:44]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2016-06-01 06:46:58]

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

[commit-bot: I haz the power, 2016-06-01 08:14:57]

Description was changed from

==========
Show notification instead of opening app launcher after extension installation

This patch suppresses opening the launcher and show app-installed notifications
instead after user installs apps from CWS.

Even with this patch, the app launcher is opened to show the installation
progress. It'll be suppressed in the separated patch,

BUG=606843
TEST=manual tested
==========

to

==========
Show notification instead of opening app launcher after extension installation

This patch suppresses opening the launcher and show app-installed notifications
instead after user installs apps from CWS.

Even with this patch, the app launcher is opened to show the installation
progress. It'll be suppressed in the separated patch,

BUG=606843
TEST=manual tested
==========

[commit-bot: I haz the power, 2016-06-01 08:14:58]

Committed patchset #3 (id:160001)

[commit-bot: I haz the power, 2016-06-01 08:16:36]

Description was changed from

==========
Show notification instead of opening app launcher after extension installation

This patch suppresses opening the launcher and show app-installed notifications
instead after user installs apps from CWS.

Even with this patch, the app launcher is opened to show the installation
progress. It'll be suppressed in the separated patch,

BUG=606843
TEST=manual tested
==========

to

==========
Show notification instead of opening app launcher after extension installation

This patch suppresses opening the launcher and show app-installed notifications
instead after user installs apps from CWS.

Even with this patch, the app launcher is opened to show the installation
progress. It'll be suppressed in the separated patch,

BUG=606843
TEST=manual tested

Committed: https://crrev.com/fd0c9080b93a56883be8eda1de4f09420422966a
Cr-Commit-Position: refs/heads/master@{#397081}
==========

[commit-bot: I haz the power, 2016-06-01 08:16:38]

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