NTP: Use the store as the last thumbnail in case we have no apps installed....

NTP: Use the store as the last thumbnail in case we have no apps installed.

1. If no apps are installed we add an item for the store to the most visited section.
2. No longer hide the most visited by default.
3. This removes the default apps on non ChromeOS

BUG=46028, 47700
TEST=With a clean profile without any apps installed you should see the store thumbnail unless you blacklist it or pin 8 ohter pages.


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

[arv (Not doing code reviews), 2010-07-15 17:15:07]

ping

[Aaron Boodman, 2010-07-15 18:35:19]

Can you also attach some screen shots of what it looks like in the various
configurations?

http://codereview.chromium.org/3001003/diff/14001/15004
File chrome/browser/dom_ui/most_visited_handler.cc (right):

http://codereview.chromium.org/3001003/diff/14001/15004#newcode367
chrome/browser/dom_ui/most_visited_handler.cc:367: bool add_chrome_store =
!HasApps();
I think HasApps() should be in ExtensionsService so that others can use it.

http://codereview.chromium.org/3001003/diff/14001/15004#newcode524
chrome/browser/dom_ui/most_visited_handler.cc:524:
GURL("chrome://theme/IDR_NEWTAB_CHROME_STORE_PAGE_FAVICON")};
The image here is supposed to be the webstore icon, not a thumbnail of the page.
The same webstore icon we use in the case where there are apps.

http://codereview.chromium.org/3001003/diff/14001/15009
File chrome/browser/extensions/extensions_service.cc (left):

http://codereview.chromium.org/3001003/diff/14001/15009#oldcode112
chrome/browser/extensions/extensions_service.cc:112:
CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
Why are you changing this?

http://codereview.chromium.org/3001003/diff/14001/15009#oldcode136
chrome/browser/extensions/extensions_service.cc:136: // Allow command line
gallery url to be referrer for the gallery downloads.
Why are you changing this?

[arv (Not doing code reviews), 2010-07-15 19:04:18]

http://codereview.chromium.org/3001003/diff/14001/15009
File chrome/browser/extensions/extensions_service.cc (left):

http://codereview.chromium.org/3001003/diff/14001/15009#oldcode112
chrome/browser/extensions/extensions_service.cc:112:
CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
On 2010/07/15 18:35:19, Aaron Boodman wrote:
> Why are you changing this?

This was used to determine whether to show the Chrome Store link before, wasn't
it?

[arv (Not doing code reviews), 2010-07-15 19:16:59]

On Thu, Jul 15, 2010 at 11:35,  <aa@chromium.org> wrote:
> Can you also attach some screen shots of what it looks like in the various
> configurations?

New profile

http://twitpic.com/25kaqx/full

After clicking on the "Chrome Web Store" and installing an app. The
"Chrome Store" no longer shows up because we have an app installed.
The "Chrome Extension Gallery" shows up because it is one of the most
visited pages.

http://twitpic.com/25kbiw/full

If navigate a lot of navigation and uninstall the single app the
"Chrome Store" thumbnail is replaces the last thumbnail.


>
>
> http://codereview.chromium.org/3001003/diff/14001/15004
> File chrome/browser/dom_ui/most_visited_handler.cc (right):
>
> http://codereview.chromium.org/3001003/diff/14001/15004#newcode367
> chrome/browser/dom_ui/most_visited_handler.cc:367: bool add_chrome_store
> = !HasApps();
> I think HasApps() should be in ExtensionsService so that others can use
> it.
>
> http://codereview.chromium.org/3001003/diff/14001/15004#newcode524
> chrome/browser/dom_ui/most_visited_handler.cc:524:
> GURL("chrome://theme/IDR_NEWTAB_CHROME_STORE_PAGE_FAVICON")};
> The image here is supposed to be the webstore icon, not a thumbnail of
> the page. The same webstore icon we use in the case where there are
> apps.
>
> http://codereview.chromium.org/3001003/diff/14001/15009
> File chrome/browser/extensions/extensions_service.cc (left):
>
> http://codereview.chromium.org/3001003/diff/14001/15009#oldcode112
> chrome/browser/extensions/extensions_service.cc:112:
> CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
> Why are you changing this?
>
> http://codereview.chromium.org/3001003/diff/14001/15009#oldcode136
> chrome/browser/extensions/extensions_service.cc:136: // Allow command
> line gallery url to be referrer for the gallery downloads.
> Why are you changing this?
>
> http://codereview.chromium.org/3001003/show
>

[Aaron Boodman, 2010-07-15 20:19:26]

http://codereview.chromium.org/3001003/diff/14001/15009
File chrome/browser/extensions/extensions_service.cc (left):

http://codereview.chromium.org/3001003/diff/14001/15009#oldcode112
chrome/browser/extensions/extensions_service.cc:112:
CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
On 2010/07/15 19:04:18, arv wrote:
> On 2010/07/15 18:35:19, Aaron Boodman wrote:
> > Why are you changing this?
> 
> This was used to determine whether to show the Chrome Store link before,
wasn't
> it?

These two functions are used in many places (for example to decide on the exact
installation presentation, depending on the source URL). They should not be
changed.

The kAppsGalleryURL switch should stay and all the logic that depends on it
should stay.

[arv (Not doing code reviews), 2010-07-16 19:15:29]

ptal

[Aaron Boodman, 2010-07-16 19:31:30]

I still think that HasApps() should move to ExtensionsService so others can use
it. Other than that LGTM.

[arv (Not doing code reviews), 2010-07-16 20:26:14]

On Fri, Jul 16, 2010 at 12:31,  <aa@chromium.org> wrote:
> I still think that HasApps() should move to ExtensionsService so others can
> use
> it. Other than that LGTM.

Yeah, me too, I had to run to lunch and forgot.

-- 
erik