Don't send a SHChangeNotify for creating an app icon when creating a shortcut.

Don't send a SHChangeNotify for creating an app icon when creating a shortcut.

This CL fixes an issue with the initial app sync on sign-in causing many
taskbar flashes due to icon creation always sending a SHChangeNotify.

This shouldn't be necessary for icon creation due to shortcut creation so
avoid SHChangeNotify in this case.

BUG=470002
Committed: https://crrev.com/e823d885146e3afea82f31a685bbab560a523e3f
Cr-Commit-Position: refs/heads/master@{#322324}

[calamity, 2015-03-24 07:35:08]

calamity@chromium.org changed reviewers:
+ mgiuca@chromium.org

[calamity, 2015-03-24 07:35:09]

https://codereview.chromium.org/1026113003/diff/1/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/1026113003/diff/1/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_win.cc:588: NULL);
This logic should be identical to the old code except for the
is_shortcut_creation part.

[Matt Giuca, 2015-03-25 03:31:19]

https://codereview.chromium.org/1026113003/diff/1/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/1026113003/diff/1/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_win.cc:580: // Shortcut creation doesn't
require a SHChangeNotify as the icon will be
will be ...?

(I hate cliffhangers.)

https://codereview.chromium.org/1026113003/diff/1/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_win.cc:588: NULL);
I like the refactoring you did here, but I think this last part should be inside
an if statement:

if (!is_shortcut_creation) {
  SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT, NULL,
                 NULL);
}

Because the return value is the same anyway; I prefer not to think of this as an
early exit, but conditional execution of the SHChangeNotify.

https://codereview.chromium.org/1026113003/diff/1/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_win.h (right):

https://codereview.chromium.org/1026113003/diff/1/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_win.h:42: bool is_shortcut_creation);
Call this "do_shchangenotify" (and invert the truth value).

I don't like argument names that explain the situation with respect to the
caller, and let the callee make a policy decision based on that. That's high
coupling. Instead, let the caller tell the API exactly what it wants, and the
API just blindly does it.

[calamity, 2015-03-25 04:32:32]

Patchset #2 (id:20001) has been deleted

[calamity, 2015-03-25 04:37:23]

https://codereview.chromium.org/1026113003/diff/1/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/1026113003/diff/1/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_win.cc:580: // Shortcut creation doesn't
require a SHChangeNotify as the icon will be
On 2015/03/25 03:31:18, Matt Giuca wrote:
> will be ...?
> 
> (I hate cliffhangers.)

Moved to header comment.

https://codereview.chromium.org/1026113003/diff/1/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_win.cc:588: NULL);
On 2015/03/25 03:31:18, Matt Giuca wrote:
> I like the refactoring you did here, but I think this last part should be
inside
> an if statement:
> 
> if (!is_shortcut_creation) {
>   SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT, NULL,
>                  NULL);
> }
> 
> Because the return value is the same anyway; I prefer not to think of this as
an
> early exit, but conditional execution of the SHChangeNotify.

Done.

https://codereview.chromium.org/1026113003/diff/1/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_win.h (right):

https://codereview.chromium.org/1026113003/diff/1/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_win.h:42: bool is_shortcut_creation);
On 2015/03/25 03:31:19, Matt Giuca wrote:
> Call this "do_shchangenotify" (and invert the truth value).
> 
> I don't like argument names that explain the situation with respect to the
> caller, and let the callee make a policy decision based on that. That's high
> coupling. Instead, let the caller tell the API exactly what it wants, and the
> API just blindly does it.

Changed it to refresh_shell_icon_cache.

[Matt Giuca, 2015-03-25 06:20:53]

lgtm nits

https://codereview.chromium.org/1026113003/diff/40001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app_win.h (right):

https://codereview.chromium.org/1026113003/diff/40001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_win.h:43: // shell's icon cache will be
refreshed, causing a flicker. Refreshing the icon
"and refresh shell's icon cache to ensure correct icon is displayed" --- weird
to have that in the opening sentence when it is conditional.

Why not delete that clause of the first sentence, and elaborate here where you
talk about the parameter:
"If |refresh_shell_icon_cache| is true, the shell's icon cache will be
refreshed, ensuring the correct icon is displayed, but causing a flicker."

https://codereview.chromium.org/1026113003/diff/40001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_win.h:44: // cache is not necessary on
shortcut creation as the shell wil be notified when
nit: wil

[calamity, 2015-03-26 04:09:43]

Patchset #3 (id:60001) has been deleted

[calamity, 2015-03-26 04:09:52]

https://codereview.chromium.org/1026113003/diff/40001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app_win.h (right):

https://codereview.chromium.org/1026113003/diff/40001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_win.h:43: // shell's icon cache will be
refreshed, causing a flicker. Refreshing the icon
On 2015/03/25 06:20:52, Matt Giuca wrote:
> "and refresh shell's icon cache to ensure correct icon is displayed" --- weird
> to have that in the opening sentence when it is conditional.
> 
> Why not delete that clause of the first sentence, and elaborate here where you
> talk about the parameter:
> "If |refresh_shell_icon_cache| is true, the shell's icon cache will be
> refreshed, ensuring the correct icon is displayed, but causing a flicker."

Done.

https://codereview.chromium.org/1026113003/diff/40001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_win.h:44: // cache is not necessary on
shortcut creation as the shell wil be notified when
On 2015/03/25 06:20:53, Matt Giuca wrote:
> nit: wil

Done.

[calamity, 2015-03-26 04:09:56]

The CQ bit was checked by calamity@chromium.org

[calamity, 2015-03-26 04:09:56]

The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org
Link to the patchset: https://codereview.chromium.org/1026113003/#ps80001
(title: "fix nits")

[commit-bot: I haz the power, 2015-03-26 04:10:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1026113003/80001

[commit-bot: I haz the power, 2015-03-26 04:54:52]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-03-26 04:55:32]

Patchset 3 (id:??) landed as
https://crrev.com/e823d885146e3afea82f31a685bbab560a523e3f
Cr-Commit-Position: refs/heads/master@{#322324}