[Mac] Bounce app shims when app windows request attention.

[Mac] Bounce app shims when app windows request attention.

Currently we only bounce the app shim when the app is hidden but wants
to show a window.

This CL makes AppWindow.drawAttention() bounce the app icon continuously
until the app is focused or AppWindow.clearAttention() is called.

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

[tapted, 2014-08-13 03:37:08]

I can't imagine they would be terribly interesting.. but should we add tests for
this too? (or are there already API tests for AppWindow.drawAttention() that
gives basic coverage for this?)

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/app_shim_messa...
File apps/app_shim/app_shim_messages.h (right):

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/app_shim_messa...
apps/app_shim/app_shim_messages.h:36:
IPC_MESSAGE_CONTROL1(AppShimMsg_RequestUserAttention,
So.. now that this is released, I'm not sure how safe it is to change message
sizes.

I think if you:

1. Run Chrome
2. Have un upgrade come in; downloaded waiting for a relaunch
3. Launch an App without relaunching chrome
4. that app does requestUserAttention


Then the IPC messages will get out of sync, and the shim will probably freeze or
behave randomly. Probably bad enough and hidden enough (i.e. we won't get crash
reports) to want to avoid.

Adding a new message type should work, with a TODO to remove the old IPC message
in a few milestones. But it's yucky.

Maybe our lives will be easier if we can trick the app_mode_loader to load the
framework of the chrome it's connecting to, rather than the one that will be
used next time chrome is run - WDYT?

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/chrome_main_ap...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/chrome_main_ap...
apps/app_shim/chrome_main_app_mode_mac.mm:151:
ping_chrome_reply_received_(false) {
initialize attention_request_id_

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/chrome_main_ap...
apps/app_shim/chrome_main_app_mode_mac.mm:327: break;
maybe
  case apps::APP_SHIM_ATTENTION_NUM_TYPES:
    NOTREACHED();

This means we get a compile error if an enum value is added

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/extension_app_...
File apps/app_shim/extension_app_shim_handler_mac.h (right):

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/extension_app_...
apps/app_shim/extension_app_shim_handler_mac.h:88: static bool
RequestUserAttentionForWindow(
return void? I don't think anything uses the return value

https://codereview.chromium.org/461303002/diff/1/chrome/browser/ui/cocoa/apps...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/461303002/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:736: }
is NativeAppWindowCocoa::attention_request_id_ unused now?

or....

do we need to handle the case when there is no Shim, to activate Chrome instead?
(I'm leaning towards no..)

[jackhou1, 2014-08-13 05:53:46]

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/app_shim_messa...
File apps/app_shim/app_shim_messages.h (right):

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/app_shim_messa...
apps/app_shim/app_shim_messages.h:36:
IPC_MESSAGE_CONTROL1(AppShimMsg_RequestUserAttention,
On 2014/08/13 03:37:08, tapted wrote:
> So.. now that this is released, I'm not sure how safe it is to change message
> sizes.
> 
> I think if you:
> 
> 1. Run Chrome
> 2. Have un upgrade come in; downloaded waiting for a relaunch
> 3. Launch an App without relaunching chrome
> 4. that app does requestUserAttention
> 
> 
> Then the IPC messages will get out of sync, and the shim will probably freeze
or
> behave randomly. Probably bad enough and hidden enough (i.e. we won't get
crash
> reports) to want to avoid.
> 
> Adding a new message type should work, with a TODO to remove the old IPC
message
> in a few milestones. But it's yucky.
> 
> Maybe our lives will be easier if we can trick the app_mode_loader to load the
> framework of the chrome it's connecting to, rather than the one that will be
> used next time chrome is run - WDYT?

Argh.

Just adding a new IPC message instead.

As discussed, it's probably not safe to delete any either. I added a comment
about this.

I think it'd be tedious trying to guarantee that the shim and Chrome have the
same framework version. It's easier just to make sure the IPC messages are
consistent. If for some reason we plan to add/change a heap of app shim
messages, we should probably switch to something more robust, like Mojo.

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/chrome_main_ap...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/chrome_main_ap...
apps/app_shim/chrome_main_app_mode_mac.mm:151:
ping_chrome_reply_received_(false) {
On 2014/08/13 03:37:08, tapted wrote:
> initialize attention_request_id_

Done.

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/chrome_main_ap...
apps/app_shim/chrome_main_app_mode_mac.mm:327: break;
On 2014/08/13 03:37:08, tapted wrote:
> maybe
>   case apps::APP_SHIM_ATTENTION_NUM_TYPES:
>     NOTREACHED();
> 
> This means we get a compile error if an enum value is added

Done.

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/extension_app_...
File apps/app_shim/extension_app_shim_handler_mac.h (right):

https://codereview.chromium.org/461303002/diff/1/apps/app_shim/extension_app_...
apps/app_shim/extension_app_shim_handler_mac.h:88: static bool
RequestUserAttentionForWindow(
On 2014/08/13 03:37:08, tapted wrote:
> return void? I don't think anything uses the return value

Done.

https://codereview.chromium.org/461303002/diff/1/chrome/browser/ui/cocoa/apps...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/461303002/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:736: }
On 2014/08/13 03:37:08, tapted wrote:
> is NativeAppWindowCocoa::attention_request_id_ unused now?
> 
> or....
> 
> do we need to handle the case when there is no Shim, to activate Chrome
instead?
> (I'm leaning towards no..)

Removed.

I don't think we need to do anything in the no-shim case.

[tapted, 2014-08-13 06:53:15]

Did you see my comment about tests :3

https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/app_shim_m...
File apps/app_shim/app_shim_messages.h (right):

https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/app_shim_m...
apps/app_shim/app_shim_messages.h:27: // IMPORTANT: Since app shims could be
running a different framework version to
nit: different -> newer?

https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/chrome_mai...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/chrome_mai...
apps/app_shim/chrome_main_app_mode_mac.mm:133: void OnRequestUserAttention();
since this shim is "always" newer, I don't think this can called any more...
remove?

https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/chrome_mai...
apps/app_shim/chrome_main_app_mode_mac.mm:333: case
apps::APP_SHIM_ATTENTION_NUM_TYPES:
You need a break; before this :o

[jackhou1, 2014-08-14 04:24:52]

> Did you see my comment about tests :3

Oh yeah. There's nothing that calls those methods, so I added a test that does.
It doesn't test very much though.

https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/app_shim_m...
File apps/app_shim/app_shim_messages.h (right):

https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/app_shim_m...
apps/app_shim/app_shim_messages.h:27: // IMPORTANT: Since app shims could be
running a different framework version to
On 2014/08/13 06:53:15, tapted wrote:
> nit: different -> newer?

Done.

https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/chrome_mai...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/chrome_mai...
apps/app_shim/chrome_main_app_mode_mac.mm:133: void OnRequestUserAttention();
On 2014/08/13 06:53:15, tapted wrote:
> since this shim is "always" newer, I don't think this can called any more...
> remove?

The shim is newer, so the older Chrome might send it the old message.

https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/chrome_mai...
apps/app_shim/chrome_main_app_mode_mac.mm:333: case
apps::APP_SHIM_ATTENTION_NUM_TYPES:
On 2014/08/13 06:53:15, tapted wrote:
> You need a break; before this :o

Done.

[tapted, 2014-08-14 04:38:51]

lgtm - it would be nice if the test was running with shims enabled (I think they
are still disabled for most tests?), or there was some way to verify the bouncy
state, but I can't think of one :/ (let alone a cross-platform way).

https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/chrome_mai...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/chrome_mai...
apps/app_shim/chrome_main_app_mode_mac.mm:133: void OnRequestUserAttention();
On 2014/08/14 04:24:51, jackhou1 wrote:
> On 2014/08/13 06:53:15, tapted wrote:
> > since this shim is "always" newer, I don't think this can called any more...
> > remove?
> 
> The shim is newer, so the older Chrome might send it the old message.

ohhhhh of course..

[jackhou1, 2014-08-14 05:41:23]

On 2014/08/14 04:38:51, tapted wrote:
> lgtm - it would be nice if the test was running with shims enabled (I think
they
> are still disabled for most tests?), or there was some way to verify the
bouncy
> state, but I can't think of one :/ (let alone a cross-platform way).

Yeah, they're disabled in all tests except explicit app shim tests. I don't
think this attention feature is important enough to warrant an end-to-end test.
We could maybe add an app shim test that sends it all possible IPC messages,
verify whatever behavior we can, and check that none of them cause a crash.

>
https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/chrome_mai...
> File apps/app_shim/chrome_main_app_mode_mac.mm (right):
> 
>
https://codereview.chromium.org/461303002/diff/20001/apps/app_shim/chrome_mai...
> apps/app_shim/chrome_main_app_mode_mac.mm:133: void OnRequestUserAttention();
> On 2014/08/14 04:24:51, jackhou1 wrote:
> > On 2014/08/13 06:53:15, tapted wrote:
> > > since this shim is "always" newer, I don't think this can called any
more...
> > > remove?
> > 
> > The shim is newer, so the older Chrome might send it the old message.
> 
> ohhhhh of course..

[jackhou1, 2014-08-14 05:41:41]

palmer, could you please review IPC changes for security?

[jackhou1, 2014-08-19 03:09:54]

Ping. Also adding rsesek in case palmer is away/busy.

[Robert Sesek, 2014-08-19 17:05:26]

On 2014/08/19 03:09:54, jackhou1 wrote:
> Ping. Also adding rsesek in case palmer is away/busy.

Unfortunately I'm not an OWNER for *_messages.h.

[chromium-reviews, 2014-08-19 17:10:46]

I'll get to it this afternoon. I'm way behind on reviews after being OOO
for a bunch of days.
On Aug 19, 2014 10:05 AM, <rsesek@chromium.org> wrote:

> On 2014/08/19 03:09:54, jackhou1 wrote:
>
>> Ping. Also adding rsesek in case palmer is away/busy.
>>
>
> Unfortunately I'm not an OWNER for *_messages.h.
>
> https://codereview.chromium.org/461303002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[palmer, 2014-08-19 18:14:15]

lgtm

[jackhou1, 2014-08-19 23:49:21]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-08-19 23:50:43]

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

[commit-bot: I haz the power, 2014-08-20 01:11:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-20 01:18:36]

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

[commit-bot: I haz the power, 2014-08-20 01:18:37]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jackhou1, 2014-08-20 02:36:23]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-08-20 02:36:58]

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

[commit-bot: I haz the power, 2014-08-20 04:26:36]

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

[commit-bot: I haz the power, 2014-08-20 05:18:08]

Committed patchset #4 (60001) as 290768