Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2848813002:
Introduce ash mojo interface for lock screen action handlers (Closed)
|
Created:
3 years, 7 months ago by tbarzic Modified:
3 years, 7 months ago CC:
chromium-reviews, sadrul, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, tfarina, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, oshima+watch_chromium.org, kalyank, darin (slow to review), davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce ash mojo interface for lock screen action handlers
This is part of support for note taking apps on lock screen.
This is the interface that will enable ash to observe state of
apps that can/are allowed to handle actions (currently, only
"new note" action is supported) on lock screen and update it's
state accordingly.
For example, this interface will be used for tray icon for launching
new note. The icon will be exposed only if the action handler
controller is in the state indicating that there is an app registered
for handling new note action on lock screen.
Clicking the icon would issue a request to action handler to handle
the action (thus adding RequestHandleAction interface).
BUG=716081
Review-Url: https://codereview.chromium.org/2848813002
Cr-Original-Commit-Position: refs/heads/master@{#469870}
Committed: https://chromium.googlesource.com/chromium/src/+/033f267aec04cedfa513cef1880e9dc81dc83648
Review-Url: https://codereview.chromium.org/2848813002
Cr-Commit-Position: refs/heads/master@{#470115}
Committed: https://chromium.googlesource.com/chromium/src/+/c78da1ff9f01a79c89aa715e2dd42ff0dfde0cec
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #
Total comments: 2
Patch Set 7 : rename to TrayActionHandler #Patch Set 8 : . #
Total comments: 49
Patch Set 9 : . #Patch Set 10 : . #Patch Set 11 : . #Patch Set 12 : . #
Total comments: 14
Patch Set 13 : . #Patch Set 14 : . #Patch Set 15 : . #Patch Set 16 : . #
Total comments: 53
Patch Set 17 : . #Patch Set 18 : . #Patch Set 19 : . #
Total comments: 10
Patch Set 20 : . #Patch Set 21 : Fix outdated comment #Patch Set 22 : add ASH_EXPORTS #Messages
Total messages: 82 (53 generated)
The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Introduce ash mojo interface for managing lock screen actions BUG=716081 ========== to ========== Introduce ash mojo interface for lock screen action handlers This is part of support for note taking apps on lock screen. This is the interface that will enable ash to observe state of apps that can/are allowed to handle actions (currently, only "new note" action is supported) on lock screen and update it's state accordingly. For example, this interface will be used for tray icon for launching new note. The icon will be exposed only if the action handler controller is in the state indicating that there is an app registered for handling new note action on lock screen. Clicking the icon would issue a request to action handler to handle the action (thus adding RequestHandleAction interface). BUG=716081 ==========
tbarzic@chromium.org changed reviewers: + jamescook@chromium.org, jdufault@chromium.org
NOTE: this depends on https://codereview.chromium.org/2839383002/
jamescook@chromium.org changed reviewers: + xiyuan@chromium.org
Is there a design doc or mocks for this feature? Also, I think xiyuan should look at this, since he's been working on mojo and lock screen stuff recently. I didn't look at the code, just skimmed the mojo interface. https://codereview.chromium.org/2848813002/diff/100001/ash/public/interfaces/... File ash/public/interfaces/action_handler.mojom (right): https://codereview.chromium.org/2848813002/diff/100001/ash/public/interfaces/... ash/public/interfaces/action_handler.mojom:32: interface ActionHandlerStateController { "ActionHandler" seems like a very generic name. Is the intent that this will be used for all sorts of generic actions, or mostly lock screen and/or shelf actions? If it's lock screen/shelf the name should reflect that. Also, our usual naming pattern is: FooController (in ash) FooControllerClient or FooClient (in chrome)
I linked docs in the issue tracker. https://codereview.chromium.org/2848813002/diff/100001/ash/public/interfaces/... File ash/public/interfaces/action_handler.mojom (right): https://codereview.chromium.org/2848813002/diff/100001/ash/public/interfaces/... ash/public/interfaces/action_handler.mojom:32: interface ActionHandlerStateController { On 2017/05/02 15:43:38, James Cook wrote: > "ActionHandler" seems like a very generic name. Is the intent that this will be > used for all sorts of generic actions, or mostly lock screen and/or shelf > actions? If it's lock screen/shelf the name should reflect that. > > Also, our usual naming pattern is: > > FooController (in ash) > FooControllerClient or FooClient (in chrome) Yeah, I'm not to content with the name either. The intent is to use this with Chrome apps that support action handler API - which currently supports a single action - new note. Though, from ash perspective there is nothing that would stop ash to use this interface for more generic actions. I guess ShelfActionHandler might be slightly better naming. Other option I considered was AppActionHandler, though "app" seems more like Chrome than ash concept (and the fact these actions are handled by chrome/arc apps seems like the client implementation detail).
On 2017/05/02 17:05:54, tbarzic wrote: > I linked docs in the issue tracker. > > https://codereview.chromium.org/2848813002/diff/100001/ash/public/interfaces/... > File ash/public/interfaces/action_handler.mojom (right): > > https://codereview.chromium.org/2848813002/diff/100001/ash/public/interfaces/... > ash/public/interfaces/action_handler.mojom:32: interface > ActionHandlerStateController { > On 2017/05/02 15:43:38, James Cook wrote: > > "ActionHandler" seems like a very generic name. Is the intent that this will > be > > used for all sorts of generic actions, or mostly lock screen and/or shelf > > actions? If it's lock screen/shelf the name should reflect that. > > > > Also, our usual naming pattern is: > > > > FooController (in ash) > > FooControllerClient or FooClient (in chrome) > > Yeah, I'm not to content with the name either. > > The intent is to use this with Chrome apps that support action handler API - > which currently supports a single action - new note. > > Though, from ash perspective there is nothing that would stop ash to use this > interface for more generic actions. > > I guess ShelfActionHandler might be slightly better naming. > > Other option I considered was AppActionHandler, though "app" seems more like > Chrome than ash concept (and the fact these actions are handled by chrome/arc > apps seems like the client implementation detail). I would call it TrayActionHandler since the mock UI is in in status area rather than in shelf.
Do you want me to start looking at this now, or should I wait for a rename pass?
On 2017/05/03 00:19:10, James Cook wrote: > Do you want me to start looking at this now, or should I wait for a rename pass? let me rename it first
I renamed the interface to TrayActionHandler
Sorry for all the comments - we're still sorting out the preferred style for new mojo interfaces in ash and you're one of the first victims. :-) I'm glad to see you were able to get something working and write tests for it! https://codereview.chromium.org/2848813002/diff/140001/ash/public/interfaces/... File ash/public/interfaces/tray_action_handler.mojom (right): https://codereview.chromium.org/2848813002/diff/140001/ash/public/interfaces/... ash/public/interfaces/tray_action_handler.mojom:8: enum TrayActionHandlerAction { kNewLockScreenNote, }; It doesn't seem like we will have many of these actions, at least for now. This seems like premature generalization. I would prefer to hard-code the concept of taking a note into this interface (like have methods SetLockScreenNoteState / StartLockScreenNote or similar below). https://codereview.chromium.org/2848813002/diff/140001/ash/public/interfaces/... ash/public/interfaces/tray_action_handler.mojom:28: kBackground, I don't understand this state. Why would the app be in the background? It seems like it should either be running and active, or it should be closed. https://codereview.chromium.org/2848813002/diff/140001/ash/public/interfaces/... ash/public/interfaces/tray_action_handler.mojom:33: interface TrayActionHandlerController { Sorry to thrash on names. Our preferred naming style is: mojom::Foo for the mojo interface (here) ash::Foo for the implementation in ash FooClient for the client/delegate in chrome The word "controller" in existing interfaces reflects the fact we were bolting things onto existing ash classes called controller. In this case, what do you think of: mojom::TrayAction / ash::TrayAction for the ash piece (It's an "action" button that lives on the tray) and TrayActionClient or TrayActionHandler in chrome btw, I like to use tools/git/move_source_file.py and tools/git/mffr.py for these sorts of renames https://codereview.chromium.org/2848813002/diff/140001/ash/public/interfaces/... ash/public/interfaces/tray_action_handler.mojom:37: // Updates action handler state for a tray action. Is it required to have a client before calling this? Either way, document it. https://codereview.chromium.org/2848813002/diff/140001/ash/public/interfaces/... ash/public/interfaces/tray_action_handler.mojom:46: }; Nice interface docs, btw. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... File ash/tray_action_handler/tray_action_handler_controller.cc (right): https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller.cc:14: TrayActionHandlerController::TrayActionHandlerController() {} optional: = default https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller.cc:44: mojom::TrayActionHandlerState old_effective_new_note_state = What does "effective" mean here? Isn't this just "old_state"? https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... File ash/tray_action_handler/tray_action_handler_controller.h (right): https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller.h:43: // mojom::TrayActionHandlerController impl: super-nit: Just "mojom::TrayActionHandlerController:" or "mojom::TrayActionHandlerController overrides:" https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... File ash/tray_action_handler/tray_action_handler_controller_unittest.cc (right): https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:10: #include "ash/tray_action_handler/tray_action_handler_controller.h" nit: put this include at the top https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:51: class ScopedTestTrayActionHandler : public mojom::TrayActionHandlerClient { nit: I think the tests would be easier to understand if they manually called a SetClient() method. You could also call this TestTrayActionHandlerClient if you want a shorter name. For example, this class could expose a CreateInterfacePtrAndBind() method and the test code could call controller->SetClient(action_handler_client.CreateInterfacePtrAndBind()) and controller->SetClient(nullptr) Or if that doesn't seem clearer, just rename this to TestTrayAction(Handler)Client to emphasize it is setting itself as the client. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:82: class TrayActionHandlerControllerTest : public test::AshTestBase { optional: "using TrayActionHandlerControllerTest = test::AshTestBase" and make GetController() a utility method, or just inline it. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:84: TrayActionHandlerControllerTest() {} optional nit: = default instead https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:95: } // namespace nit: can this go at the bottom (put all the test code in the anonymous namespace)? https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:102: mojom::TrayActionHandlerState::kNotSupported, optional: I think you may be able to do "using mojom::TrayActionHandlerState;" at file scope to make these names shorter. mojoms generate enum class. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:179: } I suggest one test that moves through the expected state sequence (not available, available, launching, active, background) then another small test that checks that observers are not notified for a duplicate transition. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:200: } // namespace ash Thanks for writing a nice test suite! https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... File ash/tray_action_handler/tray_action_handler_observer.h (right): https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_observer.h:8: #include "ash/ash_export.h" not needed? https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_observer.h:14: class TrayActionHandlerObserver { This doesn't look used - do you need it for a follow up CL? https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/tray_action_handler_client.cc (right): https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/tray_action_handler_client.cc:18: case lock_screen_apps::ActionState::kNotSupported: Can these two action state enums go somewhere shared and get used by both ash and chrome? Either use the ash mojom version in lock_screen_apps, or introduce a shared enum into ash/public/cpp and add EnumTraits for mojo? https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/tray_action_handler_client.cc:28: case lock_screen_apps::ActionState::kHidden: Why is there a difference in name between "hidden" and "background"? https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/tray_action_handler_client.cc:42: binding_(this) {} nit: DCHECK(state_controller_) https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/tray_action_handler_client.cc:47: if (!lock_screen_apps_state_controller_) Can this happen in production? https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/tray_action_handler_client.h (right): https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/tray_action_handler_client.h:20: class TrayActionHandlerClient : public ash::mojom::TrayActionHandlerClient, High-level comment: Do you need this class at all? The lock_screen_apps::StateController could be the TrayActionClient, and it could grab the mojo interface pointer and call it when necessary. That would eliminate a layer of observers and calls back and forth. If you do decide to keep this class, it needs test coverage. https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/tray_action_handler_client.h:30: // ash::mojom::ActionHandler impl: nit: no "impl:", just ":" or "overrides:" or something else more common in chrome https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right): https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:113: tray_action_handler_client_ = base::MakeUnique<TrayActionHandlerClient>( If you still need this class, can it be owned by ChromeBrowserMainPartsChromeOS instead?
Oh, and at a high level... Unless there are specific UX plans for lock screen actions other than note taking I think you should hard code the concept of "take note" into the classes and mojo interfaces. Extensions need to be generic since its a public API we have to support forever, but the mojom stuff is internal only. We can always make it more generic later.
The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2848813002/diff/140001/ash/public/interfaces/... File ash/public/interfaces/tray_action_handler.mojom (right): https://codereview.chromium.org/2848813002/diff/140001/ash/public/interfaces/... ash/public/interfaces/tray_action_handler.mojom:8: enum TrayActionHandlerAction { kNewLockScreenNote, }; On 2017/05/03 23:31:50, James Cook wrote: > It doesn't seem like we will have many of these actions, at least for now. This > seems like premature generalization. > > I would prefer to hard-code the concept of taking a note into this interface > (like have methods SetLockScreenNoteState / StartLockScreenNote or similar > below). OK, sounds good, I've myself started considering the same - I do like to generalize APIs; but yeah, given this is internal API, and should be easy to change later, less generalization is more readable/easier to use. https://codereview.chromium.org/2848813002/diff/140001/ash/public/interfaces/... ash/public/interfaces/tray_action_handler.mojom:28: kBackground, On 2017/05/03 23:31:50, James Cook wrote: > I don't understand this state. Why would the app be in the background? It seems > like it should either be running and active, or it should be closed. To preserve the state of the app window (and be able to show it behind the user pods) when lock screen user pod UI is active. https://codereview.chromium.org/2848813002/diff/140001/ash/public/interfaces/... ash/public/interfaces/tray_action_handler.mojom:33: interface TrayActionHandlerController { On 2017/05/03 23:31:50, James Cook wrote: > Sorry to thrash on names. Our preferred naming style is: > > mojom::Foo for the mojo interface (here) > ash::Foo for the implementation in ash > FooClient for the client/delegate in chrome > > The word "controller" in existing interfaces reflects the fact we were bolting > things onto existing ash classes called controller. > > In this case, what do you think of: > > mojom::TrayAction / ash::TrayAction for the ash piece (It's an "action" button > that lives on the tray) > > and > > TrayActionClient or TrayActionHandler in chrome > > btw, I like to use tools/git/move_source_file.py and tools/git/mffr.py for these > sorts of renames Don't be sorry :) I like having better names... https://codereview.chromium.org/2848813002/diff/140001/ash/public/interfaces/... ash/public/interfaces/tray_action_handler.mojom:37: // Updates action handler state for a tray action. On 2017/05/03 23:31:50, James Cook wrote: > Is it required to have a client before calling this? Either way, document it. Done. https://codereview.chromium.org/2848813002/diff/140001/ash/public/interfaces/... ash/public/interfaces/tray_action_handler.mojom:46: }; On 2017/05/03 23:31:50, James Cook wrote: > Nice interface docs, btw. Acknowledged. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... File ash/tray_action_handler/tray_action_handler_controller.cc (right): https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller.cc:14: TrayActionHandlerController::TrayActionHandlerController() {} On 2017/05/03 23:31:51, James Cook wrote: > optional: = default Done. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller.cc:44: mojom::TrayActionHandlerState old_effective_new_note_state = On 2017/05/03 23:31:50, James Cook wrote: > What does "effective" mean here? Isn't this just "old_state"? yeah, pretty much https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... File ash/tray_action_handler/tray_action_handler_controller.h (right): https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller.h:43: // mojom::TrayActionHandlerController impl: On 2017/05/03 23:31:51, James Cook wrote: > super-nit: Just "mojom::TrayActionHandlerController:" or > "mojom::TrayActionHandlerController overrides:" Done. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... File ash/tray_action_handler/tray_action_handler_controller_unittest.cc (right): https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:10: #include "ash/tray_action_handler/tray_action_handler_controller.h" On 2017/05/03 23:31:51, James Cook wrote: > nit: put this include at the top Done. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:51: class ScopedTestTrayActionHandler : public mojom::TrayActionHandlerClient { On 2017/05/03 23:31:51, James Cook wrote: > nit: I think the tests would be easier to understand if they manually called a > SetClient() method. You could also call this TestTrayActionHandlerClient if you > want a shorter name. > > For example, this class could expose a CreateInterfacePtrAndBind() method and > the test code could call > controller->SetClient(action_handler_client.CreateInterfacePtrAndBind()) and > controller->SetClient(nullptr) > > Or if that doesn't seem clearer, just rename this to > TestTrayAction(Handler)Client to emphasize it is setting itself as the client. Done. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:82: class TrayActionHandlerControllerTest : public test::AshTestBase { On 2017/05/03 23:31:51, James Cook wrote: > optional: "using TrayActionHandlerControllerTest = test::AshTestBase" and make > GetController() a utility method, or just inline it. Done. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:84: TrayActionHandlerControllerTest() {} On 2017/05/03 23:31:51, James Cook wrote: > optional nit: = default instead Done. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:95: } // namespace On 2017/05/03 23:31:51, James Cook wrote: > nit: can this go at the bottom (put all the test code in the anonymous > namespace)? I vaguely remember a chromium-dev thread that suggested against anonymous-namespacing TEST_Fs (I think in order to better cope with test name conflicts). Though, I don't think that was a particularly strong guideline, so I can move it down, if you'd prefer it that way. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:102: mojom::TrayActionHandlerState::kNotSupported, On 2017/05/03 23:31:51, James Cook wrote: > optional: I think you may be able to do "using mojom::TrayActionHandlerState;" > at file scope to make these names shorter. mojoms generate enum class. Done. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_controller_unittest.cc:179: } On 2017/05/03 23:31:51, James Cook wrote: > I suggest one test that moves through the expected state sequence (not > available, available, launching, active, background) then another small test > that checks that observers are not notified for a duplicate transition. Done. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... File ash/tray_action_handler/tray_action_handler_observer.h (right): https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_observer.h:8: #include "ash/ash_export.h" On 2017/05/03 23:31:51, James Cook wrote: > not needed? Done. https://codereview.chromium.org/2848813002/diff/140001/ash/tray_action_handle... ash/tray_action_handler/tray_action_handler_observer.h:14: class TrayActionHandlerObserver { On 2017/05/03 23:31:51, James Cook wrote: > This doesn't look used - do you need it for a follow up CL? yes, it's needed for a follow-up cl https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/tray_action_handler_client.cc (right): https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/tray_action_handler_client.cc:18: case lock_screen_apps::ActionState::kNotSupported: On 2017/05/03 23:31:51, James Cook wrote: > Can these two action state enums go somewhere shared and get used by both ash > and chrome? Either use the ash mojom version in lock_screen_apps, or introduce a > shared enum into ash/public/cpp and add EnumTraits for mojo? Done. https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/tray_action_handler_client.cc:28: case lock_screen_apps::ActionState::kHidden: On 2017/05/03 23:31:51, James Cook wrote: > Why is there a difference in name between "hidden" and "background"? I changed my mind about naming after lading kHidden name :), though, it's a moot point now. https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/tray_action_handler_client.cc:42: binding_(this) {} On 2017/05/03 23:31:51, James Cook wrote: > nit: DCHECK(state_controller_) Acknowledged. https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/tray_action_handler_client.cc:47: if (!lock_screen_apps_state_controller_) On 2017/05/03 23:31:51, James Cook wrote: > Can this happen in production? It could happen, in case lock screen apps were not enabled (via flag). Though, I cleaned this up a little (now instead of later, as I originally planned). https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/tray_action_handler_client.h (right): https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/tray_action_handler_client.h:20: class TrayActionHandlerClient : public ash::mojom::TrayActionHandlerClient, On 2017/05/03 23:31:52, James Cook wrote: > High-level comment: Do you need this class at all? The > lock_screen_apps::StateController could be the TrayActionClient, and it could > grab the mojo interface pointer and call it when necessary. That would eliminate > a layer of observers and calls back and forth. > > If you do decide to keep this class, it needs test coverage. OK, done. I though it might be useful to separate concepts to make the interface easier to reuse for other actions, but I guess that can be done later, if needed. https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/tray_action_handler_client.h:30: // ash::mojom::ActionHandler impl: On 2017/05/03 23:31:51, James Cook wrote: > nit: no "impl:", just ":" or "overrides:" or something else more common in > chrome Done. https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right): https://codereview.chromium.org/2848813002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:113: tray_action_handler_client_ = base::MakeUnique<TrayActionHandlerClient>( On 2017/05/03 23:31:52, James Cook wrote: > If you still need this class, can it be owned by ChromeBrowserMainPartsChromeOS > instead? Done - kinda, I made ChroomeBrowserMainPartsChromeOS effectively own lock_screen_apps::StateController
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
https://codereview.chromium.org/2848813002/diff/220001/ash/public/interfaces/... File ash/public/interfaces/tray_action.mojom (right): https://codereview.chromium.org/2848813002/diff/220001/ash/public/interfaces/... ash/public/interfaces/tray_action.mojom:36: SetClient(TrayActionClient action_handler); nit: wrong intent https://codereview.chromium.org/2848813002/diff/220001/ash/tray_action/tray_a... File ash/tray_action/tray_action.cc (right): https://codereview.chromium.org/2848813002/diff/220001/ash/tray_action/tray_a... ash/tray_action/tray_action.cc:43: if (GetLockScreenNoteState() != old_lock_screen_note_state) Do we need to worry about SetClient to nullptr case? If we do, we probably should handle connection lost and having an Initialize(client, state_in_client) to set client as well as setting state in the client. https://codereview.chromium.org/2848813002/diff/220001/ash/tray_action/tray_a... ash/tray_action/tray_action.cc:53: if (tray_action_client_) Why the notification does not need to fire without an |tray_action_client_| ? I would think whether there is an |tray_action_client_| is irrelevant on the state change notification. https://codereview.chromium.org/2848813002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.cc (right): https://codereview.chromium.org/2848813002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:46: g_instance = instance.get(); nit: move this to ctor and rremove the one in CreateForTesting on line 62 ? https://codereview.chromium.org/2848813002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:66: // static nit: remove ? https://codereview.chromium.org/2848813002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:121: if (state == TrayActionState::kBackground && nit: comment that we only allow go to kBackground from kActive ? https://codereview.chromium.org/2848813002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.h (right): https://codereview.chromium.org/2848813002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.h:29: // Returns global StateControll instance. Note that this can return nullptr nit: StateControll -> StateController
The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run
https://codereview.chromium.org/2848813002/diff/220001/ash/public/interfaces/... File ash/public/interfaces/tray_action.mojom (right): https://codereview.chromium.org/2848813002/diff/220001/ash/public/interfaces/... ash/public/interfaces/tray_action.mojom:36: SetClient(TrayActionClient action_handler); On 2017/05/04 21:19:16, xiyuan wrote: > nit: wrong intent Done. https://codereview.chromium.org/2848813002/diff/220001/ash/tray_action/tray_a... File ash/tray_action/tray_action.cc (right): https://codereview.chromium.org/2848813002/diff/220001/ash/tray_action/tray_a... ash/tray_action/tray_action.cc:43: if (GetLockScreenNoteState() != old_lock_screen_note_state) On 2017/05/04 21:19:16, xiyuan wrote: > Do we need to worry about SetClient to nullptr case? If we do, we probably > should handle connection lost and having an Initialize(client, state_in_client) > to set client as well as setting state in the client. Yeah good point, I planned to do this, but it slipped my mind :/ https://codereview.chromium.org/2848813002/diff/220001/ash/tray_action/tray_a... ash/tray_action/tray_action.cc:53: if (tray_action_client_) On 2017/05/04 21:19:16, xiyuan wrote: > Why the notification does not need to fire without an |tray_action_client_| ? I > would think whether there is an |tray_action_client_| is irrelevant on the state > change notification. If client us not set, the effective action state has not changed - it's not supported (as ash cannot handle the action itself). https://codereview.chromium.org/2848813002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.cc (right): https://codereview.chromium.org/2848813002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:46: g_instance = instance.get(); On 2017/05/04 21:19:16, xiyuan wrote: > nit: move this to ctor and rremove the one in CreateForTesting on line 62 ? Done. https://codereview.chromium.org/2848813002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:66: // static On 2017/05/04 21:19:16, xiyuan wrote: > nit: remove ? Done. https://codereview.chromium.org/2848813002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:121: if (state == TrayActionState::kBackground && On 2017/05/04 21:19:16, xiyuan wrote: > nit: comment that we only allow go to kBackground from kActive ? Done. https://codereview.chromium.org/2848813002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.h (right): https://codereview.chromium.org/2848813002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.h:29: // Returns global StateControll instance. Note that this can return nullptr On 2017/05/04 21:19:16, xiyuan wrote: > nit: StateControll -> StateController Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2848813002/diff/300001/ash/public/interfaces/... File ash/public/interfaces/tray_action.mojom (right): https://codereview.chromium.org/2848813002/diff/300001/ash/public/interfaces/... ash/public/interfaces/tray_action.mojom:28: // surfeace user sign-in UI). nit: surface Or rewrite as: "(e.g. the handler window for the lock screen action was moved to the background and appears underneath the login user pods)" https://codereview.chromium.org/2848813002/diff/300001/ash/public/interfaces/... ash/public/interfaces/tray_action.mojom:33: // state changes. nit: I would also say what a tray action is. For example, "Allows a client (e.g. Chrome) to create a "tray action" button that appears in the status area and invokes a client/callback method when clicked." https://codereview.chromium.org/2848813002/diff/300001/ash/public/interfaces/... ash/public/interfaces/tray_action.mojom:34: interface TrayAction { optional: This API is OK for now, but in the long run I wonder if it would be better to make the API look like: AddTrayAction(TrayActionState initial_state, TrayActionObserver observer) or AddTrayAction(ImageSkia icon, TrayActionObserver observer) or AddLockScreenTrayAction(TrayActionObserver observer) the initial hard-coded version could be something like: AddLockScreenTrayAction(...) and interface TrayActionObserver { OnClicked(...modifier keys...); } or interface TrayActionHandler { OnEvent(...event...); } Functionally it's pretty similar to what you have now, but "SetClient" seems like a permanent delegate-style thing that you'll set up once and will be used for a bunch of operations, whereas TrayActionObserver feels more lightweight and if we added different buttons could be different for each. It could even be part of the system_tray.mojom interface. WDYT? (BTW, in the future feel free to send me just mojom files or design docs for a review pass. Apologies for not thinking of this before when I read your doc.) https://codereview.chromium.org/2848813002/diff/300001/ash/public/interfaces/... ash/public/interfaces/tray_action.mojom:42: // Null client is equivalent to kNoteSupported state. kNotSupported https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... File ash/tray_action/tray_action_unittest.cc (right): https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:60: void RequestNewLockScreenNote() override { action_requests_count_++; } nit: "// mojom::TrayActionClient:" above https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:64: void reset_action_requests_count() { action_requests_count_ = 0; } nit: ResetActionRequestsCount(). https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:101: // Effective state hasn't changed, so observer should be notified of it. nit: "should be" -> "should not be", or just remove the comment. https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:102: ASSERT_EQ(0u, observer.observed_states().size()); EXPECT_EQ https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:110: ASSERT_EQ(0u, observer.observed_states().size()); EXPECT_EQ https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:124: observer.ClearObservedStates(); do you have to do this? https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:126: tray_action->SetClient(nullptr, TrayActionState::kNotSupported); do you have to do this? https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:150: observer.ClearObservedStates(); ditto https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:152: tray_action->SetClient(nullptr, TrayActionState::kNotSupported); ditto, and below https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:155: TEST_F(TrayActionTest, StateChangesWithHandlerSet) { This test is kinda duplicated by the NormalStateProgression and the ObserversNotNotifiedOnDuplicateState. Maybe just delete this one? https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:251: EXPECT_EQ(1, action_client.action_requests_count()); Why does it get a request if it set the state as "not supported"? That seems odd. Maybe there shouldn't be a kNotSupported state at all (and you don't set a client if you don't support the action)? Or maybe the ash TrayAction controller shouldn't send requests if the action isn't supported. https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.cc (right): https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:23: StateController* g_instance = nullptr; The way you're handling instance creation and singleton management here seems a little unusual and I'm finding it hard to follow. I recommend copying the pattern xiyuan@ used in SessionControllerClient. Basically: public constructor in constuctor, DCHECK(IsEnabled()), DCHECK(!g_instance) and assign g_instance public destructor DCHECK_EQ(this, g_instance) public Init method Allow tests to reset the tray_action_ptr_ between construction and calling Init(), either by the tests being a friend or by providing a SetFooForTesting() method. public Get() method that does DCHECK(g_instance) Then you don't need special Create/CreateForTesting methods, and you don't need to check IsEnabled() so many places. https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:35: CHECK(g_instance || !IsEnabled()); Do all these really need to be CHECK or would DCHECK be enough? https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:65: g_instance = nullptr; Nice that you reset the pointer at the start of the destructor and not at the end! I've been burned by people doing it the other way around. https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:104: void StateController::RegisterAsAshTrayActionClient() { This could become the Init() method. https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.h (right): https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.h:36: // object is desctructed. nit: "destructed" or "destroyed", here and below https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.h:59: // show an app window for the specified action. probably don't need this comment (it should be documented in the underlying interface) https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc (right): https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:72: void clear_observed_states() { observed_states_.clear(); } ClearObservedStates https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:157: ASSERT_EQ(0u, observer()->observed_states().size()); EXPECT_EQ https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:182: base::RunLoop().RunUntilIdle(); If you want to be explicit about why you have to spin the message loop you can expose a FlushForTesting() method and use that instead. https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:207: EXPECT_EQ(0u, observer()->observed_states().size()); A comment here would be helpful (since a trivial read of the code makes me wonder "why doesn't it get another request?") https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:209: } Thanks for adding good test coverage, both here and in ash. It makes later refactoring much easier.
The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2848813002/diff/300001/ash/public/interfaces/... File ash/public/interfaces/tray_action.mojom (right): https://codereview.chromium.org/2848813002/diff/300001/ash/public/interfaces/... ash/public/interfaces/tray_action.mojom:28: // surfeace user sign-in UI). On 2017/05/05 02:55:44, James Cook wrote: > nit: surface > > Or rewrite as: "(e.g. the handler window for the lock screen action was moved to > the background and appears underneath the login user pods)" Done. https://codereview.chromium.org/2848813002/diff/300001/ash/public/interfaces/... ash/public/interfaces/tray_action.mojom:33: // state changes. On 2017/05/05 02:55:44, James Cook wrote: > nit: I would also say what a tray action is. For example, "Allows a client (e.g. > Chrome) to create a "tray action" button that appears in the status area and > invokes a client/callback method when clicked." Done. https://codereview.chromium.org/2848813002/diff/300001/ash/public/interfaces/... ash/public/interfaces/tray_action.mojom:34: interface TrayAction { On 2017/05/05 02:55:44, James Cook wrote: > optional: This API is OK for now, but in the long run I wonder if it would be > better to make the API look like: > > AddTrayAction(TrayActionState initial_state, TrayActionObserver observer) > or > AddTrayAction(ImageSkia icon, TrayActionObserver observer) > or > AddLockScreenTrayAction(TrayActionObserver observer) > > the initial hard-coded version could be something like: > > AddLockScreenTrayAction(...) > > and > > interface TrayActionObserver { > OnClicked(...modifier keys...); > } > > or > > interface TrayActionHandler { > OnEvent(...event...); > } > > Functionally it's pretty similar to what you have now, but "SetClient" seems > like a permanent delegate-style thing that you'll set up once and will be used > for a bunch of operations, whereas TrayActionObserver feels more lightweight and > if we added different buttons could be different for each. > > It could even be part of the system_tray.mojom interface. > > WDYT? > > (BTW, in the future feel free to send me just mojom files or design docs for a > review pass. Apologies for not thinking of this before when I read your doc.) Yes, I think this would be a good approach when we have to generalize this, though I'd wait with going this way until we introduce another action type. https://codereview.chromium.org/2848813002/diff/300001/ash/public/interfaces/... ash/public/interfaces/tray_action.mojom:42: // Null client is equivalent to kNoteSupported state. On 2017/05/05 02:55:44, James Cook wrote: > kNotSupported Done. https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... File ash/tray_action/tray_action_unittest.cc (right): https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:60: void RequestNewLockScreenNote() override { action_requests_count_++; } On 2017/05/05 02:55:44, James Cook wrote: > nit: "// mojom::TrayActionClient:" above Done. https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:64: void reset_action_requests_count() { action_requests_count_ = 0; } On 2017/05/05 02:55:45, James Cook wrote: > nit: ResetActionRequestsCount(). Done. https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:101: // Effective state hasn't changed, so observer should be notified of it. On 2017/05/05 02:55:45, James Cook wrote: > nit: "should be" -> "should not be", or just remove the comment. Done. https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:102: ASSERT_EQ(0u, observer.observed_states().size()); On 2017/05/05 02:55:44, James Cook wrote: > EXPECT_EQ Done. https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:110: ASSERT_EQ(0u, observer.observed_states().size()); On 2017/05/05 02:55:45, James Cook wrote: > EXPECT_EQ Done. https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:124: observer.ClearObservedStates(); On 2017/05/05 02:55:45, James Cook wrote: > do you have to do this? Done. https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:126: tray_action->SetClient(nullptr, TrayActionState::kNotSupported); On 2017/05/05 02:55:45, James Cook wrote: > do you have to do this? Done. https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:150: observer.ClearObservedStates(); On 2017/05/05 02:55:45, James Cook wrote: > ditto Done. https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:152: tray_action->SetClient(nullptr, TrayActionState::kNotSupported); On 2017/05/05 02:55:44, James Cook wrote: > ditto, and below Done. https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:155: TEST_F(TrayActionTest, StateChangesWithHandlerSet) { On 2017/05/05 02:55:44, James Cook wrote: > This test is kinda duplicated by the NormalStateProgression and the > ObserversNotNotifiedOnDuplicateState. Maybe just delete this one? Done. https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:251: EXPECT_EQ(1, action_client.action_requests_count()); On 2017/05/05 02:55:45, James Cook wrote: > Why does it get a request if it set the state as "not supported"? That seems > odd. > > Maybe there shouldn't be a kNotSupported state at all (and you don't set a > client if you don't support the action)? Or maybe the ash TrayAction controller > shouldn't send requests if the action isn't supported. Yeah, updated the logic to check that the action can be handled before sending the request. In practice, client will be unset when the state is kNotSupported (when the feature is implemented, at least). https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.cc (right): https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:23: StateController* g_instance = nullptr; On 2017/05/05 02:55:45, James Cook wrote: > The way you're handling instance creation and singleton management here seems a > little unusual and I'm finding it hard to follow. I recommend copying the > pattern xiyuan@ used in SessionControllerClient. Basically: > > public constructor > in constuctor, DCHECK(IsEnabled()), DCHECK(!g_instance) and assign g_instance > > public destructor > DCHECK_EQ(this, g_instance) > > public Init method > > Allow tests to reset the tray_action_ptr_ between construction and calling > Init(), either by the tests being a friend or by providing a SetFooForTesting() > method. > > public Get() method that does DCHECK(g_instance) > > Then you don't need special Create/CreateForTesting methods, and you don't need > to check IsEnabled() so many places. This pretty much what I'm doing now, but I have the creation/initialization logic here, in Create methods, but I can move it to call sites. https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:35: CHECK(g_instance || !IsEnabled()); On 2017/05/05 02:55:45, James Cook wrote: > Do all these really need to be CHECK or would DCHECK be enough? I guess it could be dcheck https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:65: g_instance = nullptr; On 2017/05/05 02:55:45, James Cook wrote: > Nice that you reset the pointer at the start of the destructor and not at the > end! I've been burned by people doing it the other way around. Acknowledged. https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:104: void StateController::RegisterAsAshTrayActionClient() { On 2017/05/05 02:55:45, James Cook wrote: > This could become the Init() method. Done. https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.h (right): https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.h:36: // object is desctructed. On 2017/05/05 02:55:45, James Cook wrote: > nit: "destructed" or "destroyed", here and below Done. https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.h:59: // show an app window for the specified action. On 2017/05/05 02:55:45, James Cook wrote: > probably don't need this comment (it should be documented in the underlying > interface) Done. https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc (right): https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:72: void clear_observed_states() { observed_states_.clear(); } On 2017/05/05 02:55:45, James Cook wrote: > ClearObservedStates Done. https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:157: ASSERT_EQ(0u, observer()->observed_states().size()); On 2017/05/05 02:55:45, James Cook wrote: > EXPECT_EQ Done. https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:182: base::RunLoop().RunUntilIdle(); On 2017/05/05 02:55:45, James Cook wrote: > If you want to be explicit about why you have to spin the message loop you can > expose a FlushForTesting() method and use that instead. Done. https://codereview.chromium.org/2848813002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:207: EXPECT_EQ(0u, observer()->observed_states().size()); On 2017/05/05 02:55:45, James Cook wrote: > A comment here would be helpful (since a trivial read of the code makes me > wonder "why doesn't it get another request?") Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
LGTM. Thanks for doing this with mojo - it will make my life much easier for mustash. https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... File ash/tray_action/tray_action_unittest.cc (right): https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:251: EXPECT_EQ(1, action_client.action_requests_count()); On 2017/05/05 04:46:26, tbarzic wrote: > On 2017/05/05 02:55:45, James Cook wrote: > > Why does it get a request if it set the state as "not supported"? That seems > > odd. > > > > Maybe there shouldn't be a kNotSupported state at all (and you don't set a > > client if you don't support the action)? Or maybe the ash TrayAction > controller > > shouldn't send requests if the action isn't supported. > > Yeah, updated the logic to check that the action can be handled before sending > the request. > > In practice, client will be unset when the state is kNotSupported (when the > feature is implemented, at least). After more consideration, I think you should eliminate the possibility of having two states ("no client" vs. "yes client but not available"). It seems like that exists to support having multiple different actions, which you don't have. If you had multiple actions, I think it would be better to have a "client" or "observer" per action. Either way, I think it would be better to get rid of kNotSupported. Feel free to do this in a follow-up CL. https://codereview.chromium.org/2848813002/diff/360001/ash/tray_action/tray_a... File ash/tray_action/tray_action.cc (right): https://codereview.chromium.org/2848813002/diff/360001/ash/tray_action/tray_a... ash/tray_action/tray_action.cc:71: if (GetLockScreenNoteState() != mojom::TrayActionState::kAvailable) nit: you still might want to check tray_action_client_ here, since this will crash if you request a note without a client. Or DCHECK() it at the top of the function to be clear that it's not valid to call this without a client. https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:611: lock_screen_apps_state_controller_.reset( nit: base::MakeUnique https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.cc (right): https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:45: DCHECK(g_instance); nit: DCHECK_EQ(this, g_instance) https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.h (right): https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.h:34: // StateController will set global instance ptr that cen be accessed using nit: cen -> can https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc (right): https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:113: state_controller_.reset(new lock_screen_apps::StateController()); MakeUnique
The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run
https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... File ash/tray_action/tray_action_unittest.cc (right): https://codereview.chromium.org/2848813002/diff/300001/ash/tray_action/tray_a... ash/tray_action/tray_action_unittest.cc:251: EXPECT_EQ(1, action_client.action_requests_count()); On 2017/05/05 17:22:46, James Cook (OOO) wrote: > On 2017/05/05 04:46:26, tbarzic wrote: > > On 2017/05/05 02:55:45, James Cook wrote: > > > Why does it get a request if it set the state as "not supported"? That seems > > > odd. > > > > > > Maybe there shouldn't be a kNotSupported state at all (and you don't set a > > > client if you don't support the action)? Or maybe the ash TrayAction > > controller > > > shouldn't send requests if the action isn't supported. > > > > Yeah, updated the logic to check that the action can be handled before sending > > the request. > > > > In practice, client will be unset when the state is kNotSupported (when the > > feature is implemented, at least). > > After more consideration, I think you should eliminate the possibility of having > two states ("no client" vs. "yes client but not available"). It seems like that > exists to support having multiple different actions, which you don't have. If > you had multiple actions, I think it would be better to have a "client" or > "observer" per action. Either way, I think it would be better to get rid of > kNotSupported. Feel free to do this in a follow-up CL. OK, there are subtle differences, but probably not relevant enough at the moment (especially not in ash) - replaced kNotSupported with kNotAvailable. If I Chrome code ends up needing to separate the two, I can always add another state later. https://codereview.chromium.org/2848813002/diff/360001/ash/tray_action/tray_a... File ash/tray_action/tray_action.cc (right): https://codereview.chromium.org/2848813002/diff/360001/ash/tray_action/tray_a... ash/tray_action/tray_action.cc:71: if (GetLockScreenNoteState() != mojom::TrayActionState::kAvailable) On 2017/05/05 17:22:46, James Cook (OOO) wrote: > nit: you still might want to check tray_action_client_ here, since this will > crash if you request a note without a client. Or DCHECK() it at the top of the > function to be clear that it's not valid to call this without a client. GetLockScreenNoteState() being kAvailable implies that tray_action_client_ is set - added a comment and a DCHECK. https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:611: lock_screen_apps_state_controller_.reset( On 2017/05/05 17:22:46, James Cook (OOO) wrote: > nit: base::MakeUnique Done. https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.cc (right): https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:45: DCHECK(g_instance); On 2017/05/05 17:22:47, James Cook (OOO) wrote: > nit: DCHECK_EQ(this, g_instance) Done. https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.h (right): https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.h:34: // StateController will set global instance ptr that cen be accessed using On 2017/05/05 17:22:47, James Cook (OOO) wrote: > nit: cen -> can Done. https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc (right): https://codereview.chromium.org/2848813002/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:113: state_controller_.reset(new lock_screen_apps::StateController()); On 2017/05/05 17:22:47, James Cook (OOO) wrote: > MakeUnique Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Still LGTM.
tbarzic@chromium.org changed reviewers: + tsepez@chromium.org
+tsepez for ash/public/interfaces/tray_action.mojom
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm
The CQ bit was checked by tbarzic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, tsepez@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2848813002/#ps400001 (title: "Fix outdated comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by tbarzic@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 400001, "attempt_start_ts": 1494034265170270,
"parent_rev": "d98981b9ab09719407adcb63f639276590f4263b", "commit_rev":
"033f267aec04cedfa513cef1880e9dc81dc83648"}
Message was sent while issue was closed.
Description was changed from ========== Introduce ash mojo interface for lock screen action handlers This is part of support for note taking apps on lock screen. This is the interface that will enable ash to observe state of apps that can/are allowed to handle actions (currently, only "new note" action is supported) on lock screen and update it's state accordingly. For example, this interface will be used for tray icon for launching new note. The icon will be exposed only if the action handler controller is in the state indicating that there is an app registered for handling new note action on lock screen. Clicking the icon would issue a request to action handler to handle the action (thus adding RequestHandleAction interface). BUG=716081 ========== to ========== Introduce ash mojo interface for lock screen action handlers This is part of support for note taking apps on lock screen. This is the interface that will enable ash to observe state of apps that can/are allowed to handle actions (currently, only "new note" action is supported) on lock screen and update it's state accordingly. For example, this interface will be used for tray icon for launching new note. The icon will be exposed only if the action handler controller is in the state indicating that there is an app registered for handling new note action on lock screen. Clicking the icon would issue a request to action handler to handle the action (thus adding RequestHandleAction interface). BUG=716081 Review-Url: https://codereview.chromium.org/2848813002 Cr-Commit-Position: refs/heads/master@{#469870} Committed: https://chromium.googlesource.com/chromium/src/+/033f267aec04cedfa513cef1880e... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/033f267aec04cedfa513cef1880e...
Message was sent while issue was closed.
A revert of this CL (patchset #21 id:400001) has been created in https://codereview.chromium.org/2870473002/ by findit-for-me@appspot.gserviceaccount.com. The reason for reverting is: Findit (https://goo.gl/kROfz5) identified CL at revision 469870 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....
Message was sent while issue was closed.
Description was changed from ========== Introduce ash mojo interface for lock screen action handlers This is part of support for note taking apps on lock screen. This is the interface that will enable ash to observe state of apps that can/are allowed to handle actions (currently, only "new note" action is supported) on lock screen and update it's state accordingly. For example, this interface will be used for tray icon for launching new note. The icon will be exposed only if the action handler controller is in the state indicating that there is an app registered for handling new note action on lock screen. Clicking the icon would issue a request to action handler to handle the action (thus adding RequestHandleAction interface). BUG=716081 Review-Url: https://codereview.chromium.org/2848813002 Cr-Commit-Position: refs/heads/master@{#469870} Committed: https://chromium.googlesource.com/chromium/src/+/033f267aec04cedfa513cef1880e... ========== to ========== Introduce ash mojo interface for lock screen action handlers This is part of support for note taking apps on lock screen. This is the interface that will enable ash to observe state of apps that can/are allowed to handle actions (currently, only "new note" action is supported) on lock screen and update it's state accordingly. For example, this interface will be used for tray icon for launching new note. The icon will be exposed only if the action handler controller is in the state indicating that there is an app registered for handling new note action on lock screen. Clicking the icon would issue a request to action handler to handle the action (thus adding RequestHandleAction interface). BUG=716081 Review-Url: https://codereview.chromium.org/2848813002 Cr-Commit-Position: refs/heads/master@{#469870} Committed: https://chromium.googlesource.com/chromium/src/+/033f267aec04cedfa513cef1880e... ==========
I added ASH_EXPORTs in patchset 22 - hopefully that should fix link errors in ash_unittests can you please take another look
LGTM
The CQ bit was checked by tbarzic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2848813002/#ps420001 (title: "add ASH_EXPORTS")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1494270076611860,
"parent_rev": "d4356478ea0f780d9aaebefad24439437bc7b13e", "commit_rev":
"c78da1ff9f01a79c89aa715e2dd42ff0dfde0cec"}
Message was sent while issue was closed.
Description was changed from ========== Introduce ash mojo interface for lock screen action handlers This is part of support for note taking apps on lock screen. This is the interface that will enable ash to observe state of apps that can/are allowed to handle actions (currently, only "new note" action is supported) on lock screen and update it's state accordingly. For example, this interface will be used for tray icon for launching new note. The icon will be exposed only if the action handler controller is in the state indicating that there is an app registered for handling new note action on lock screen. Clicking the icon would issue a request to action handler to handle the action (thus adding RequestHandleAction interface). BUG=716081 Review-Url: https://codereview.chromium.org/2848813002 Cr-Commit-Position: refs/heads/master@{#469870} Committed: https://chromium.googlesource.com/chromium/src/+/033f267aec04cedfa513cef1880e... ========== to ========== Introduce ash mojo interface for lock screen action handlers This is part of support for note taking apps on lock screen. This is the interface that will enable ash to observe state of apps that can/are allowed to handle actions (currently, only "new note" action is supported) on lock screen and update it's state accordingly. For example, this interface will be used for tray icon for launching new note. The icon will be exposed only if the action handler controller is in the state indicating that there is an app registered for handling new note action on lock screen. Clicking the icon would issue a request to action handler to handle the action (thus adding RequestHandleAction interface). BUG=716081 Review-Url: https://codereview.chromium.org/2848813002 Cr-Original-Commit-Position: refs/heads/master@{#469870} Committed: https://chromium.googlesource.com/chromium/src/+/033f267aec04cedfa513cef1880e... Review-Url: https://codereview.chromium.org/2848813002 Cr-Commit-Position: refs/heads/master@{#470115} Committed: https://chromium.googlesource.com/chromium/src/+/c78da1ff9f01a79c89aa715e2dd4... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001) as https://chromium.googlesource.com/chromium/src/+/c78da1ff9f01a79c89aa715e2dd4... |
