ash/public/interfaces/touch_view.mojom - Issue 2505403003: TouchView Mojom

Side by Side Diff: ash/public/interfaces/touch_view.mojom

Issue 2505403003: TouchView Mojom (Closed)

Patch Set: Dont export for Windows Created 4 years ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« no previous file with comments | « ash/public/interfaces/BUILD.gn ('k') | chrome/browser/chrome_content_browser_manifest_overlay.json » ('j') | chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 // Copyright 2016 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4

	5 module ash.mojom;

	6

	7 // Used to listen for when TouchView is toggled.

	8 interface TouchViewObserver {

	9 OnTouchViewChanged(bool enabled);
	James Cook 2016/12/01 16:55:09 nit: document when this is fired (e.g. before or a nit: document when this is fired (e.g. before or after). I presume it's after, but since mojom is our permanent interface between ash and chrome I prefer everything to be obsessively documented. optional: This also could be called OnTouchViewEnabled(bool enabled) or OnTouchViewToggled(bool enabled) which would make calls like OnTouchViewChanged(false) self-documenting. Or you could keep separate Started/Ended methods. jonross 2016/12/01 19:20:57 Added docs on timing. Going with the suggested OnT Show quoted text On 2016/12/01 16:55:09, James Cook wrote: > nit: document when this is fired (e.g. before or after). I presume it's after, > but since mojom is our permanent interface between ash and chrome I prefer > everything to be obsessively documented. > > optional: This also could be called OnTouchViewEnabled(bool enabled) or > OnTouchViewToggled(bool enabled) which would make calls like > OnTouchViewChanged(false) self-documenting. Or you could keep separate > Started/Ended methods. Added docs on timing. Going with the suggested OnTouchViewToggled(bool enabled) I like methods to be as self-documenting as possible.
	10 };

	11

	12 // Accepts observers which are notified of the current state of TouchView, and

	13 // of all future state changes.

	14 interface TouchViewManager {
	James Cook 2016/12/01 16:55:09 Just curious, any particular reason for Manager vs Just curious, any particular reason for Manager vs. Controller vs. just TouchView? I'm OK with any of them, and I like that you're calling everything TouchView, it just seemed a little weird that mojom::TouchViewManager is implemented by MaximizeModeController. jonross 2016/12/01 19:20:56 I went with Manager as this interface provides no Show quoted text On 2016/12/01 16:55:09, James Cook wrote: > Just curious, any particular reason for Manager vs. Controller vs. just > TouchView? I'm OK with any of them, and I like that you're calling everything > TouchView, it just seemed a little weird that mojom::TouchViewManager is > implemented by MaximizeModeController. I went with Manager as this interface provides no way to make changes. To me there was nothing to request from a "Controller". I'm open to suggestions on this name though, so I could switch it to just TouchView if desired. Yeah I realized it's weird with MaximizeModeController, but really that should be renamed at some point to TouchView* as its now does more than simply maximize windows.
	15 AddObserver(TouchViewObserver client);
	James Cook 2016/12/01 16:55:09 Just curious, there's no need to remove? Also, yo Just curious, there's no need to remove? Also, you might move (some of or all of) the interface comment to this method, just to emphasize that calling the method triggers an initial notification. jonross 2016/12/01 19:20:56 No need to remove, the mojo::InterfacePtrSet clear Show quoted text On 2016/12/01 16:55:09, James Cook wrote: > Just curious, there's no need to remove? > > Also, you might move (some of or all of) the interface comment to this method, > just to emphasize that calling the method triggers an initial notification. No need to remove, the mojo::InterfacePtrSet clears our ptrs as connections close. So when the signin screen is destructed the ash side clears out the observer.
	16 };

OLD	NEW