App windows launched in windows ASH should not be messing with the AppUserModelId for the remote HW…

App windows launched in windows ASH should not be messing with the AppUserModelId for the remote HWND.

This causes problems on Windows 7 Chrome OS mode where launching popup app windows in Chrome OS mode
causes the taskbar button for the remote HWND to change to the application button. On Windows 8
these calls silently fail.

BUG=409824
Committed: https://crrev.com/10c902754f40ffe3d7ff1685293e6f5b16b809f3
Cr-Commit-Position: refs/heads/master@{#296347}

[ananta, 2014-09-23 00:40:07]

ananta@chromium.org changed reviewers:
+ tapted@chromium.org

[tapted, 2014-09-23 01:24:39]

https://codereview.chromium.org/589303002/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc (right):

https://codereview.chromium.org/589303002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc:115: if
(!IsRunningInAsh()) {
I don't think we want the stuff in UpdateShelfMenu() either - that's for
updating the taskbar jumplist entries. Also, EnsureCaptionStyleSet()  below is
already in !IsRunningInAsh.

I think we can remove the `!IsRunningInAsh()` from the condition below, and here
just do

  // Remaining initialization is for Windows shell integration, which doesn't
  // apply to app windows in Ash.
  if (IsRunningInAsh())
    return;


But UpdateShelfMenu() is called from other code, and it depends on app_model_id_
being set (which now won't happen, since it's a few lines down from here).

So UpdateShelfMenu now also needs an update to bail out. Probably:

void ChromeNativeAppWindowViewsWin::UpdateShelfMenu() {
  if (!JumpListUpdater::IsEnabled() || IsRunningInAsh())
    return;

  DCHECK(!app_model_id_.empty());
  /* etc */
}


(we could also exchange app_model_id_.empty <-> IsRunningInAsh there, but I
think the former will get more error cases. But note there's not much bot
coverage for jump lists, since in tests Windows picks up a random prefix that
results in lots of small files in system dirs which explode bot disk space over
time)


Then can you update the comment on ChromeNativeAppWindowViewsWin::app_model_id_.
Just to add, "Not set for windows running inside Ash.".

(we could also just set it, but I think that's less robust).

[ananta, 2014-09-23 18:18:49]

https://codereview.chromium.org/589303002/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc (right):

https://codereview.chromium.org/589303002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc:115: if
(!IsRunningInAsh()) {
On 2014/09/23 01:24:39, tapted wrote:
> I don't think we want the stuff in UpdateShelfMenu() either - that's for
> updating the taskbar jumplist entries. Also, EnsureCaptionStyleSet()  below is
> already in !IsRunningInAsh.
> 
> I think we can remove the `!IsRunningInAsh()` from the condition below, and
here
> just do
> 
>   // Remaining initialization is for Windows shell integration, which doesn't
>   // apply to app windows in Ash.
>   if (IsRunningInAsh())
>     return;
> 
> 
> But UpdateShelfMenu() is called from other code, and it depends on
app_model_id_
> being set (which now won't happen, since it's a few lines down from here).
> 
> So UpdateShelfMenu now also needs an update to bail out. Probably:
> 
> void ChromeNativeAppWindowViewsWin::UpdateShelfMenu() {
>   if (!JumpListUpdater::IsEnabled() || IsRunningInAsh())
>     return;
> 
>   DCHECK(!app_model_id_.empty());
>   /* etc */
> }
> 
> 
> (we could also exchange app_model_id_.empty <-> IsRunningInAsh there, but I
> think the former will get more error cases. But note there's not much bot
> coverage for jump lists, since in tests Windows picks up a random prefix that
> results in lots of small files in system dirs which explode bot disk space
over
> time)
> 
> 
> Then can you update the comment on
ChromeNativeAppWindowViewsWin::app_model_id_.
> Just to add, "Not set for windows running inside Ash.".
> 
> (we could also just set it, but I think that's less robust).

Done.

[tapted, 2014-09-23 23:42:42]

thanks! lgtm

[ananta, 2014-09-24 00:03:04]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-09-24 00:03:40]

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

[commit-bot: I haz the power, 2014-09-24 00:09:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-24 00:09:34]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[ananta, 2014-09-24 00:20:28]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-09-24 00:21:25]

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

[commit-bot: I haz the power, 2014-09-24 00:26:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-24 00:26:27]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)

[ananta, 2014-09-24 04:32:43]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-09-24 04:33:22]

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

[commit-bot: I haz the power, 2014-09-24 04:43:30]

Committed patchset #2 (id:20001) as 398bc55037d8155451a52627e99858e95c321544

[commit-bot: I haz the power, 2014-09-24 04:44:12]

Patchset 2 (id:??) landed as
https://crrev.com/10c902754f40ffe3d7ff1685293e6f5b16b809f3
Cr-Commit-Position: refs/heads/master@{#296347}