Make the Chrome Web Store Icon Syncable

Make the Chrome Web Store Icon Syncable

Mark the CWS as a syncable app so that it page and app launch ordinals are synced. It also ensure that the CWS has valid ordinals as soon as it it added to the client, instead of waiting until the NTP is shown.

BUG=112290
TEST=Users should be able to move the Chrome Web store icon and have its positions update to other signed in locations.


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

[csharp, 2012-02-06 22:23:56]

This is the same change as before with a sync integration test (just for the
CWS) and a fix to prevent this from breaking the sync integration tests.

Basically the tests broke because they were trying to compare the CWS ordinals
before the ordinals got set. The CWS ordinals aren't setup because the elements
created in ComponentLoader don't go to ExtensionService::OnExtensionInstalled,
which is where most of the extensions have their ordinals initialized. The CWS
instead was having its ordinal initialized AppLauncherHandler::CreateAppInfo, as
it was getting ready to be sent to the NTP, which the sync test profiles don't
do because they don't need to show the NTP.

The fix I've added for this is to just ensure in ExtensionService::AddExtension
the CWS gets valid ordinals if it doesn't have any. I dug around in the code and
this seemed like the only good place to be setting this value.

[Finnur, 2012-02-07 10:12:53]

Extension changes LGTM, I'll leave it to akalin to judge the sync part.

https://chromiumcodereview.appspot.com/9340007/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/9340007/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:2074:
extension_sorting->CreateFirstAppLaunchOrdinal(page_ordinal);
nit: indentation.

https://chromiumcodereview.appspot.com/9340007/diff/1/chrome/common/extension...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/9340007/diff/1/chrome/common/extension...
chrome/common/extensions/extension.cc:2977: // something more systematically
should be done.
s/systematically/systematic.

[csharp, 2012-02-07 15:49:39]

https://chromiumcodereview.appspot.com/9340007/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/9340007/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:2074:
extension_sorting->CreateFirstAppLaunchOrdinal(page_ordinal);
On 2012/02/07 10:12:53, Finnur wrote:
> nit: indentation.

Done.

https://chromiumcodereview.appspot.com/9340007/diff/1/chrome/common/extension...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/9340007/diff/1/chrome/common/extension...
chrome/common/extensions/extension.cc:2977: // something more systematically
should be done.
On 2012/02/07 10:12:53, Finnur wrote:
> s/systematically/systematic.

Done.

[akalin, 2012-02-07 22:49:09]

On 2012/02/07 15:49:39, csharp wrote:
>
https://chromiumcodereview.appspot.com/9340007/diff/1/chrome/browser/extensio...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9340007/diff/1/chrome/browser/extensio...
> chrome/browser/extensions/extension_service.cc:2074:
> extension_sorting->CreateFirstAppLaunchOrdinal(page_ordinal);
> On 2012/02/07 10:12:53, Finnur wrote:
> > nit: indentation.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/9340007/diff/1/chrome/common/extension...
> File chrome/common/extensions/extension.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9340007/diff/1/chrome/common/extension...
> chrome/common/extensions/extension.cc:2977: // something more systematically
> should be done.
> On 2012/02/07 10:12:53, Finnur wrote:
> > s/systematically/systematic.
> 
> Done.

You should also run the sync integration tests, which were the ones that failed
last time.  I'm surprised it doesn't do so automatically, since it should if you
modify anything under sync.  Are you manually picking the try bots?

[akalin, 2012-02-07 22:54:13]

LGTM after clean trybot runs (don't use the CQ until you get those)

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

http://codereview.chromium.org/9340007/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2060: if (extension->id() ==
extension_misc::kWebStoreAppId) {
Why does this only apply to this particular app?  Why doesn't it apply to apps
in general?  Is it because CWS is the only app that can be installed before the
NTP shows up?  This should probably be explained in a comment.

[Aaron Boodman, 2012-02-07 23:01:18]

I'm not crazy about the special cases in core extension classes. Can you hold
off on this until the end of the day so I can look closer?

[Aaron Boodman, 2012-02-08 00:37:53]

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

http://codereview.chromium.org/9340007/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1576: // This shouldn't be called
for component extensions other than the
Sorry, why does this happen - sync?

http://codereview.chromium.org/9340007/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2060: if (extension->id() ==
extension_misc::kWebStoreAppId) {
On 2012/02/07 22:54:13, akalin wrote:
> Why does this only apply to this particular app?  Why doesn't it apply to apps
> in general?  Is it because CWS is the only app that can be installed before
the
> NTP shows up?  This should probably be explained in a comment.

Can we just initialize all ordinals if they haven't already been done in
::AddExtension()?

Probably a better filter of what kinds of things to do this work for would be
extension->IsApp(). It doesn't really matter what the install location is. If
it's an app, it's going to show up in the launcher, and therefore should have an
ordinal (right?).

http://codereview.chromium.org/9340007/diff/5001/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9340007/diff/5001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2978: if (id() ==
extension_misc::kWebStoreAppId)
Perhaps a better test would be:

return extension->location() == Extension::INTERNAL || extension->IsApp() ?

Do all loaded apps show up in launcher, or only ones that are location() ==
INTERNAL, or what?

[csharp, 2012-02-08 15:46:16]

I'll run this change though the try bots again to make sure it doesn't break
anything and I hope to submit it tomorrow morning. Let me know if there are any
issues with that.

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

http://codereview.chromium.org/9340007/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1576: // This shouldn't be called
for component extensions other than the
On 2012/02/08 00:37:53, Aaron Boodman wrote:
> Sorry, why does this happen - sync?

Yup.

http://codereview.chromium.org/9340007/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2060: if (extension->id() ==
extension_misc::kWebStoreAppId) {
On 2012/02/08 00:37:53, Aaron Boodman wrote:
> On 2012/02/07 22:54:13, akalin wrote:
> > Why does this only apply to this particular app?  Why doesn't it apply to
apps
> > in general?  Is it because CWS is the only app that can be installed before
> the
> > NTP shows up?  This should probably be explained in a comment.
> 
> Can we just initialize all ordinals if they haven't already been done in
> ::AddExtension()?
> 
> Probably a better filter of what kinds of things to do this work for would be
> extension->IsApp(). It doesn't really matter what the install location is. If
> it's an app, it's going to show up in the launcher, and therefore should have
an
> ordinal (right?).

Good points. I'll initialize all uninitialized ordinals for apps here.

http://codereview.chromium.org/9340007/diff/5001/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9340007/diff/5001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2978: if (id() ==
extension_misc::kWebStoreAppId)
On 2012/02/08 00:37:53, Aaron Boodman wrote:
> Perhaps a better test would be:
> 
> return extension->location() == Extension::INTERNAL || extension->IsApp() ?
> 
> Do all loaded apps show up in launcher, or only ones that are location() ==
> INTERNAL, or what?

I'm fairly certain that all loaded apps will appear on the NTP, so I've modify
the check to not return SYNC_TYPE_NONE for app (since we will want to sync the
ordinals). 

akalin, does this seems reasonable to you?

[csharp, 2012-02-08 16:43:33]

http://codereview.chromium.org/9340007/diff/5001/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9340007/diff/5001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2978: if (id() ==
extension_misc::kWebStoreAppId)
On 2012/02/08 15:46:16, csharp wrote:
> On 2012/02/08 00:37:53, Aaron Boodman wrote:
> > Perhaps a better test would be:
> > 
> > return extension->location() == Extension::INTERNAL || extension->IsApp() ?
> > 
> > Do all loaded apps show up in launcher, or only ones that are location() ==
> > INTERNAL, or what?
> 
> I'm fairly certain that all loaded apps will appear on the NTP, so I've modify
> the check to not return SYNC_TYPE_NONE for app (since we will want to sync the
> ordinals). 
> 
> akalin, does this seems reasonable to you?

Ok, so not all loaded apps appear in the app launch (cloud print is an app and
doesn't appear). This change cause the cloud print app to be synced, which I
don't think it should be. I've reverted this back to just treating the webstore
different here, because otherwise I think I'll end up with a  lot of other
places where the cloud print app needs to be treated differently.

[Aaron Boodman, 2012-02-08 18:23:28]

How about this:

bool Extension::ShouldDisplayInLauncher() {
  return location_ == INTERNAL || <is webstore>;
}

bool Extension::IsSyncable() {
  return location_ == INTERNAL || ShouldDisplayInLauncher();
}

... and make sure to call ShouldDisplayInLauncher in the launcher code.

http://codereview.chromium.org/9340007/diff/10003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/9340007/diff/10003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1578: DCHECK_EQ(extension_id,
std::string(extension_misc::kWebStoreAppId));
Can't this just be:

DCHECK(extension->IsSyncable())

That's more self-documenting anyway.

http://codereview.chromium.org/9340007/diff/10003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:2061: if (extension->is_app()) {
But you just said this isn't true. Will it be bad to give cloud print an
ordinal?

[akalin, 2012-02-08 18:27:42]

http://codereview.chromium.org/9340007/diff/10003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/9340007/diff/10003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:2067: page_ordinal =
extension->id() == extension_misc::kWebStoreAppId ?
add a comment explaining the special cases for the web store icon

http://codereview.chromium.org/9340007/diff/10003/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_apps_sync_test.cc (right):

http://codereview.chromium.org/9340007/diff/10003/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_apps_sync_test.cc:379:
ASSERT_TRUE(AllProfilesHaveSameAppsAsVerifier());
it looks like the integration tests crash if there's a problem with the
ordinals. can you change it so that the test merely fails?  That may require
adding a method to make sure AppState is valid (i.e., contains only valid string
ordinals) and calling those before calling Equals().

[akalin, 2012-02-08 18:29:02]

On 2012/02/08 15:46:16, csharp wrote:
> I'll run this change though the try bots again to make sure it doesn't break
> anything and I hope to submit it tomorrow morning. Let me know if there are
any
> issues with that.

It sounds like you need to wait for a LGTM from aa@.

[csharp, 2012-02-08 20:02:26]

estade, could you look at the webui changes? Thanks

http://codereview.chromium.org/9340007/diff/10003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/9340007/diff/10003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1578: DCHECK_EQ(extension_id,
std::string(extension_misc::kWebStoreAppId));
On 2012/02/08 18:23:28, Aaron Boodman wrote:
> Can't this just be:
> 
> DCHECK(extension->IsSyncable())
> 
> That's more self-documenting anyway. 

Done.

http://codereview.chromium.org/9340007/diff/10003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:2061: if (extension->is_app()) {
On 2012/02/08 18:23:28, Aaron Boodman wrote:
> But you just said this isn't true. Will it be bad to give cloud print an
> ordinal?

Changing to only do if the extension Should display in the launcher.

http://codereview.chromium.org/9340007/diff/10003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:2067: page_ordinal =
extension->id() == extension_misc::kWebStoreAppId ?
On 2012/02/08 18:27:43, akalin wrote:
> add a comment explaining the special cases for the web store icon

Done.

http://codereview.chromium.org/9340007/diff/10003/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_apps_sync_test.cc (right):

http://codereview.chromium.org/9340007/diff/10003/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_apps_sync_test.cc:379:
ASSERT_TRUE(AllProfilesHaveSameAppsAsVerifier());
On 2012/02/08 18:27:43, akalin wrote:
> it looks like the integration tests crash if there's a problem with the
> ordinals. can you change it so that the test merely fails?  That may require
> adding a method to make sure AppState is valid (i.e., contains only valid
string
> ordinals) and calling those before calling Equals().

Done.

[Aaron Boodman, 2012-02-09 00:05:46]

http://codereview.chromium.org/9340007/diff/11004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/9340007/diff/11004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:2060: if
(extension->ShouldDisplayInLauncher()) {
Last comment: Is it possible to pull all this code out into ExtensionSorting?
We're trying to keep the amount of glue in ExtensionService down.

[Evan Stade, 2012-02-09 02:23:39]

webui lgtm

[csharp, 2012-02-09 15:45:45]

http://codereview.chromium.org/9340007/diff/11004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/9340007/diff/11004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:2060: if
(extension->ShouldDisplayInLauncher()) {
On 2012/02/09 00:05:46, Aaron Boodman wrote:
> Last comment: Is it possible to pull all this code out into ExtensionSorting?
> We're trying to keep the amount of glue in ExtensionService down.

Done.

[csharp, 2012-02-09 17:02:28]

sky, can you look at app_list_model_builder.cc? Thanks

[sky, 2012-02-09 19:06:36]

LGTM

[Aaron Boodman, 2012-02-09 19:17:37]

lgtm

http://codereview.chromium.org/9340007/diff/18006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/9340007/diff/18006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:2060: if
(extension->ShouldDisplayInLauncher()) {
Nit: Braces are not required for one-line ifs. In the extension system we tend
to omit them in those cases.

[csharp, 2012-02-09 19:53:16]

http://codereview.chromium.org/9340007/diff/18006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/9340007/diff/18006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:2060: if
(extension->ShouldDisplayInLauncher()) {
On 2012/02/09 19:17:37, Aaron Boodman wrote:
> Nit: Braces are not required for one-line ifs. In the extension system we tend
> to omit them in those cases.

Done.

[commit-bot: I haz the power, 2012-02-09 19:53:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/9340007/19014

[akalin, 2012-02-09 20:40:47]

LGTM

[commit-bot: I haz the power, 2012-02-09 21:51:53]

Change committed as 121310