ntp4: default to 18 apps per page for new installs

ntp4: default to 18 apps per page for new installs

BUG=none
TEST=manual

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

[Evan Stade, 2011-08-25 01:10:15]

rbyers for app_launcher_handler, Matt for extensions

[Rick Byers, 2011-08-25 02:51:06]

app_launcher_handler.cc LGTM

I like that you're avoiding the ugly '-2' hack you were previously going to
use...

[Evan Stade, 2011-08-25 04:07:52]

On 2011/08/25 02:51:06, Rick Byers wrote:
> app_launcher_handler.cc LGTM
> 
> I like that you're avoiding the ugly '-2' hack you were previously going to
> use...

no, I am still planning to use that hack.

[Rick Byers, 2011-08-25 14:38:03]

On Thu, Aug 25, 2011 at 12:07 AM, <estade@chromium.org> wrote:

> On 2011/08/25 02:51:06, Rick Byers wrote:
>
>> app_launcher_handler.cc LGTM
>>
>
>  I like that you're avoiding the ugly '-2' hack you were previously going
>> to
>> use...
>>
>
> no, I am still planning to use that hack.
>

Oh sorry, I see - the -2 hack is for getting it first on the page, and this
logic is for getting it to be on the first page.  Now that the web store is
more like a regular app, it seems a shame to have to special case it in so
many places.   Oh well, I don't see an obvious way of simplifying this.  At
least this is all conceptually temporary (for migrating profiles from older
versions of Chrome), right?  Maybe we can remove most of the special
treatment of the webstore in some future version...

[Evan Stade, 2011-08-25 16:59:21]

it's for migration, yea. It's also for new chrome users, although if it were
only for new chrome users and not also migration then I would probably find a
better place to put the webstore special-casing.

[Matt Perry, 2011-08-25 17:59:35]

LGTM

http://codereview.chromium.org/7717012/diff/1011/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7717012/diff/1011/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2234: // extension has an index
(page or in-page), set it to -1.
out of curiosity, why do we care whether a non-app has a page or in-page index?

[Mihai Parparita -not on Chrome, 2011-08-25 18:27:08]

http://codereview.chromium.org/7717012/diff/1011/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/7717012/diff/1011/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:104: // If |page_index| is -1, and
the then a page will be found for the App.
Nit: "and the then" doesn't sound quite right and "App" probably shouldn't be
capitalized.

[Evan Stade, 2011-08-25 18:50:49]

http://codereview.chromium.org/7717012/diff/1011/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/7717012/diff/1011/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:104: // If |page_index| is -1, and
the then a page will be found for the App.
On 2011/08/25 18:27:08, Mihai Parparita wrote:
> Nit: "and the then" doesn't sound quite right and "App" probably shouldn't be
> capitalized.

you're right on both counts.

http://codereview.chromium.org/7717012/diff/1011/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7717012/diff/1011/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2234: // extension has an index
(page or in-page), set it to -1.
On 2011/08/25 17:59:35, Matt Perry wrote:
> out of curiosity, why do we care whether a non-app has a page or in-page
index?

because we need to be able to count the number of apps on a given page. We could
leave out the extensions when counting apps per page, but AFAICT ExtensionPrefs
doesn't have a way of knowing whether a given extension is an app. If there's
something I'm missing I'll happily change this.

[Matt Perry, 2011-08-25 18:52:02]

http://codereview.chromium.org/7717012/diff/1011/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7717012/diff/1011/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2234: // extension has an index
(page or in-page), set it to -1.
On 2011/08/25 18:50:49, Evan Stade wrote:
> On 2011/08/25 17:59:35, Matt Perry wrote:
> > out of curiosity, why do we care whether a non-app has a page or in-page
> index?
> 
> because we need to be able to count the number of apps on a given page. We
could
> leave out the extensions when counting apps per page, but AFAICT
ExtensionPrefs
> doesn't have a way of knowing whether a given extension is an app. If there's
> something I'm missing I'll happily change this.

I see. Makes sense. Thanks for the explanation.