Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1128173003:
Implements onBoundsChanged event in virtualKeyboardPrivate namespace (Closed)
|
Created:
5 years, 7 months ago by bshe Modified:
5 years, 7 months ago CC:
chromium-reviews, kalyank, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplements onBoundsChanged event in virtualKeyboardPrivate namespace
BUG=484699
Committed: https://crrev.com/2b52cba3b5a831c466311faba812d51ddd532ba5
Cr-Commit-Position: refs/heads/master@{#330169}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : review #Patch Set 4 : review #
Total comments: 2
Patch Set 5 : Dont set KC in KCP #
Total comments: 6
Patch Set 6 : nits #
Total comments: 6
Patch Set 7 : destroy KCP before destroying observer_list_ #Patch Set 8 : use SetController(nullptr) #
Total comments: 4
Patch Set 9 : review #
Messages
Total messages: 33 (6 generated)
bshe@chromium.org changed reviewers: + sadrul@chromium.org
Hey Sadrul. Do you mind to take a look at this CL? I dont particularly like that I have to create another NotifyKeyboardBoundsChanged function but it seems the easiest to do. Another option is to create a VirtualKeyboardObserver in chrome/ directory and dispatch the onBoundsChanged event from the observer. But I dont know where is the best place to put the observer. Perhaps chrome/browser/ui/virtual_keyboard? What do you think is a better solution? Thanks!
On 2015/05/06 21:27:38, bshe wrote: > Hey Sadrul. > > Do you mind to take a look at this CL? > I dont particularly like that I have to create another > NotifyKeyboardBoundsChanged function but it seems the easiest to do. Another > option is to create a VirtualKeyboardObserver in chrome/ directory and dispatch > the onBoundsChanged event from the observer. But I dont know where is the best > place to put the observer. Perhaps chrome/browser/ui/virtual_keyboard? What do > you think is a better solution? Thanks! Can the AshKeyboardControllerProxy own and install the KeyboardControllerObserver?
On 2015/05/08 01:03:50, sadrul wrote: > On 2015/05/06 21:27:38, bshe wrote: > > Hey Sadrul. > > > > Do you mind to take a look at this CL? > > I dont particularly like that I have to create another > > NotifyKeyboardBoundsChanged function but it seems the easiest to do. Another > > option is to create a VirtualKeyboardObserver in chrome/ directory and > dispatch > > the onBoundsChanged event from the observer. But I dont know where is the best > > place to put the observer. Perhaps chrome/browser/ui/virtual_keyboard? What do > > you think is a better solution? Thanks! > > Can the AshKeyboardControllerProxy own and install the > KeyboardControllerObserver? If I understand correctly, that would require AshKeyboardControllerProxy to access KeyboardController instance in order to add the observer, right? As KC owns KCP, I am a little bit hesitate to access KC in KCP. I have made KCP inherits KeyboardControllerObserver and it seems working. WDYT?
On 2015/05/08 17:24:23, bshe wrote: > On 2015/05/08 01:03:50, sadrul wrote: > > On 2015/05/06 21:27:38, bshe wrote: > > > Hey Sadrul. > > > > > > Do you mind to take a look at this CL? > > > I dont particularly like that I have to create another > > > NotifyKeyboardBoundsChanged function but it seems the easiest to do. Another > > > option is to create a VirtualKeyboardObserver in chrome/ directory and > > dispatch > > > the onBoundsChanged event from the observer. But I dont know where is the > best > > > place to put the observer. Perhaps chrome/browser/ui/virtual_keyboard? What > do > > > you think is a better solution? Thanks! > > > > Can the AshKeyboardControllerProxy own and install the > > KeyboardControllerObserver? > If I understand correctly, that would require AshKeyboardControllerProxy to > access > KeyboardController instance in order to add the observer, right? As KC owns KCP, > I am > a little bit hesitate to access KC in KCP. > I have made KCP inherits KeyboardControllerObserver and it seems working. WDYT? friendly ping?
Sorry, I thought I already responded to this. I don't think it's a good idea for KCP to be a KCO. It would be better to introduce something like KCP::SetController(), or KCP::InitializeForController() (or something like that) which gets called by the KC upon construction.
Patchset #4 (id:60001) has been deleted
On 2015/05/12 19:40:50, sadrul wrote: > Sorry, I thought I already responded to this. > > I don't think it's a good idea for KCP to be a KCO. It would be better to > introduce something like KCP::SetController(), or KCP::InitializeForController() > (or something like that) which gets called by the KC upon construction. Done. PTAL, Thanks!
https://codereview.chromium.org/1128173003/diff/80001/ui/keyboard/keyboard_co... File ui/keyboard/keyboard_controller_proxy.h (right): https://codereview.chromium.org/1128173003/diff/80001/ui/keyboard/keyboard_co... ui/keyboard/keyboard_controller_proxy.h:138: There's no reason for the default impl here to store this, right? The ash impl can do the work instead.
Thanks for review! PTAL https://codereview.chromium.org/1128173003/diff/80001/ui/keyboard/keyboard_co... File ui/keyboard/keyboard_controller_proxy.h (right): https://codereview.chromium.org/1128173003/diff/80001/ui/keyboard/keyboard_co... ui/keyboard/keyboard_controller_proxy.h:138: On 2015/05/13 16:47:30, sadrul wrote: > There's no reason for the default impl here to store this, right? The ash impl > can do the work instead. Correct. I have changed the code according to your suggestion. I didn't make SetController pure virtual to avoid implement them in two other places. But if you think it is better to make it pure virtual, I will do the change.
Some nits. lgtm https://codereview.chromium.org/1128173003/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/ash_keyboard_controller_proxy.h (right): https://codereview.chromium.org/1128173003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.h:22: namespace keyboard { Should go after gfx? https://codereview.chromium.org/1128173003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.h:78: scoped_ptr<AshKeyboardControllerObserver> observer_; I think you can keep this as just scoped_ptr<KeyboardControllerObserver>, and not need to forward declare AshKeyboardControllerObserver. https://codereview.chromium.org/1128173003/diff/100001/ui/keyboard/keyboard_c... File ui/keyboard/keyboard_controller_proxy.h (right): https://codereview.chromium.org/1128173003/diff/100001/ui/keyboard/keyboard_c... ui/keyboard/keyboard_controller_proxy.h:106: virtual void SetController(KeyboardController* controller) {}; No ; at the end. Add a comment clarifying that the KC owns the KCP, and so here the KCP does not get to own the KC.
bshe@chromium.org changed reviewers: + oshima@chromium.org, rockot@chromium.org
Thanks for review! +OWNERs +rockot for extensions/common/api/virtual_keyboard_private.json +oshima for chrome/browser/ui/ash/* https://codereview.chromium.org/1128173003/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/ash_keyboard_controller_proxy.h (right): https://codereview.chromium.org/1128173003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.h:22: namespace keyboard { On 2015/05/14 07:58:42, sadrul wrote: > Should go after gfx? oops. Done https://codereview.chromium.org/1128173003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.h:78: scoped_ptr<AshKeyboardControllerObserver> observer_; On 2015/05/14 07:58:42, sadrul wrote: > I think you can keep this as just scoped_ptr<KeyboardControllerObserver>, and > not need to forward declare AshKeyboardControllerObserver. Done. https://codereview.chromium.org/1128173003/diff/100001/ui/keyboard/keyboard_c... File ui/keyboard/keyboard_controller_proxy.h (right): https://codereview.chromium.org/1128173003/diff/100001/ui/keyboard/keyboard_c... ui/keyboard/keyboard_controller_proxy.h:106: virtual void SetController(KeyboardController* controller) {}; On 2015/05/14 07:58:42, sadrul wrote: > No ; at the end. > > Add a comment clarifying that the KC owns the KCP, and so here the KCP does not > get to own the KC. Done.
schema lgtm
https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right): https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:107: content::BrowserContext* context_; nit: new line before DISALLOW... https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:119: keyboard_controller_->RemoveObserver(observer_.get()); looks like the proxy is owned by controller, so this will be called during Keyboard controller's dtor, which means that this depend on the order of proxy_ and observer_list_ in order to work correctly. Does this have to be in proxy? Or can you put this somewhere that has cleaner dependency?
https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right): https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:119: keyboard_controller_->RemoveObserver(observer_.get()); On 2015/05/14 15:37:22, oshima wrote: > looks like the proxy is owned by controller, so this will be called during > Keyboard controller's dtor, which means that this depend on the order of proxy_ > and observer_list_ in order to work correctly. > > Does this have to be in proxy? Or can you put this somewhere that has cleaner > dependency? What do you think of letting ChromeShellDelegate owns the observer or save the instance to anonymous namespace and manage the lifetime explicitly? I can probably add/remove the observer in https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui... depends on the value of |activated|
https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right): https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:119: keyboard_controller_->RemoveObserver(observer_.get()); On 2015/05/15 15:32:51, bshe wrote: > On 2015/05/14 15:37:22, oshima wrote: > > looks like the proxy is owned by controller, so this will be called during > > Keyboard controller's dtor, which means that this depend on the order of > proxy_ > > and observer_list_ in order to work correctly. > > > > Does this have to be in proxy? Or can you put this somewhere that has cleaner > > dependency? > > What do you think of letting ChromeShellDelegate owns the observer or save the > instance to anonymous namespace and manage the lifetime explicitly? > I can probably add/remove the observer in > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui... > depends on the value of |activated| How about calling proxy->SetController(nullptr) from KC dtor?
On 2015/05/15 15:32:51, bshe wrote: > https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right): > > https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:119: > keyboard_controller_->RemoveObserver(observer_.get()); > On 2015/05/14 15:37:22, oshima wrote: > > looks like the proxy is owned by controller, so this will be called during > > Keyboard controller's dtor, which means that this depend on the order of > proxy_ > > and observer_list_ in order to work correctly. > > > > Does this have to be in proxy? Or can you put this somewhere that has cleaner > > dependency? > > What do you think of letting ChromeShellDelegate owns the observer or save the > instance to anonymous namespace and manage the lifetime explicitly? > I can probably add/remove the observer in > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui... > depends on the value of |activated| huh. hitting send to soon. I didn't do the actual code yet. Just want to run the idea by you before go ahead and do the change. Let me know if you have any concern about that approach or do you have better suggestion. The main limitation is that we only have two places in chrome/ to add/remove the KeyboardControllerObserver. One is during construction/desctruction of the AKCP and another is probably in the VirtualKeyboardActivated function.
On 2015/05/15 15:37:00, sadrul wrote: > https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right): > > https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:119: > keyboard_controller_->RemoveObserver(observer_.get()); > On 2015/05/15 15:32:51, bshe wrote: > > On 2015/05/14 15:37:22, oshima wrote: > > > looks like the proxy is owned by controller, so this will be called during > > > Keyboard controller's dtor, which means that this depend on the order of > > proxy_ > > > and observer_list_ in order to work correctly. > > > > > > Does this have to be in proxy? Or can you put this somewhere that has > cleaner > > > dependency? > > > > What do you think of letting ChromeShellDelegate owns the observer or save the > > instance to anonymous namespace and manage the lifetime explicitly? > > I can probably add/remove the observer in > > > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui... > > depends on the value of |activated| > > How about calling proxy->SetController(nullptr) from KC dtor? huh. Good suggestion. I can do that.
On 2015/05/15 15:39:23, bshe wrote: > On 2015/05/15 15:37:00, sadrul wrote: > > > https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... > > File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right): > > > > > https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... > > chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:119: > > keyboard_controller_->RemoveObserver(observer_.get()); > > On 2015/05/15 15:32:51, bshe wrote: > > > On 2015/05/14 15:37:22, oshima wrote: > > > > looks like the proxy is owned by controller, so this will be called during > > > > Keyboard controller's dtor, which means that this depend on the order of > > > proxy_ > > > > and observer_list_ in order to work correctly. > > > > > > > > Does this have to be in proxy? Or can you put this somewhere that has > > cleaner > > > > dependency? > > > > > > What do you think of letting ChromeShellDelegate owns the observer or save > the > > > instance to anonymous namespace and manage the lifetime explicitly? > > > I can probably add/remove the observer in > > > > > > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui... > > > depends on the value of |activated| > > > > How about calling proxy->SetController(nullptr) from KC dtor? > > huh. Good suggestion. I can do that. I'm fine with that too.
Patchset #7 (id:140001) has been deleted
PTAL, Thanks! https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right): https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:107: content::BrowserContext* context_; On 2015/05/14 15:37:22, oshima wrote: > nit: new line before DISALLOW... Done. https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:119: keyboard_controller_->RemoveObserver(observer_.get()); On 2015/05/15 15:37:00, sadrul wrote: > On 2015/05/15 15:32:51, bshe wrote: > > On 2015/05/14 15:37:22, oshima wrote: > > > looks like the proxy is owned by controller, so this will be called during > > > Keyboard controller's dtor, which means that this depend on the order of > > proxy_ > > > and observer_list_ in order to work correctly. > > > > > > Does this have to be in proxy? Or can you put this somewhere that has > cleaner > > > dependency? > > > > What do you think of letting ChromeShellDelegate owns the observer or save the > > instance to anonymous namespace and manage the lifetime explicitly? > > I can probably add/remove the observer in > > > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui... > > depends on the value of |activated| > > How about calling proxy->SetController(nullptr) from KC dtor? called proxy_.reset() directly. Otherwise, I need to add more code in SetController to handle nullptr case. It seems simpler to directly destroy proxy.
On 2015/05/15 16:10:42, bshe wrote: > PTAL, Thanks! > > https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right): > > https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:107: > content::BrowserContext* context_; > On 2015/05/14 15:37:22, oshima wrote: > > nit: new line before DISALLOW... > > Done. > > https://codereview.chromium.org/1128173003/diff/120001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:119: > keyboard_controller_->RemoveObserver(observer_.get()); > On 2015/05/15 15:37:00, sadrul wrote: > > On 2015/05/15 15:32:51, bshe wrote: > > > On 2015/05/14 15:37:22, oshima wrote: > > > > looks like the proxy is owned by controller, so this will be called during > > > > Keyboard controller's dtor, which means that this depend on the order of > > > proxy_ > > > > and observer_list_ in order to work correctly. > > > > > > > > Does this have to be in proxy? Or can you put this somewhere that has > > cleaner > > > > dependency? > > > > > > What do you think of letting ChromeShellDelegate owns the observer or save > the > > > instance to anonymous namespace and manage the lifetime explicitly? > > > I can probably add/remove the observer in > > > > > > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui... > > > depends on the value of |activated| > > > > How about calling proxy->SetController(nullptr) from KC dtor? > > called proxy_.reset() directly. Otherwise, I need to add more code in > SetController to handle nullptr case. It seems simpler to directly destroy > proxy. That's the same amount of code you have in dtor, which you can simply remove. maybe with DCHECK(!controller). Can you change dtor to DCHECK(controller_) controller_->RemoveObserver(observer_.get()); ? I still have slight preference for SetController(nullptr), but am fine if you want that way.
Yeah, I prefer SetController(null) too. (please make sure to update the doc to say the controller can be null when the controller is detached/destroyed from the proxy)
Changed to use SetController(nullptr) so the dtor doesn't need to do anything. PTAL Thanks for reviews!
https://codereview.chromium.org/1128173003/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right): https://codereview.chromium.org/1128173003/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:117: AshKeyboardControllerProxy::~AshKeyboardControllerProxy() {} can you add DCHECK(!keyboard_controller_) ? https://codereview.chromium.org/1128173003/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:169: keyboard_controller_->RemoveObserver(observer_.get()); keyboard_controller_ = nullptr;
https://codereview.chromium.org/1128173003/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right): https://codereview.chromium.org/1128173003/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:117: AshKeyboardControllerProxy::~AshKeyboardControllerProxy() {} On 2015/05/15 17:29:16, oshima wrote: > can you add DCHECK(!keyboard_controller_) ? Done. https://codereview.chromium.org/1128173003/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:169: keyboard_controller_->RemoveObserver(observer_.get()); On 2015/05/15 17:29:16, oshima wrote: > keyboard_controller_ = nullptr; Done.
lgtm
The CQ bit was checked by bshe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, rockot@chromium.org Link to the patchset: https://codereview.chromium.org/1128173003/#ps200001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128173003/200001
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2b52cba3b5a831c466311faba812d51ddd532ba5 Cr-Commit-Position: refs/heads/master@{#330169} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
