New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 175377 link

Starred by 4 users

Issue metadata

Status: Verified
Owner:
Email to this user bounced
Closed: Feb 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

In NTP, Only 17 Apps (Should be 18) are added in first Apps Tab.

Reported by smokana@chromium.org, Feb 11 2013

Issue description

Version: 26.0.1408.0 
OS: MAC

What steps will reproduce the problem?
1. Do a Fresh Install of Chrome or Create new Profile
2. Add Apps from Chrome Web store.
3. First Apps Page should have 18 Apps, but 18th App is moved to 2nd Page.

Page should contain 18 Apps. 

This is a regression issue.


 
Labels: -Type-Bug Type-Regression ReleaseBlock-Beta
This is a regression issue and below is the Bisect Info:

http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&range=180567%3A180602
Screenshot attached for Additional Info
Screen Shot 2013-02-11 at 2.57.11 PM.png
418 KB View Download

Comment 3 by dbeam@chromium.org, Feb 11 2013

Cc: tapted@chromium.org
probably tapted@ http://crrev.com/180596

Comment 4 by rsesek@chromium.org, Feb 11 2013

Cc: -tapted@chromium.org
Labels: -Pri-2 Pri-1
Owner: tapted@chromium.org
Status: Assigned

Comment 5 by tapted@chromium.org, Feb 12 2013

Cc: benwells@chromium.org
Nice work finding such a subtle bug! I think I've found the guts of the problem. Bit of context: when installed, app icons get both a 'launch_ordinal' and a 'page_ordinal', determined by extension_sorting.cc. page_ordinal determines which page an extension pops up on.

Now, both the CrOS-style app_list and the new tab page use extension_sorting.cc, but it has a hardcoded "natural" apps per page of 18 used for determining initial page ordinals in ExtensionSorting::GetNaturalAppPageOrdinal() [1]. (note: NTP can also have more than 18 on a page). Now the app_list has 16 apps per page, not 18(+), so it mostly ignores 'page_ordinals' (the one exception [2] might be a mistake). However, it still creates page_ordinals, and it can take up spots that the NTP would like to use.

Some ways to fix this might be:
1. alter the check at [1] to exclude extensions in that page which shouldn't take up a page_ordinal
2. skip adding app-list-only extensions to the page_ordinals map (and ensure the rest of extension_sorting.cc can cope with that!)
3. create a dummy/sentinel page ordinal for app_list-only extensions, and put all app-list-only extensions there
4. give app_list its own extension_sorting.h

I think "1." makes the most sense, since the "natural" apps per page is a kind of soft limit anyway. "2." and "3." are a bit hacky. "4." means a user loses their sort order if using the app_list.

There's also the first-run only aspect of this bug.. This is a bit more complex. It looks like when it's _not_ the first-run, sort ordinals are initialized in ExtensionPrefs::InitPrefStore [3]. However, the list of extensions passed in is incomplete -- e.g. it is missing WebStore, which gets a special case in extension_sorting.cc, but it is also missing other component extensions, which should* be part of the initialization. This might come up again with  Issue 171770 , but it doesn't currently seem to cause problems, since the only component, non-webstore extension doesn't need a page ordinal anyway.


[1] https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/extensions/extension_sorting.cc&q=knaturalapppagesize&sq=package:chrome&l=367

[2] https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/ui/app_list/extension_app_item.cc&q=PageOrdinal&sq=package:chrome&l=188&type=cs

[3] https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/extensions/extension_prefs.cc&q=extension_sorting_-%3EInitialize&l=2030
Labels: -ReleaseBlock-Beta
though its a regression, its a blocker. Please request for merge when we have a fix. thanks!
Cc: tapted@chromium.org
Owner: koz@chromium.org
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 27 2013

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=184833

------------------------------------------------------------------------
r184833 | koz@chromium.org | 2013-02-27T02:13:25.380360Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_sorting.h?r1=184833&r2=184832&pathrev=184833
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_service.cc?r1=184833&r2=184832&pathrev=184833
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_sorting.cc?r1=184833&r2=184832&pathrev=184833

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.

Review URL: https://chromiumcodereview.appspot.com/12331003
------------------------------------------------------------------------

Comment 9 by koz@chromium.org, Feb 27 2013

Labels: Merge-Requested

Comment 10 by dharani@google.com, Feb 27 2013

smokana: could you please verify it in 27.0.1424.0 build? thanks!
Labels: TE-Verified-27.0.1424.0
Works as expected . Verified using - 27.0.1424.0 (Official Build 184875) 
Labels: -Merge-Requested Merge-Approved
Project Member

Comment 13 by bugdroid1@chromium.org, Feb 28 2013

Labels: -Merge-Approved merge-merged-1410
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=185173

------------------------------------------------------------------------
r185173 | koz@chromium.org | 2013-02-28T07:06:55.553524Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/chrome/browser/extensions/extension_sorting.cc?r1=185173&r2=185172&pathrev=185173
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/chrome/browser/extensions/extension_sorting.h?r1=185173&r2=185172&pathrev=185173
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/chrome/browser/extensions/extension_service.cc?r1=185173&r2=185172&pathrev=185173

Merge 184833
> 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.
> 
> Review URL: https://chromiumcodereview.appspot.com/12331003

TBR=koz@chromium.org
Review URL: https://codereview.chromium.org/12390007
------------------------------------------------------------------------

Comment 14 by koz@chromium.org, Feb 28 2013

Status: Fixed
Labels: TE-Verified-26.0.1410.21
Status: Verified
this issue is fixed and verified on:-

26.0.1410.21 (Official Build 185427) beta
27.0.1425.2 (Official Build 185250) dev
Project Member

Comment 16 by bugdroid1@chromium.org, Mar 9 2013

Labels: -Type-Regression -Area-UI -Mstone-26 -Feature-NewTabPage Cr-UI-Browser-NewTabPage Type-Bug-Regression Cr-UI M-26

Sign in to add a comment