Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
304603002:
Created paring flow interface for controller side. (Closed)
|
Created:
6 years, 7 months ago by dzhioev (left Google) Modified:
6 years, 6 months ago CC:
stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCreated paring flow interface for controller side.
This interface will be used by controller's UI to pass through pairing.
BUG=375191
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274545
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removed |previous_stage| param. #Patch Set 3 : Added methods and notifications related to devices list. #
Total comments: 1
Patch Set 4 : Moved to chromeos/. #
Total comments: 8
Patch Set 5 : Comments addressed. #
Total comments: 1
Patch Set 6 : Added more OWNERS for 'pairing' dir. #
Messages
Total messages: 21 (0 generated)
Hi, I'm going to start pairing UI implementation this week, and I need something to start with. That's why I designed this pairing flow interface for controller. It is based on latest controller's flow mocks. This interface will be used by controller UI. I suspect Zackhary is the one, who will be implementing this interface for real back-end. I'm going to implement fake, for testing purpose. So please take a look.
https://codereview.chromium.org/304603002/diff/1/ui/chromeos/pairing/controll... File ui/chromeos/pairing/controller_pairing_flow.h (right): https://codereview.chromium.org/304603002/diff/1/ui/chromeos/pairing/controll... ui/chromeos/pairing/controller_pairing_flow.h:47: virtual void PairingStageChanged(Stage previous_stage, Stage new_stage) = 0; Why do you expect the previous stage to be sent here? Shouldn't the observer already know that?
https://codereview.chromium.org/304603002/diff/1/ui/chromeos/pairing/controll... File ui/chromeos/pairing/controller_pairing_flow.h (right): https://codereview.chromium.org/304603002/diff/1/ui/chromeos/pairing/controll... ui/chromeos/pairing/controller_pairing_flow.h:47: virtual void PairingStageChanged(Stage previous_stage, Stage new_stage) = 0; On 2014/05/27 23:46:43, Zachary Kuznia wrote: > Why do you expect the previous stage to be sent here? Shouldn't the observer > already know that? Yes, probably |previous_stage| is excess.
lgtm
On 2014/05/28 15:45:07, Zachary Kuznia wrote: > lgtm Sorry I forgot create methods and notifications for retrieving a list of available devices. Also I found out that Bluetooth discovery process is not clear from UI perspective. I'm sending email with questions to xiaowenx@, you are added to CC.
On 2014/05/28 19:52:05, dzhioev wrote: > On 2014/05/28 15:45:07, Zachary Kuznia wrote: > > lgtm > Sorry I forgot create methods and notifications for retrieving a list of > available devices. Also I found out that Bluetooth discovery process is not > clear from UI perspective. I'm sending email with questions to xiaowenx@, you > are added to CC. PTAL. I've added methods for retrieving devices list. Also I've merged STAGE_SCANNING_FOR_DEVICES and STAGE_WAITING_FOR_DEVICE_SELECTION into STAGE_DEVICES_DISCOVERY. UI will be responsible for switching between first and second pages during this stage.
On 2014/05/28 19:52:05, dzhioev wrote: > On 2014/05/28 15:45:07, Zachary Kuznia wrote: > > lgtm > Sorry I forgot create methods and notifications for retrieving a list of > available devices. Also I found out that Bluetooth discovery process is not > clear from UI perspective. I'm sending email with questions to xiaowenx@, you > are added to CC. PTAL. I've added methods for retrieving devices list. Also I've merged STAGE_SCANNING_FOR_DEVICES and STAGE_WAITING_FOR_DEVICE_SELECTION into STAGE_DEVICES_DISCOVERY. UI will be responsible for switching between first and second pages during this stage.
On 2014/05/29 15:14:59, dzhioev wrote: > On 2014/05/28 19:52:05, dzhioev wrote: > > On 2014/05/28 15:45:07, Zachary Kuznia wrote: > > > lgtm > > Sorry I forgot create methods and notifications for retrieving a list of > > available devices. Also I found out that Bluetooth discovery process is not > > clear from UI perspective. I'm sending email with questions to xiaowenx@, you > > are added to CC. > > PTAL. I've added methods for retrieving devices list. > Also I've merged STAGE_SCANNING_FOR_DEVICES and > STAGE_WAITING_FOR_DEVICE_SELECTION into STAGE_DEVICES_DISCOVERY. UI will be > responsible for switching between first and second pages during this stage. Is "ui/chromeos" a right place for these files?
It's hard to judge the necessity of this as an interface without an initial implementation. Personally I would recommend waiting for an implementation before committing this. How is this intended to be implemented? If it's WebUI, then this seems like a reasonable location, although I wonder if it is necessary; it seems like this flow could be handled by the JS UI code. If it's going to be part of the status area (aka system tray) it should probably go in ash/system/chromeos. https://codereview.chromium.org/304603002/diff/40001/ui/chromeos/pairing/cont... File ui/chromeos/pairing/controller_pairing_flow.h (right): https://codereview.chromium.org/304603002/diff/40001/ui/chromeos/pairing/cont... ui/chromeos/pairing/controller_pairing_flow.h:101: virtual void StartSession() = 0; WS
On 2014/05/29 16:06:55, stevenjb wrote: > It's hard to judge the necessity of this as an interface without an initial > implementation. Personally I would recommend waiting for an implementation > before committing this. > > How is this intended to be implemented? If it's WebUI, then this seems like a > reasonable location, although I wonder if it is necessary; it seems like this > flow could be handled by the JS UI code. > > If it's going to be part of the status area (aka system tray) it should probably > go in ash/system/chromeos. > > https://codereview.chromium.org/304603002/diff/40001/ui/chromeos/pairing/cont... > File ui/chromeos/pairing/controller_pairing_flow.h (right): > > https://codereview.chromium.org/304603002/diff/40001/ui/chromeos/pairing/cont... > ui/chromeos/pairing/controller_pairing_flow.h:101: virtual void StartSession() = > 0; > WS We have the following plan: I work on UI implementation (WebUI), while zork@ works on back-end (BT things, messages exchange, enrollment, and so on) implementation at parallel. Back-end is implementing this interface, UI acts like Observer. I'm going to use fake implementation of the interface until the real one is finished. Pairing flow is a complicated process (see design doc in issue), so back-end will not be ready in the near feature. So fixing this interface will unblock both me and zork@. This flow is a part of OOBE, so UI will be implemented as WebUI. Since OOBE/Login code lives in chrome/browser, my implementation will most likely live there too. The work on extracting OOBE/Login code from 'chrome/browser' is started already, so hopefully my code will be moved out from 'chrome/browser' eventually too. I can't say much about back-end, but probably some part of code will live in 'chrome/browser' too, because it will use code from 'chrome/browser/chromeos/login' and 'chrome/browser/chromeos/login/enrollment'.
On 2014/05/29 17:28:43, dzhioev wrote: > On 2014/05/29 16:06:55, stevenjb wrote: > > It's hard to judge the necessity of this as an interface without an initial > > implementation. Personally I would recommend waiting for an implementation > > before committing this. > > > > How is this intended to be implemented? If it's WebUI, then this seems like a > > reasonable location, although I wonder if it is necessary; it seems like this > > flow could be handled by the JS UI code. > > > > If it's going to be part of the status area (aka system tray) it should > probably > > go in ash/system/chromeos. > > > > > https://codereview.chromium.org/304603002/diff/40001/ui/chromeos/pairing/cont... > > File ui/chromeos/pairing/controller_pairing_flow.h (right): > > > > > https://codereview.chromium.org/304603002/diff/40001/ui/chromeos/pairing/cont... > > ui/chromeos/pairing/controller_pairing_flow.h:101: virtual void StartSession() > = > > 0; > > WS > > We have the following plan: I work on UI implementation (WebUI), while zork@ > works on back-end (BT things, messages exchange, enrollment, and so on) > implementation at parallel. Back-end is implementing this interface, UI acts > like Observer. I'm going to use fake implementation of the interface until the > real one is finished. Pairing flow is a complicated process (see design doc in > issue), so back-end will not be ready in the near feature. So fixing this > interface will unblock both me and zork@. > > This flow is a part of OOBE, so UI will be implemented as WebUI. Since > OOBE/Login code lives in chrome/browser, my implementation will most likely live > there too. The work on extracting OOBE/Login code from 'chrome/browser' is > started already, so hopefully my code will be moved out from 'chrome/browser' > eventually too. > > I can't say much about back-end, but probably some part of code will live in > 'chrome/browser' too, because it will use code from > 'chrome/browser/chromeos/login' and 'chrome/browser/chromeos/login/enrollment'. I entirely support moving / keeping code out of src/chrome. Looking at this again, this looks more like controller code than UI code, and probably belongs in src/chromeos/pairing. The UI (View) code will implement the Observer and provide an API to the WebView/JS code.
On 2014/05/29 22:12:42, stevenjb wrote: > On 2014/05/29 17:28:43, dzhioev wrote: > > On 2014/05/29 16:06:55, stevenjb wrote: > > > It's hard to judge the necessity of this as an interface without an initial > > > implementation. Personally I would recommend waiting for an implementation > > > before committing this. > > > > > > How is this intended to be implemented? If it's WebUI, then this seems like > a > > > reasonable location, although I wonder if it is necessary; it seems like > this > > > flow could be handled by the JS UI code. > > > > > > If it's going to be part of the status area (aka system tray) it should > > probably > > > go in ash/system/chromeos. > > > > > > > > > https://codereview.chromium.org/304603002/diff/40001/ui/chromeos/pairing/cont... > > > File ui/chromeos/pairing/controller_pairing_flow.h (right): > > > > > > > > > https://codereview.chromium.org/304603002/diff/40001/ui/chromeos/pairing/cont... > > > ui/chromeos/pairing/controller_pairing_flow.h:101: virtual void > StartSession() > > = > > > 0; > > > WS > > > > We have the following plan: I work on UI implementation (WebUI), while zork@ > > works on back-end (BT things, messages exchange, enrollment, and so on) > > implementation at parallel. Back-end is implementing this interface, UI acts > > like Observer. I'm going to use fake implementation of the interface until the > > real one is finished. Pairing flow is a complicated process (see design doc in > > issue), so back-end will not be ready in the near feature. So fixing this > > interface will unblock both me and zork@. > > > > This flow is a part of OOBE, so UI will be implemented as WebUI. Since > > OOBE/Login code lives in chrome/browser, my implementation will most likely > live > > there too. The work on extracting OOBE/Login code from 'chrome/browser' is > > started already, so hopefully my code will be moved out from 'chrome/browser' > > eventually too. > > > > I can't say much about back-end, but probably some part of code will live in > > 'chrome/browser' too, because it will use code from > > 'chrome/browser/chromeos/login' and > 'chrome/browser/chromeos/login/enrollment'. > > I entirely support moving / keeping code out of src/chrome. > > Looking at this again, this looks more like controller code than UI code, and > probably belongs in src/chromeos/pairing. The UI (View) code will implement the > Observer and provide an API to the WebView/JS code. I agree. Moved to chromeos/pairing. Also STAGE_ESTABLISHING_CONNECTION_ERROR was added.
https://codereview.chromium.org/304603002/diff/60001/chromeos/pairing/control... File chromeos/pairing/controller_pairing_flow.h (right): https://codereview.chromium.org/304603002/diff/60001/chromeos/pairing/control... chromeos/pairing/controller_pairing_flow.h:59: typedef std::string DeviceID; I think that typdefing to a string adds more potential confusion than it removes. It would however be reasonable to typedef std::vector<std::string> to something like DeviceIdList. https://codereview.chromium.org/304603002/diff/60001/chromeos/pairing/control... chromeos/pairing/controller_pairing_flow.h:61: public: Remove duplicate 'public' https://codereview.chromium.org/304603002/diff/60001/chromeos/pairing/control... chromeos/pairing/controller_pairing_flow.h:92: virtual void SetConfirmationCodeIsRight(bool right) = 0; s/Right/Correct/ https://codereview.chromium.org/304603002/diff/60001/chromeos/pairing/control... chromeos/pairing/controller_pairing_flow.h:102: virtual void StartSession() = 0; WS
https://codereview.chromium.org/304603002/diff/60001/chromeos/pairing/control... File chromeos/pairing/controller_pairing_flow.h (right): https://codereview.chromium.org/304603002/diff/60001/chromeos/pairing/control... chromeos/pairing/controller_pairing_flow.h:59: typedef std::string DeviceID; On 2014/06/02 16:35:57, stevenjb wrote: > I think that typdefing to a string adds more potential confusion than it > removes. It would however be reasonable to typedef std::vector<std::string> to > something like DeviceIdList. Done. https://codereview.chromium.org/304603002/diff/60001/chromeos/pairing/control... chromeos/pairing/controller_pairing_flow.h:61: public: On 2014/06/02 16:35:57, stevenjb wrote: > Remove duplicate 'public' Done. https://codereview.chromium.org/304603002/diff/60001/chromeos/pairing/control... chromeos/pairing/controller_pairing_flow.h:92: virtual void SetConfirmationCodeIsRight(bool right) = 0; On 2014/06/02 16:35:57, stevenjb wrote: > s/Right/Correct/ > Done. https://codereview.chromium.org/304603002/diff/60001/chromeos/pairing/control... chromeos/pairing/controller_pairing_flow.h:102: virtual void StartSession() = 0; On 2014/06/02 16:35:57, stevenjb wrote: > WS Done.
lgtm
lgtm https://codereview.chromium.org/304603002/diff/80001/chromeos/pairing/OWNERS File chromeos/pairing/OWNERS (right): https://codereview.chromium.org/304603002/diff/80001/chromeos/pairing/OWNERS#... chromeos/pairing/OWNERS:1: dzhioev@chromium.org Please also add zork@ and achuith@
On 2014/06/02 20:54:28, Dmitry Polukhin wrote: > lgtm > > https://codereview.chromium.org/304603002/diff/80001/chromeos/pairing/OWNERS > File chromeos/pairing/OWNERS (right): > > https://codereview.chromium.org/304603002/diff/80001/chromeos/pairing/OWNERS#... > chromeos/pairing/OWNERS:1: mailto:dzhioev@chromium.org > Please also add zork@ and achuith@ Done.
The CQ bit was checked by dzhioev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dzhioev@chromium.org/304603002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
Message was sent while issue was closed.
Change committed as 274545 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
