Add a flag that lets the webstore show a different UI on app install.

Add a flag that lets the webstore show a different UI on app install.

When people install apps, they seem to get confused about how to launch them.
We want to experiment with a different UI after install, that instead of 
immediately transitioning to the New Tab Page, instead shows a bubble pointing
at the New Tab button on the tabstrip with a "show me" link which will open a
new tab and animate the app icon showing up there.

This CL has the views implementation - OSX and GTK implementations will come in
a separate CL.

BUG=89687
TEST=Requires webstore changes to fully test (the CL includes an automated
browser test)


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95514

[asargent_no_longer_on_chrome, 2011-08-03 22:02:21]

sky: chrome/browser/ui/*

mpcomplete: everything else

[Matt Perry, 2011-08-03 22:19:26]

LGTM

http://codereview.chromium.org/7529011/diff/13002/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7529011/diff/13002/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:74: const char kAppsNewInstallBubble[]       =
"apps-new-install-bubble";
align the =

http://codereview.chromium.org/7529011/diff/13002/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/webstore_private/app_install_bubble.js
(right):

http://codereview.chromium.org/7529011/diff/13002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webstore_private/app_install_bubble.js:16:
function(result) {
I think you want to wrap this callback in a callbackPass.

[Matt Perry, 2011-08-03 22:19:26]

LGTM

[sky, 2011-08-03 22:23:24]

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip.h (right):

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip.h:37: virtual views::View*
GetNewTabButton() OVERRIDE { return NULL; }
Don't inline virtual methods.

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble.cc (right):

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble.cc:119: if (type_
== ExtensionInstalledBubble::PAGE_ACTION) {
nit: seems weird to have three separate ifs for the same field. Is it a time for
a switch followed by type_ != APP at the end?

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble.cc:183: };
nit: no ;

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble.cc:250:
std::string extension_id_;
nit: const

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble.cc:333:
AbstractTabStripView* tabstrip = browser_view->tabstrip();
There are two cases that are probably worth checking here:
. Is the tabstrip visible? Use browser_view->IsTabStripVisible() to test that.
This is false for things like popups/apps.
. Is the new tab button actually showing (IsVisibleInRootView). Things like
compact nav can make this false.

I don't know what you should do if either of these are false.

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/side_tab_strip.h (right):

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/side_tab_strip.h:26: virtual views::View*
GetNewTabButton() OVERRIDE { return newtab_button_; }
Don't inline virtual methods.

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_strip.h (right):

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_strip.h:57: virtual views::View*
GetNewTabButton() OVERRIDE { return newtab_button_; }
Don't inline virtual methods.

[asargent_no_longer_on_chrome, 2011-08-04 00:00:16]

Addressed all comments. 

sky: please look again.

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip.h (right):

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip.h:37: virtual views::View*
GetNewTabButton() OVERRIDE { return NULL; }
On 2011/08/03 22:23:24, sky wrote:
> Don't inline virtual methods.

Done.

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble.cc (right):

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble.cc:119: if (type_
== ExtensionInstalledBubble::PAGE_ACTION) {
On 2011/08/03 22:23:24, sky wrote:
> nit: seems weird to have three separate ifs for the same field. Is it a time
for
> a switch followed by type_ != APP at the end?

Good suggestion - that looks a little cleaner.

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble.cc:183: };
On 2011/08/03 22:23:24, sky wrote:
> nit: no ;

oops! Done.

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble.cc:250:
std::string extension_id_;
On 2011/08/03 22:23:24, sky wrote:
> nit: const

Done.

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble.cc:333:
AbstractTabStripView* tabstrip = browser_view->tabstrip();
On 2011/08/03 22:23:24, sky wrote:
> There are two cases that are probably worth checking here:
> . Is the tabstrip visible? Use browser_view->IsTabStripVisible() to test that.
> This is false for things like popups/apps.
> . Is the new tab button actually showing (IsVisibleInRootView). Things like
> compact nav can make this false.
> 
> I don't know what you should do if either of these are false.

Thanks for pointing this out! I've modified the CL so that if the tab strip
isn't visible, we just fall back to the same general reference view behavior as
extension installs with no UI component (ie no page/browser action, omnibox
hook, etc.) which is to point the bubble at the wrench menu. And if the tab
strip is visible but the new tab button isn't, I have the bubble point at the
tab strip.

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/side_tab_strip.h (right):

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/side_tab_strip.h:26: virtual views::View*
GetNewTabButton() OVERRIDE { return newtab_button_; }
On 2011/08/03 22:23:24, sky wrote:
> Don't inline virtual methods.

Done.

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_strip.h (right):

http://codereview.chromium.org/7529011/diff/13002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_strip.h:57: virtual views::View*
GetNewTabButton() OVERRIDE { return newtab_button_; }
On 2011/08/03 22:23:24, sky wrote:
> Don't inline virtual methods.

Done.

http://codereview.chromium.org/7529011/diff/13002/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7529011/diff/13002/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:74: const char kAppsNewInstallBubble[]       =
"apps-new-install-bubble";
On 2011/08/03 22:19:26, Matt Perry wrote:
> align the =

Done.

http://codereview.chromium.org/7529011/diff/13002/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/webstore_private/app_install_bubble.js
(right):

http://codereview.chromium.org/7529011/diff/13002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webstore_private/app_install_bubble.js:16:
function(result) {
On 2011/08/03 22:19:26, Matt Perry wrote:
> I think you want to wrap this callback in a callbackPass.
Good catch, done. (I also removed the assertNoLastError because callbackPass
does that for me.)

[sky, 2011-08-04 03:40:14]

LGTM

[commit-bot: I haz the power, 2011-08-04 22:12:51]

Change committed as 95514