Replace OnceOffCreateShortcuts with UpdateShortcutsForAllAppsIfNeeded.

Replace OnceOffCreateShortcuts with UpdateShortcutsForAllAppsIfNeeded.

The once-off creation logic is no longer needed as app
shortcuts have been enabled by default for a while.

This replaces kAppShortcutsHaveBeenCreated with
kAppShortcutsVersion and allows us to trigger a rebuild
of all app shortcuts. This is only implemented for Mac.

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

[jackhou1, 2014-05-06 08:15:46]

What do you guys think of this approach? Is is sufficient for your needs on
Win/Linux?

[Matt Giuca, 2014-05-07 05:21:51]

Yeah looks like a good approach. See a few comments.

But I didn't do a very thorough review. Let me know when you're ready for a full
review.

https://codereview.chromium.org/263403002/diff/1/chrome/browser/apps/shortcut...
File chrome/browser/apps/shortcut_manager.cc (right):

https://codereview.chromium.org/263403002/diff/1/chrome/browser/apps/shortcut...
chrome/browser/apps/shortcut_manager.cc:35: const int
kCurrentAppShortcutsVersion = 1;
This needs a comment. Please say something about exactly what conditions this
should be bumped for (e.g., whenever a new version of Chrome would change the
way app shortcuts are written to disk, bump this number to force them all to be
recreated).

Also say that 0 means that no app shortcuts have ever been created.

https://codereview.chromium.org/263403002/diff/1/chrome/browser/apps/shortcut...
chrome/browser/apps/shortcut_manager.cc:157: if (last_version ==
current_version)
Should probably be (last_version >= current_version), so you don't rewrite
shortcuts that are somehow from the future.

Also, since you're using a special value 0 for --disable-app-shims, your logic
will write a shortcut if there is an existing shortcut and --disable-app-shims,
which you don't want.

[calamity, 2014-05-07 06:00:18]

https://codereview.chromium.org/263403002/diff/1/chrome/browser/apps/shortcut...
File chrome/browser/apps/shortcut_manager.cc (right):

https://codereview.chromium.org/263403002/diff/1/chrome/browser/apps/shortcut...
chrome/browser/apps/shortcut_manager.cc:160:
web_app::UpdateShortcutsForAllApps(profile_);
Maybe this should support a callback so that the pref is set after all IO
operations are done. Of course, that would require plumbing callback all the way
down. I wouldn't worry about it for this patch.

https://codereview.chromium.org/263403002/diff/1/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/263403002/diff/1/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:290:
DCHECK_CURRENTLY_ON(BrowserThread::FILE);
Why does this need to be on the FILE thread? Can you just roll all of this into
UpdateShortcutsForAllApps()? You already seem to have the Extension*s and the
Profile* that you need over there.

[jackhou1, 2014-05-22 05:26:46]

PTAL

The once-off code is gone. 

There's no default implementation for UpdateShortcutsForAllAppsForProfile. On
Mac, it just recreates all shortcuts, which is sufficient for now. I'll leave it
for you guys to implement for Win/Linux when you next need to push a shortcuts
change.

BTW, I'll land this after removing IsAppShimsEnabled:
https://codereview.chromium.org/298953002/

https://codereview.chromium.org/263403002/diff/1/chrome/browser/apps/shortcut...
File chrome/browser/apps/shortcut_manager.cc (right):

https://codereview.chromium.org/263403002/diff/1/chrome/browser/apps/shortcut...
chrome/browser/apps/shortcut_manager.cc:35: const int
kCurrentAppShortcutsVersion = 1;
On 2014/05/07 05:21:51, Matt Giuca wrote:
> This needs a comment. Please say something about exactly what conditions this
> should be bumped for (e.g., whenever a new version of Chrome would change the
> way app shortcuts are written to disk, bump this number to force them all to
be
> recreated).
> 
> Also say that 0 means that no app shortcuts have ever been created.

Done.

The 0 case should be gone soon. I'll put up a CL to remove --disable-app-shims.

https://codereview.chromium.org/263403002/diff/1/chrome/browser/apps/shortcut...
chrome/browser/apps/shortcut_manager.cc:157: if (last_version ==
current_version)
On 2014/05/07 05:21:51, Matt Giuca wrote:
> Should probably be (last_version >= current_version), so you don't rewrite
> shortcuts that are somehow from the future.
> 
> Also, since you're using a special value 0 for --disable-app-shims, your logic
> will write a shortcut if there is an existing shortcut and
--disable-app-shims,
> which you don't want.

Done.

https://codereview.chromium.org/263403002/diff/1/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/263403002/diff/1/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:290:
DCHECK_CURRENTLY_ON(BrowserThread::FILE);
On 2014/05/07 06:00:19, calamity wrote:
> Why does this need to be on the FILE thread? Can you just roll all of this
into
> UpdateShortcutsForAllApps()? You already seem to have the Extension*s and the
> Profile* that you need over there.

This is where we'll scan for shortcuts and decide what needs updating/deleting.

[Matt Giuca, 2014-05-23 07:01:57]

Handful of small nits. LGTM.

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/apps/shor...
File chrome/browser/apps/shortcut_manager.cc (right):

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/apps/shor...
chrome/browser/apps/shortcut_manager.cc:37: // shortcuts like command line flags
or associated icons, binaries, etc.
Nit: command-line

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/apps/shor...
File chrome/browser/apps/shortcut_manager.h (right):

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/apps/shor...
chrome/browser/apps/shortcut_manager.h:32: // Updates all shortcuts if if
kAppShortcutsVersion in prefs is less than
if if

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_impl.cc:347: }
Can you explain why this call to OnceOffCreateShortcuts
(UpdateShortcutsForAllAppsIfNeeded) was removed?

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app.cc:470: if (!(extension_system =
extensions::ExtensionSystem::Get(profile)) ||
Uh, != ?

[jackhou1, 2014-05-26 07:57:36]

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/apps/shor...
File chrome/browser/apps/shortcut_manager.cc (right):

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/apps/shor...
chrome/browser/apps/shortcut_manager.cc:37: // shortcuts like command line flags
or associated icons, binaries, etc.
On 2014/05/23 07:01:57, Matt Giuca wrote:
> Nit: command-line

Done.

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/apps/shor...
File chrome/browser/apps/shortcut_manager.h (right):

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/apps/shor...
chrome/browser/apps/shortcut_manager.h:32: // Updates all shortcuts if if
kAppShortcutsVersion in prefs is less than
On 2014/05/23 07:01:57, Matt Giuca wrote:
> if if

Done.

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_impl.cc:347: }
On 2014/05/23 07:01:57, Matt Giuca wrote:
> Can you explain why this call to OnceOffCreateShortcuts
> (UpdateShortcutsForAllAppsIfNeeded) was removed?

We don't need it any more since we've had app shortcuts enabled for ages. It was
previously here for people who enabled apps before we created shortcuts for
them. If you like, I can split these into two CLs, one removes the
OnceOffCreateShortcuts logic, another adds UpdateShortcutsForAllAppsIfNeeded.
The reason they're both in this CL is that they share a lot of code, so I
figured it's easier to move/rename than to delete then re-add.

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app.cc:470: if (!(extension_system =
extensions::ExtensionSystem::Get(profile)) ||
On 2014/05/23 07:01:57, Matt Giuca wrote:
> Uh, != ?

Get() and extension_service() both can return NULL. I think this is the correct
pattern for checking that they worked.

[jackhou1, 2014-05-26 07:58:49]

calamity, did you have any more comments?

[Matt Giuca, 2014-05-26 08:07:35]

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_impl.cc:347: }
Yeah would you be able to split it, if it's not too much effort?

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app.cc:470: if (!(extension_system =
extensions::ExtensionSystem::Get(profile)) ||
Ohh.. wow, I totally didn't see that it was = instead of ==! Oops.

I think this is confusing code. I don't see any code outside of third_party
using this:
Try this regex on codesite:
"!\([a-z_]\ \=\ "

[jackhou1, 2014-05-27 01:03:57]

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_impl.cc:347: }
On 2014/05/26 08:07:36, Matt Giuca wrote:
> Yeah would you be able to split it, if it's not too much effort?

As per discussion, keeping this CL has the benefit that it clearly shows the
overall change. In particular in shortcut_manager.cc, it's clear that
UpdateAllShortcutsIfNeeded replaces OnceOffCreateShortcuts, and int
kAppShortcutsVersion replaces bool kAppShortcutsHaveBeenCreated.

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/263403002/diff/40001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app.cc:470: if (!(extension_system =
extensions::ExtensionSystem::Get(profile)) ||
On 2014/05/26 08:07:36, Matt Giuca wrote:
> Ohh.. wow, I totally didn't see that it was = instead of ==! Oops.
> 
> I think this is confusing code. I don't see any code outside of third_party
> using this:
> Try this regex on codesite:
> "!\([a-z_]\ \=\ "

Ok, changed to something simpler.

[Matt Giuca, 2014-05-27 01:14:52]

SLGTM

[calamity, 2014-05-29 03:03:09]

https://codereview.chromium.org/263403002/diff/100001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/263403002/diff/100001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app.cc:232: app_id,
extensions::ExtensionRegistry::ENABLED);
Shouldn't we be excluding disabled/terminated extensions?

[jackhou1, 2014-05-29 04:23:46]

https://codereview.chromium.org/263403002/diff/100001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/263403002/diff/100001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app.cc:232: app_id,
extensions::ExtensionRegistry::ENABLED);
On 2014/05/29 03:03:10, calamity wrote:
> Shouldn't we be excluding disabled/terminated extensions?

I take it you mean including. UpdateShortcutsForAllApps filters all app_ids with
ShouldCreateShortcutFor, so this can just use ExtensionRegistry::EVERYTHING.

However, UpdateShortcutsForAllApps should actually be grabbing all extensions,
since we don't do anything with regard to shortcuts when extensions are
disabled/terminated/blacklisted or re-enabled.

[calamity, 2014-05-29 05:53:27]

lgtm

[jackhou1, 2014-05-29 05:54:10]

tapted, could you review:

chrome/browser/ui/app_list/app_list_service_impl.cc

also, for OWNERS:

chrome/browser/apps/shortcut_manager.h
chrome/browser/apps/shortcut_manager.cc

[tapted, 2014-05-29 08:56:40]

I thought it was really weird why ShortcutOperationCallback existed at all, and
this wasn't all just dumped into web_app_mac.mm. I think this is because you
anticipate this needing to be implemented on Linux and Windows as well?  (have
those implementations been sketched out? It might be better to make a
non-generic version now, and make it generic later, since requirements often
change).

https://codereview.chromium.org/263403002/diff/100001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/263403002/diff/100001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app.cc:463: BrowserThread::FILE,
I couldn't see why this needed to be on the file thread - comment?.

And is it worth posting a delayed task so that startup is not impacted after a
possible upgrade?

https://codereview.chromium.org/263403002/diff/120001/chrome/browser/apps/sho...
File chrome/browser/apps/shortcut_manager.cc (left):

https://codereview.chromium.org/263403002/diff/120001/chrome/browser/apps/sho...
chrome/browser/apps/shortcut_manager.cc:146: if
(prefs_->GetBoolean(prefs::kAppShortcutsHaveBeenCreated))
Is this pref still used elsewhere? (delete it?)

https://codereview.chromium.org/263403002/diff/120001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/263403002/diff/120001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_mac.mm:984: content::BrowserThread::UI,
nit: indenting is off here

[tapted, 2014-05-29 13:07:43]

https://codereview.chromium.org/263403002/diff/120001/chrome/browser/apps/sho...
File chrome/browser/apps/shortcut_manager.cc (left):

https://codereview.chromium.org/263403002/diff/120001/chrome/browser/apps/sho...
chrome/browser/apps/shortcut_manager.cc:146: if
(prefs_->GetBoolean(prefs::kAppShortcutsHaveBeenCreated))
On 2014/05/29 08:56:40, tapted wrote:
> Is this pref still used elsewhere? (delete it?)

oops - I was looking for red deletey text :o. It's all blue :p

[jackhou1, 2014-05-30 04:38:52]

> It might be better to make a
> non-generic version now, and make it generic later, since requirements often
> change).

Done.

https://codereview.chromium.org/263403002/diff/120001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/263403002/diff/120001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_mac.mm:984: content::BrowserThread::UI,
On 2014/05/29 08:56:40, tapted wrote:
> nit: indenting is off here

Done.

[tapted, 2014-05-30 06:33:27]

https://codereview.chromium.org/263403002/diff/150001/chrome/browser/apps/sho...
File chrome/browser/apps/shortcut_manager.cc (right):

https://codereview.chromium.org/263403002/diff/150001/chrome/browser/apps/sho...
chrome/browser/apps/shortcut_manager.cc:37: const int
kCurrentAppShortcutsVersion = 1;
Note we might want this to be per-platform. And the first time windows/linux
gets implemented we will of course need to increment it on those platforms to 2.
I guess we can worry about it later (#ifdef or extern const int or similar). Or
we could set it to 0 on win/linux initially.

https://codereview.chromium.org/263403002/diff/150001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/263403002/diff/150001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app.cc:430: app_ids.insert((*it)->id());
Why not just call

web_app::UpdateAllShortcuts(base::string16(), profile, extension);

here?

(I'm not sure if the explanation I came up with in my head is a good one, so I
want to hear yours :p)

[tapted, 2014-05-30 06:38:17]

On 2014/05/30 06:33:27, tapted wrote:
>
https://codereview.chromium.org/263403002/diff/150001/chrome/browser/apps/sho...
> File chrome/browser/apps/shortcut_manager.cc (right):
> 
>
https://codereview.chromium.org/263403002/diff/150001/chrome/browser/apps/sho...
> chrome/browser/apps/shortcut_manager.cc:37: const int
> kCurrentAppShortcutsVersion = 1;
> Note we might want this to be per-platform. And the first time windows/linux
> gets implemented we will of course need to increment it on those platforms to
2.
> I guess we can worry about it later (#ifdef or extern const int or similar).
Or
> we could set it to 0 on win/linux initially.
> 
>
https://codereview.chromium.org/263403002/diff/150001/chrome/browser/web_appl...
> File chrome/browser/web_applications/web_app.cc (right):
> 
>
https://codereview.chromium.org/263403002/diff/150001/chrome/browser/web_appl...
> chrome/browser/web_applications/web_app.cc:430: app_ids.insert((*it)->id());
> Why not just call
> 
> web_app::UpdateAllShortcuts(base::string16(), profile, extension);
> 
> here?
> 
> (I'm not sure if the explanation I came up with in my head is a good one, so I
> want to hear yours :p)

Oh I see calamity asked this too. Definitely add a comment. But also consider
whether it makes sense to try and anticipate what the needs are at this stage,
or if it's Ok just to call it directly now.

For example, for a successful scan on the file thread, you might need to feed
through additional information in the Closure - e.g. like extension
version/properties/icon/etc that aren't currently passed through and aren't safe
to access from the file thread. That is, this interface might need to change
later anyway.

[jackhou1, 2014-06-02 04:25:19]

tapted, PTAL

https://codereview.chromium.org/263403002/diff/150001/chrome/browser/apps/sho...
File chrome/browser/apps/shortcut_manager.cc (right):

https://codereview.chromium.org/263403002/diff/150001/chrome/browser/apps/sho...
chrome/browser/apps/shortcut_manager.cc:37: const int
kCurrentAppShortcutsVersion = 1;
On 2014/05/30 06:33:27, tapted wrote:
> Note we might want this to be per-platform. And the first time windows/linux
> gets implemented we will of course need to increment it on those platforms to
2.
> I guess we can worry about it later (#ifdef or extern const int or similar).
Or
> we could set it to 0 on win/linux initially.

 Yeah, I think ifdefs here is fine. Similar to kDesiredSizes in web_app.cc. I
also realised this should start at 0, and we should only increment when we want
to trigger an update. (There's nothing to update in this CL).

https://codereview.chromium.org/263403002/diff/150001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/263403002/diff/150001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app.cc:430: app_ids.insert((*it)->id());
On 2014/05/30 06:33:27, tapted wrote:
> Why not just call
> 
> web_app::UpdateAllShortcuts(base::string16(), profile, extension);
> 
> here?
> 
> (I'm not sure if the explanation I came up with in my head is a good one, so I
> want to hear yours :p)

Yeah, I agree with making this simpler until we actually need it. It also gives
the benefit of only jumping to the FILE thread on platforms where we actually
need to read from the filesystem first. On Mac, we can just update each one
individually and it should have the same effect.

[tapted, 2014-06-02 05:49:14]

lgtm

[jackhou1, 2014-06-03 04:21:24]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-06-03 04:23:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jackhou@chromium.org/263403002/170001

[commit-bot: I haz the power, 2014-06-03 06:14:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[commit-bot: I haz the power, 2014-06-03 13:08:43]

Change committed as 274498