Do not assign launch ordinals to ephemeral apps

Do not assign launch ordinals to ephemeral apps

Ephemeral apps are not shown in the app launcher or NTP, but they are
assigned ordinals, which results in unfilled NTP pages. This patch
does not give ephemeral apps launch ordinals until they are promoted
to fully installed apps.

AppSorting::MarkExtensionAsHidden() was refactored to
AppSorting::SetExtensionVisible() to allow the visibility of the
extension in the NTP to change.

BUG=394192
TEST=browser_tests

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

[tmdiep, 2014-07-16 04:44:57]

I noticed that my NTP had unfilled pages. It turned out they were occupied by
invisible ephemeral apps. So it might be best not to give them ordinals until
they are installed. PTAL, thanks.

[tapted, 2014-07-16 05:03:38]

[drive-by]

we had to do something like this for the settings app -- see
https://chromiumcodereview.appspot.com/12331003

https://codereview.chromium.org/397903002/diff/1/chrome/browser/extensions/ch...
File chrome/browser/extensions/chrome_app_sorting.cc (right):

https://codereview.chromium.org/397903002/diff/1/chrome/browser/extensions/ch...
chrome/browser/extensions/chrome_app_sorting.cc:235: return;
It might not be safe to return early from here

"things" (e.g. chrome/browser/ui/app_list/extension_app_item.cc) can currently
assume an ordinal is set after calling this, which guarantees that the ordinal
can be fetched and compared with another ordinal without hitting any of the
CHECK(..)s in ordinal.h

There might be some more context about this failures in http://crbug.com/167752 
(or searching for closed bugs that mention 'EnsureValidOrdinals')

[tmdiep, 2014-07-16 05:25:52]

On 2014/07/16 05:03:38, tapted wrote:
> [drive-by]
> 
> we had to do something like this for the settings app -- see
> https://chromiumcodereview.appspot.com/12331003
> 
>
https://codereview.chromium.org/397903002/diff/1/chrome/browser/extensions/ch...
> File chrome/browser/extensions/chrome_app_sorting.cc (right):
> 
>
https://codereview.chromium.org/397903002/diff/1/chrome/browser/extensions/ch...
> chrome/browser/extensions/chrome_app_sorting.cc:235: return;
> It might not be safe to return early from here
> 
> "things" (e.g. chrome/browser/ui/app_list/extension_app_item.cc) can currently
> assume an ordinal is set after calling this, which guarantees that the ordinal
> can be fetched and compared with another ordinal without hitting any of the
> CHECK(..)s in ordinal.h
> 
> There might be some more context about this failures in
http://crbug.com/167752 
> (or searching for closed bugs that mention 'EnsureValidOrdinals')

Ah, thanks Trent! Explains a lot about the app sorting code.

I'll change the implementation of this since a function named
EnsureValidOrdinals() should always do as advertised.

[tmdiep, 2014-07-16 08:11:50]

I changed the implementation of this - please see patch set 2.

I still think it's a good idea not give ephemeral apps ordinals because the code
in ChromeAppSorting that resolves collisions will kick in unnecessarily when the
ordinals are synced.

EnsureValidOrdinals will always assign ordinals in case callers rely on this. We
don't expect extension_app_item to be created for an ephemeral app.

tapted, benwells: PTAL, thanks.

[tapted, 2014-07-17 04:29:46]

https://codereview.chromium.org/397903002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/397903002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1439:
!extension_prefs_->IsEphemeralApp(extension->id()));
are ephemeral apps never displayed in the new tab page? (i.e. should
extension->ShouldDisplayInNewTabPage() return false for ephemeral apps?). 

Although.. the same might also be true for
Extension::ShouldDisplayInAppLauncher, and if they're both true they disappear
from chrome;//extensions as well unless you update
Extension::ShouldDisplayInExtensionSettings() to return true for ephemeral apps
early on.

But I think Extension::ShouldDisplayInXXX() should be consistent with what's
done for ephemeral apps.

https://codereview.chromium.org/397903002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1855: if (!is_from_sync) {
After tracing down ordinal CHECK()s in the past I'm not 100% sure an ordinal is
guaranteed to come in from sync. Although the odds might be increasingly
unlikely and obscure - e.g. ~2years ago before there was "launch ordinal" there
was a different kind of ordinal. ntp/app_launcher_handler.cc has code to upgrade
them. But if someone ran Chrome 2 years ago with sync enabled does a new
install. Runs an ephemeral app, then signs in and gets their old sync data for
the same app, it might have the wrong kind of ordinal.

But I think the odds of this are too remote to bother with adding logic for :/.
You could perhaps do

if (is_from_sync) {
  bool has_ordinal = extension_prefs_->app_sorting()
      ->GetAppLaunchOrdinal(extension->id()).IsValid();
  CHECK(has_ordinal);
} else {
 ...
}

but I don't think it's necessary unless there's other more likely scenarios you
can think of.

https://codereview.chromium.org/397903002/diff/40001/extensions/browser/app_s...
File extensions/browser/app_sorting.h (right):

https://codereview.chromium.org/397903002/diff/40001/extensions/browser/app_s...
extensions/browser/app_sorting.h:112: // Sets whether the extension should be
visible in the new tab page.
Perhaps, 

// Hides an extension from the new tab page, or makes a previously hidden
// extension visible.

[tmdiep, 2014-07-17 05:48:57]

https://codereview.chromium.org/397903002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/397903002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1439:
!extension_prefs_->IsEphemeralApp(extension->id()));
On 2014/07/17 04:29:46, tapted wrote:
> are ephemeral apps never displayed in the new tab page? (i.e. should
> extension->ShouldDisplayInNewTabPage() return false for ephemeral apps?). 
> 
> Although.. the same might also be true for
> Extension::ShouldDisplayInAppLauncher, and if they're both true they disappear
> from chrome;//extensions as well unless you update
> Extension::ShouldDisplayInExtensionSettings() to return true for ephemeral
apps
> early on.
> 
> But I think Extension::ShouldDisplayInXXX() should be consistent with what's
> done for ephemeral apps.

I wish it could be just fully encapsulated in the Extension object. The problem
is that IsEphemeral is a runtime flag can change even while the app is running
and the Extension object can't access prefs.

https://codereview.chromium.org/397903002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1855: if (!is_from_sync) {
On 2014/07/17 04:29:46, tapted wrote:
> After tracing down ordinal CHECK()s in the past I'm not 100% sure an ordinal
is
> guaranteed to come in from sync. Although the odds might be increasingly
> unlikely and obscure - e.g. ~2years ago before there was "launch ordinal"
there
> was a different kind of ordinal. ntp/app_launcher_handler.cc has code to
upgrade
> them. But if someone ran Chrome 2 years ago with sync enabled does a new
> install. Runs an ephemeral app, then signs in and gets their old sync data for
> the same app, it might have the wrong kind of ordinal.
> 
> But I think the odds of this are too remote to bother with adding logic for
:/.
> You could perhaps do
> 
> if (is_from_sync) {
>   bool has_ordinal = extension_prefs_->app_sorting()
>       ->GetAppLaunchOrdinal(extension->id()).IsValid();
>   CHECK(has_ordinal);
> } else {
>  ...
> }
> 
> but I don't think it's necessary unless there's other more likely scenarios
you
> can think of.

I've refactored to always call EnsureValidOrdinals, as there is no harm in doing
so, but maybe more harm in not doing so.

It relies on sync having updated the launch ordinals in the extension prefs
before installing the extension. Can we rely on this order of events? Probably,
otherwise if the install came first, the call to EnsureValidOrdinals in
ExtensionService::AddExtension would clobber the ordinals.

https://codereview.chromium.org/397903002/diff/40001/extensions/browser/app_s...
File extensions/browser/app_sorting.h (right):

https://codereview.chromium.org/397903002/diff/40001/extensions/browser/app_s...
extensions/browser/app_sorting.h:112: // Sets whether the extension should be
visible in the new tab page.
On 2014/07/17 04:29:46, tapted wrote:
> Perhaps, 
> 
> // Hides an extension from the new tab page, or makes a previously hidden
> // extension visible.

Done.

[tapted, 2014-07-17 05:59:53]

lgtm

https://codereview.chromium.org/397903002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/397903002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1439:
!extension_prefs_->IsEphemeralApp(extension->id()));
On 2014/07/17 05:48:57, tmdiep wrote:
> On 2014/07/17 04:29:46, tapted wrote:
> > are ephemeral apps never displayed in the new tab page? (i.e. should
> > extension->ShouldDisplayInNewTabPage() return false for ephemeral apps?). 
> > 
> > Although.. the same might also be true for
> > Extension::ShouldDisplayInAppLauncher, and if they're both true they
disappear
> > from chrome;//extensions as well unless you update
> > Extension::ShouldDisplayInExtensionSettings() to return true for ephemeral
> apps
> > early on.
> > 
> > But I think Extension::ShouldDisplayInXXX() should be consistent with what's
> > done for ephemeral apps.
> 
> I wish it could be just fully encapsulated in the Extension object. The
problem
> is that IsEphemeral is a runtime flag can change even while the app is running
> and the Extension object can't access prefs.

Ah - drat.

[benwells, 2014-07-18 05:01:06]

On 2014/07/17 05:59:53, tapted wrote:
> lgtm
> 
>
https://codereview.chromium.org/397903002/diff/40001/chrome/browser/extension...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
https://codereview.chromium.org/397903002/diff/40001/chrome/browser/extension...
> chrome/browser/extensions/extension_service.cc:1439:
> !extension_prefs_->IsEphemeralApp(extension->id()));
> On 2014/07/17 05:48:57, tmdiep wrote:
> > On 2014/07/17 04:29:46, tapted wrote:
> > > are ephemeral apps never displayed in the new tab page? (i.e. should
> > > extension->ShouldDisplayInNewTabPage() return false for ephemeral apps?). 
> > > 
> > > Although.. the same might also be true for
> > > Extension::ShouldDisplayInAppLauncher, and if they're both true they
> disappear
> > > from chrome;//extensions as well unless you update
> > > Extension::ShouldDisplayInExtensionSettings() to return true for ephemeral
> > apps
> > > early on.
> > > 
> > > But I think Extension::ShouldDisplayInXXX() should be consistent with
what's
> > > done for ephemeral apps.
> > 
> > I wish it could be just fully encapsulated in the Extension object. The
> problem
> > is that IsEphemeral is a runtime flag can change even while the app is
running
> > and the Extension object can't access prefs.
> 
> Ah - drat.

stamp lgtm

[tmdiep, 2014-07-21 22:02:54]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-07-21 22:03:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/397903002/140001

[commit-bot: I haz the power, 2014-07-21 23:19:40]

Change committed as 284536