Apps that don't appear on the NTP shouldn't contribute to NTP page overflow.

Apps that don't appear on the NTP shouldn't contribute to NTP page overflow.


BUG=175377
TEST=Start chrome with a fresh profile (eg: --user-data-dir=new-dir), then keep installing apps from the Chrome Web Store until you have 18. All 18 apps should appear on the same (first) page.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184833

[koz (OOO until 15th September), 2013-02-26 00:28:36]

So here's a hacky way to fix this bug, but one with a minimal footprint for easy
merging.

Perhaps this isn't even too far from the right fix here and we just need to
change the interface so that all call sites pass Extension* so we can do the
MarkAsHidden() ourselves.

[tapted, 2013-02-26 03:38:49]

On 2013/02/26 00:28:36, koz wrote:
> So here's a hacky way to fix this bug, but one with a minimal footprint for
easy
> merging.

Nice - Doesn't even feel that hacky imo :)

> Perhaps this isn't even too far from the right fix here and we just need to
> change the interface so that all call sites pass Extension* so we can do the
> MarkAsHidden() ourselves.

There's an awkward point in initialization where you only have access to the
ExtensionPrefs, since the Extension* hasn't been created yet.
$profile/Preferences does include an entry, e.g.:
$ grep -A2 'Chrome.*app' ~/Library/Application\
Support/Chromium/Default/Preferences 
               "description": "Chrome as an app",
               "display_in_launcher": true,
               "display_in_new_tab_page": false,

But I think this CL even gives a nicer way: It would allow a follow-up call to
bulk-hide things once the extensions are all loaded, rather than trying to parse
extra prefs to keep the info in the one data structure at initialization time.

https://codereview.chromium.org/12331003/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_sorting.cc (right):

https://codereview.chromium.org/12331003/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_sorting.cc:623: if
(hidden_extensions_.count(id) == 0) {
nit: no braces on single-line if (seems to prevail in this file).

Also, `id` isn't really needed as a local arg, but there may be merit in making
it clear it's a string by having it.

https://codereview.chromium.org/12331003/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_sorting.h (right):

https://codereview.chromium.org/12331003/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_sorting.h:106: void
MarkExtensionAsHidden(const std::string& extension_id);
I think the member names need to mention NTP, or they read wrong. E.g.
MarkExtensionAsNTPHidden  here.

Elsewhere:
CountNonHiddenItems -> CountNonNTPHiddenItems
hidden_extensions -> ntp_hidden_extensions_;

WDYT?

https://codereview.chromium.org/12331003/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_sorting.h:186: // Returns the number of non
hidden items in |m|.
ubernit: "non" -> "non-"

[koz (OOO until 15th September), 2013-02-26 03:58:37]

Yeah, true. Perhaps hacky is a bit strong a word here. It's good to have an
extra data structure to hold this bit because it lets us take care of this
concern much more orthogonally than we might otherwise.

https://codereview.chromium.org/12331003/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_sorting.cc (right):

https://codereview.chromium.org/12331003/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_sorting.cc:623: if
(hidden_extensions_.count(id) == 0) {
On 2013/02/26 03:38:49, tapted wrote:
> nit: no braces on single-line if (seems to prevail in this file).
> 
> Also, `id` isn't really needed as a local arg, but there may be merit in
making
> it clear it's a string by having it.

Yeah, I find it helps when dealing with iterators to have named intermediary
variables.
Removed the curlies.

https://codereview.chromium.org/12331003/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_sorting.h (right):

https://codereview.chromium.org/12331003/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_sorting.h:106: void
MarkExtensionAsHidden(const std::string& extension_id);
On 2013/02/26 03:38:49, tapted wrote:
> I think the member names need to mention NTP, or they read wrong. E.g.
> MarkExtensionAsNTPHidden  here.
> 
> Elsewhere:
> CountNonHiddenItems -> CountNonNTPHiddenItems
> hidden_extensions -> ntp_hidden_extensions_;
> 
> WDYT?

Yep, good point. Done.

https://codereview.chromium.org/12331003/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_sorting.h:186: // Returns the number of non
hidden items in |m|.
On 2013/02/26 03:38:49, tapted wrote:
> ubernit: "non" -> "non-"

hahaha ubernit. I love it! Fixed :-)

[tapted, 2013-02-26 04:29:15]

CL description probably needs a TEST= line. Sorry should have noticed before...
E.g. With a fresh profile, add 18 apps. 18th should not appear on second page.

Otherwise, lgtm!

[koz (OOO until 15th September), 2013-02-26 04:35:59]

Done, ta!

csharp, does this LGTY?

[csharp, 2013-02-26 14:19:25]

lgtm

[commit-bot: I haz the power, 2013-02-26 23:42:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/12331003/8001

[commit-bot: I haz the power, 2013-02-27 02:13:29]

Change committed as 184833