Mode Indicator using BubbleDelegateView.

Mode Indicator using BubbleDelegateView.

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

[Jun Mukai, 2013-12-02 21:08:41]

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/mode_indicator_controller.cc (right):

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/mode_indicator_controller.cc:17: class
ModeIndicatorObserver : public views::WidgetObserver {
What is this class for?  It just observes the widget but only the observing
method is OnWidgetDestroying which just cancels active_widget_.

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/mode_indicator_controller.cc:25: virtual
void OnWidgetDestroying(views::Widget* widget) OVERRIDE {
Please move below. Normal order is: ctor/dtor -> normal methods -> inherited.

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/mode_indicator_controller.cc:95:
DCHECK(imm_);
You don't have to DCHECK, because imm_ is set in ctor and you already checked.

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc
(right):

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc:37: virtual
gfx::Size GetPreferredSize() OVERRIDE {
You don't have to inherit Label. Actually you can declare
ModeIndicatorDelegateView::GetPreferredSize to do this.

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc:52:
set_shadow(views::BubbleBorder::NO_SHADOW);
should specify parent_window?
Otherwise it will be in the default container (BubbleContainer?) which might
cause unexpected behaviors.

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc:59:
set_arrow(views::BubbleBorder::BOTTOM_CENTER);
you don't have to do that. Bubble can take care of this (IIRC)

[Jun Mukai, 2013-12-03 01:09:28]

https://codereview.chromium.org/98703003/diff/60001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc
(right):

https://codereview.chromium.org/98703003/diff/60001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc:19: const
int kMinSize = 43;
Noticed that the bubble is slightly bigger than the past. Possibly this size is
too big, because bubble itself has margins (to draw the round corner).  Probably
this should be 31, 43 - kDefaultMargin (=6) * 2.
Or you should use margin().width() (and height()) in GetPreferredSize()

[Hiro Komatsu, 2013-12-03 02:39:35]

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/mode_indicator_controller.cc (right):

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/mode_indicator_controller.cc:17: class
ModeIndicatorObserver : public views::WidgetObserver {
On 2013/12/02 21:08:41, Jun Mukai wrote:
> What is this class for?  It just observes the widget but only the observing
> method is OnWidgetDestroying which just cancels active_widget_.

This is to close the previous widget immediately without fade-out animation. 
Added comment.

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/mode_indicator_controller.cc:25: virtual
void OnWidgetDestroying(views::Widget* widget) OVERRIDE {
On 2013/12/02 21:08:41, Jun Mukai wrote:
> Please move below. Normal order is: ctor/dtor -> normal methods -> inherited.

Done.

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/mode_indicator_controller.cc:95:
DCHECK(imm_);
On 2013/12/02 21:08:41, Jun Mukai wrote:
> You don't have to DCHECK, because imm_ is set in ctor and you already checked.

Done.

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc
(right):

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc:37: virtual
gfx::Size GetPreferredSize() OVERRIDE {
On 2013/12/02 21:08:41, Jun Mukai wrote:
> You don't have to inherit Label. Actually you can declare
> ModeIndicatorDelegateView::GetPreferredSize to do this.

Done.

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc:52:
set_shadow(views::BubbleBorder::NO_SHADOW);
On 2013/12/02 21:08:41, Jun Mukai wrote:
> should specify parent_window?
> Otherwise it will be in the default container (BubbleContainer?) which might
> cause unexpected behaviors.

Done.

https://codereview.chromium.org/98703003/diff/40001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc:59:
set_arrow(views::BubbleBorder::BOTTOM_CENTER);
On 2013/12/02 21:08:41, Jun Mukai wrote:
> you don't have to do that. Bubble can take care of this (IIRC)

It seems a bug of BubbleFrameView.  Till the bug is fixed, this work around is
necessary.

https://code.google.com/p/chromium/issues/detail?id=325009

Added a comment.

[Jun Mukai, 2013-12-03 02:43:58]

lgtm with a nit. Please wait for msw replying on the bug though.

https://codereview.chromium.org/98703003/diff/120001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc
(right):

https://codereview.chromium.org/98703003/diff/120001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc:47: //
https://code.google.com/p/chromium/issues/detail?id=325009
nit: normally use crbug.com/<id>

[Hiro Komatsu, 2013-12-03 03:35:49]

committing...

https://codereview.chromium.org/98703003/diff/120001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc
(right):

https://codereview.chromium.org/98703003/diff/120001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/mode_indicator_delegate_view.cc:47: //
https://code.google.com/p/chromium/issues/detail?id=325009
On 2013/12/03 02:43:59, Jun Mukai wrote:
> nit: normally use crbug.com/<id>

Done.

[commit-bot: I haz the power, 2013-12-03 03:36:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/komatsu@chromium.org/98703003/140001

[commit-bot: I haz the power, 2013-12-03 06:30:40]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-03 09:31:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/komatsu@chromium.org/98703003/140001

[commit-bot: I haz the power, 2013-12-03 11:29:49]

Change committed as 238349