Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6384004:
Browser should get notified if a views textfield changes focus. In addition, (Closed)
|
Created:
9 years, 11 months ago by varunjain Modified:
9 years, 7 months ago CC:
chromium-reviews, nkostylev+cc_chromium.org, davemoore+watch_chromium.org, Paweł Hajdan Jr. Visibility:
Public. |
DescriptionBrowser should get notified if a views textfield changes focus. In addition,
if the browser frame is touch based, it should update the virtual keyboard.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=73908
Patch Set 1 #Patch Set 2 : minor style fixes #Patch Set 3 : Re-did CL according to discussions #
Total comments: 12
Patch Set 4 : formatting fixes #Patch Set 5 : minor change #
Total comments: 24
Patch Set 6 : Modified according to reviewer comments. #Patch Set 7 : Minor changes that were missed in last patch #
Total comments: 6
Patch Set 8 : modified according to reviewer comments #
Total comments: 5
Patch Set 9 : Modified according to comments #
Total comments: 10
Patch Set 10 : modified according to comments #Patch Set 11 : fixed merge conflicts #
Messages
Total messages: 27 (0 generated)
Are you sure that you have done this the "right way". When I see a whole lot fo little changes, it usually makes me think that I should have included something in a superclass. Thoughts?
We chatted over IM and agreed that we need better way, so this won't be checked in. Let's discuss this f2f next week. - oshima On Fri, Jan 21, 2011 at 3:36 PM, <rjkroege@chromium.org> wrote: > Are you sure that you have done this the "right way". When I see a whole > lot fo > little changes, it usually makes me think that I should have included > something > in a superclass. > > Thoughts? > > > http://codereview.chromium.org/6384004/ >
Re-did the CL according to discussions... PTAL
Also, please have bryeung approve the interaction between this CL and the extension facility. http://codereview.chromium.org/6384004/diff/7001/chrome/browser/ui/touch/fram... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/7001/chrome/browser/ui/touch/fram... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:66: bool is_add, View* parent, View* child) { could be indented the other way probably? http://codereview.chromium.org/6384004/diff/7001/chrome/browser/ui/touch/fram... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:73: extra blank http://codereview.chromium.org/6384004/diff/7001/chrome/browser/ui/touch/fram... File chrome/browser/ui/touch/frame/touch_browser_frame_view.h (right): http://codereview.chromium.org/6384004/diff/7001/chrome/browser/ui/touch/fram... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:12: no blank line http://codereview.chromium.org/6384004/diff/7001/chrome/browser/ui/touch/fram... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:25: // A focus listner that performs touch specific tasks on focus change such sp listener http://codereview.chromium.org/6384004/diff/7001/views/controls/textfield/tex... File views/controls/textfield/textfield.h (right): http://codereview.chromium.org/6384004/diff/7001/views/controls/textfield/tex... views/controls/textfield/textfield.h:272: virtual View::VirtualKeyboardType GetVirtualKeyboardType(); these ought to be in alphabetic order? I'm wondering if you should fix that. http://codereview.chromium.org/6384004/diff/7001/views/view.h File views/view.h (right): http://codereview.chromium.org/6384004/diff/7001/views/view.h#newcode140 views/view.h:140: enum VirtualKeyboardType { I think that this does not belong here. The extension code would probably be the right place to define this. NO_KEYBOARD should come first.
http://codereview.chromium.org/6384004/diff/7001/chrome/browser/ui/touch/fram... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/7001/chrome/browser/ui/touch/fram... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:66: bool is_add, View* parent, View* child) { On 2011/01/27 00:04:22, rjkroege wrote: > could be indented the other way probably? Done. http://codereview.chromium.org/6384004/diff/7001/chrome/browser/ui/touch/fram... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:73: On 2011/01/27 00:04:22, rjkroege wrote: > extra blank Done. http://codereview.chromium.org/6384004/diff/7001/chrome/browser/ui/touch/fram... File chrome/browser/ui/touch/frame/touch_browser_frame_view.h (right): http://codereview.chromium.org/6384004/diff/7001/chrome/browser/ui/touch/fram... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:12: On 2011/01/27 00:04:22, rjkroege wrote: > no blank line Done. http://codereview.chromium.org/6384004/diff/7001/chrome/browser/ui/touch/fram... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:25: // A focus listner that performs touch specific tasks on focus change such On 2011/01/27 00:04:22, rjkroege wrote: > sp listener Done. http://codereview.chromium.org/6384004/diff/7001/views/controls/textfield/tex... File views/controls/textfield/textfield.h (right): http://codereview.chromium.org/6384004/diff/7001/views/controls/textfield/tex... views/controls/textfield/textfield.h:272: virtual View::VirtualKeyboardType GetVirtualKeyboardType(); On 2011/01/27 00:04:22, rjkroege wrote: > these ought to be in alphabetic order? I'm wondering if you should fix that. I think I recall a comment from one of my earlier CLs that the ordering is also inherited from the parent class? http://codereview.chromium.org/6384004/diff/7001/views/view.h File views/view.h (right): http://codereview.chromium.org/6384004/diff/7001/views/view.h#newcode140 views/view.h:140: enum VirtualKeyboardType { On 2011/01/27 00:04:22, rjkroege wrote: > I think that this does not belong here. The extension code would probably be the > right place to define this. NO_KEYBOARD should come first. moved NO_KEYBOARD I thought we decided in the discussion that virtual keyboard type is a property of every view.. no?
On 2011/01/27 00:19:34, varunjain wrote: > ... > > http://codereview.chromium.org/6384004/diff/7001/views/view.h#newcode140 > views/view.h:140: enum VirtualKeyboardType { > On 2011/01/27 00:04:22, rjkroege wrote: > > I think that this does not belong here. The extension code would probably be > the > > right place to define this. NO_KEYBOARD should come first. > > moved NO_KEYBOARD > I thought we decided in the discussion that virtual keyboard type is a property > of every view.. no? I think we agreed that every view should have a virtual keyboard type. But I am opposed to defining the virtual keyboard types in view.h -- it is something that the extension framework needs to know about and that means something that can be shared between the keyboard extension host and the view code.
http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:66: View* child) { fix indentation. see my comment in header. http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:68: if (!focus_change_listener_.get()) { You need to check GetWidget(). This assumes that this view is always added to a parent view that is already added to widget. http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:147: if (!focused_before || (focused_now && focused_now->GetVirtualKeyboardType() moving the "(forcusd_now && .." to next line would make it more readable. http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.h (right): http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:28: TouchBrowserFocusChangeListener(TouchBrowserFrameView* browser_frame); explicit http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:32: views::View* focused_now); indent should be either function(a, b) function( a, b); or function( a , b); (your pick) http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:35: TouchBrowserFrameView* touch_browser_frame_; DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:36: }; Does this needs to be here? If not, I'd prefer to make this private to implementation. http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:62: scoped_ptr<TouchBrowserFocusChangeListener> focus_change_listener_; can this be just a FocusChangeListener? http://codereview.chromium.org/6384004/diff/17001/views/view.h File views/view.h (right): http://codereview.chromium.org/6384004/diff/17001/views/view.h#newcode146 views/view.h:146: URL_KEYBOARD, The name of the enum should be more generic, such as InputType, ContextType or something like that. We may wan t to use something similar to what WebKit is using for form field types. Also I won't add more than we need. NONE, NORMAL, URL would be sufficient for now. NORMAL_/GENERIC_ sounds better than REGULAR_. I'm probably not qualified for this, so if you prefer REGULAR_, I'm fine. I'd not add more than we need right now. NONE, REGULAR, URL would be enough. (http://en.wikipedia.org/wiki/You_ain't_gonna_need_it)
Why wasn't I a reviewer on this originally? (Considering I wrote the touch_browser_frame_view and it sounds like it involves the virtual keyboard.) Looking now...
This CL seems to be trying to do two separate things: (a) define a virtual keyboard type, and (b) make keyboard appear when focus changes into a views TextField. I would like (a) to be removed from this CL. We need to think more carefully about how to do that, and how to make it properly integrated with the extension framework. It should be possible to determine if a TextField is focused without it. (Either by checking the view class name, or by adding a simple "RequiresTextInput" style of method for now.) http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:65: void TouchBrowserFrameView::ViewHierarchyChanged(bool is_add, View* parent, Why are we doing this in ViewHierarchyChanged? Is that the standard idiom? (Didn't seem like it from a tiny bit of code searching I did.) http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:70: GetFocusManager()->AddFocusChangeListener(focus_change_listener_.get()); Related to oshima's point: GetFocusManager can return NULL. http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.h (right): http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:26: class TouchBrowserFocusChangeListener : public views::FocusChangeListener { It seems a more common idiom might be to make TouchBrowserFrameView a views::FocusChangeListener. That would also allow you to keep UpdateKeyboardAndLayout private, which I would prefer.
I agree that having "KeyboardType" in views.h is wrong. Having generic Input/ContentType in views that can be used to determine virtual keyboard type that touch_ui uses should be okay though. This has been discussed with varun, robert and me, but you were not there, which is probably mistake. Bay be we should have another session? http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:65: void TouchBrowserFrameView::ViewHierarchyChanged(bool is_add, View* parent, On 2011/01/27 20:50:34, bryeung wrote: > Why are we doing this in ViewHierarchyChanged? Is that the standard idiom? > (Didn't seem like it from a tiny bit of code searching I did.) Yes, this is a standard item to initialize things that require Widget.
I agree that having "KeyboardType" in views.h is wrong. Having generic Input/ContentType in views that can be used to determine virtual keyboard type that touch_ui uses should be okay though. This has been discussed with varun, robert and me, but you were not there, which is probably mistake. Bay be we should have another session? http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:65: void TouchBrowserFrameView::ViewHierarchyChanged(bool is_add, View* parent, On 2011/01/27 20:50:34, bryeung wrote: > Why are we doing this in ViewHierarchyChanged? Is that the standard idiom? > (Didn't seem like it from a tiny bit of code searching I did.) Yes, this is a standard item to initialize things that require Widget.
On Thu, Jan 27, 2011 at 1:35 PM, <oshima@chromium.org> wrote: > I agree that having "KeyboardType" in views.h is wrong. > Having generic Input/ContentType in views that can be used to determine > virtual > keyboard type that touch_ui uses should be okay though. > This has been discussed with varun, robert and me, but you were not there, > which is probably mistake. Bay be we should have another session? > > > > > http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... > File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): > > > http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... > chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:65: void > TouchBrowserFrameView::ViewHierarchyChanged(bool is_add, View* parent, > On 2011/01/27 20:50:34, bryeung wrote: > >> Why are we doing this in ViewHierarchyChanged? Is that the standard >> > idiom? > >> (Didn't seem like it from a tiny bit of code searching I did.) >> > > Yes, this is a standard item to initialize things that require Widget. doh, s/item/idiom/ - oshima > > > http://codereview.chromium.org/6384004/ >
@rjkroege, If you are concerned simply about the placement of the enum, I can move it to wherever you like. I am not very familiar with extensions code... so I am confused as to why can the extension host not share it from view.h. @bryeung, I need some part of (a) to implement (b). Oshima and I did discuss checking the view classname earlier but decided that its an ugly(er) hack. What I have in this CL (after incorporating Oshima's comments) is very close to your RequiresTextInput method. http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:66: View* child) { On 2011/01/27 19:58:33, oshima wrote: > fix indentation. see my comment in header. Done. http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:68: if (!focus_change_listener_.get()) { On 2011/01/27 19:58:33, oshima wrote: > You need to check GetWidget(). This assumes that this view is always added to a > parent view that is already added to widget. GetWidget is checked inside GetFocusManager. I will check GetFocusManager here. http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:70: GetFocusManager()->AddFocusChangeListener(focus_change_listener_.get()); On 2011/01/27 20:50:34, bryeung wrote: > Related to oshima's point: GetFocusManager can return NULL. Done. http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:147: if (!focused_before || (focused_now && focused_now->GetVirtualKeyboardType() On 2011/01/27 19:58:33, oshima wrote: > moving the "(forcusd_now && .." to next line would make it more readable. Done. http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.h (right): http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:26: class TouchBrowserFocusChangeListener : public views::FocusChangeListener { On 2011/01/27 20:50:34, bryeung wrote: > It seems a more common idiom might be to make TouchBrowserFrameView a > views::FocusChangeListener. > > That would also allow you to keep UpdateKeyboardAndLayout private, which I would > prefer. Done. http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:28: TouchBrowserFocusChangeListener(TouchBrowserFrameView* browser_frame); On 2011/01/27 19:58:33, oshima wrote: > explicit not needed anymore. http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:32: views::View* focused_now); On 2011/01/27 19:58:33, oshima wrote: > indent should be either > function(a, > b) > function( > a, > b); > or > function( > a , b); > (your pick) Done. http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:35: TouchBrowserFrameView* touch_browser_frame_; On 2011/01/27 19:58:33, oshima wrote: > DISALLOW_COPY_AND_ASSIGN not needed anymore http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:36: }; On 2011/01/27 19:58:33, oshima wrote: > Does this needs to be here? If not, I'd prefer to make this > private to implementation. not needed anymore http://codereview.chromium.org/6384004/diff/17001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:62: scoped_ptr<TouchBrowserFocusChangeListener> focus_change_listener_; On 2011/01/27 19:58:33, oshima wrote: > can this be just a FocusChangeListener? not needed anymore http://codereview.chromium.org/6384004/diff/17001/views/view.h File views/view.h (right): http://codereview.chromium.org/6384004/diff/17001/views/view.h#newcode146 views/view.h:146: URL_KEYBOARD, On 2011/01/27 19:58:33, oshima wrote: > The name of the enum should be more generic, such as InputType, ContextType or > something like that. We may wan t to use something similar to what WebKit is > using for form field types. > > Also I won't add more than we need. NONE, NORMAL, URL would be sufficient for > now. > > NORMAL_/GENERIC_ sounds better than REGULAR_. I'm probably not qualified for > this, so if you prefer REGULAR_, I'm fine. > > I'd not add more than we need right now. NONE, REGULAR, URL would be enough. > (http://en.wikipedia.org/wiki/You_ain%27t_gonna_need_it) Done.
Ben, please review changes to view.{h|cc}
please add Ben for views change. http://codereview.chromium.org/6384004/diff/36001/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/36001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:56: } no {} for single line condition. http://codereview.chromium.org/6384004/diff/36001/views/view.cc File views/view.cc (right): http://codereview.chromium.org/6384004/diff/36001/views/view.cc#newcode1490 views/view.cc:1490: View::ContentType View::GetContentType() { const http://codereview.chromium.org/6384004/diff/36001/views/view.h File views/view.h (right): http://codereview.chromium.org/6384004/diff/36001/views/view.h#newcode999 views/view.h:999: virtual ContentType GetContentType(); const
http://codereview.chromium.org/6384004/diff/36001/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/36001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:56: } On 2011/01/27 23:39:09, oshima wrote: > no {} for single line condition. moved removing the focus listener to ViewHierarchyCHange. PTAL http://codereview.chromium.org/6384004/diff/36001/views/view.cc File views/view.cc (right): http://codereview.chromium.org/6384004/diff/36001/views/view.cc#newcode1490 views/view.cc:1490: View::ContentType View::GetContentType() { On 2011/01/27 23:39:09, oshima wrote: > const Done. http://codereview.chromium.org/6384004/diff/36001/views/view.h File views/view.h (right): http://codereview.chromium.org/6384004/diff/36001/views/view.h#newcode999 views/view.h:999: virtual ContentType GetContentType(); On 2011/01/27 23:39:09, oshima wrote: > const Done.
http://codereview.chromium.org/6384004/diff/42001/views/view.h File views/view.h (right): http://codereview.chromium.org/6384004/diff/42001/views/view.h#newcode140 views/view.h:140: enum ContentType { I would rather you use view ids and maybe static_cast in your frame view than add this to all views.
On 2011/01/28 15:27:24, Ben Goodger wrote: > http://codereview.chromium.org/6384004/diff/42001/views/view.h > File views/view.h (right): > > http://codereview.chromium.org/6384004/diff/42001/views/view.h#newcode140 > views/view.h:140: enum ContentType { > I would rather you use view ids and maybe static_cast in your frame view than > add this to all views. I think the description is misleading and wrong. This should be "a mechanism to allow a view that needs to handle key events to use virtual keyboard." Since views allow arbitrary views to have a focus and handle key event, static casting to Textfield does not work. LocationBarView is such an example, and focus can be either on LocationBarView or on Textfield itself (via alt-shift-t).
http://codereview.chromium.org/6384004/diff/42001/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/42001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:72: // TODO: support other types of keyboard. this should have a name attached to it http://codereview.chromium.org/6384004/diff/42001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:91: if (is_add && child == this && !focus_listener_added_ && GetFocusManager()) { Maybe this would be easier to understand if you pulled out the common conditions? Chromium style would encourage you to early exit if (!GetFocusManager() || child != this).
Talked to oshima for context, here's my recommendation for this change: - Don't modify view.h/cc - In the virtual keyboard code, check to see if the view id the Omnibox ID (GetViewId) - If it isn't, then check to see if the view's classname is the textfield's classname (GetClassName on View... it's View's "RTTI"). "URL" vs "Generic input" smells much too much like browser functionality to belong in src/views itself. Think of views like a lower level of the abstraction onion... it is a generic UI toolkit and so browser stuff (e.g. URLs) should be kept out. -Ben
I have removed all view.h/cc changes. PTAL http://codereview.chromium.org/6384004/diff/42001/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/42001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:72: // TODO: support other types of keyboard. On 2011/02/01 19:03:53, bryeung wrote: > this should have a name attached to it Done. http://codereview.chromium.org/6384004/diff/42001/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:91: if (is_add && child == this && !focus_listener_added_ && GetFocusManager()) { On 2011/02/01 19:03:53, bryeung wrote: > Maybe this would be easier to understand if you pulled out the common > conditions? Chromium style would encourage you to early exit if > (!GetFocusManager() || child != this). Done.
http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:91: if (!GetFocusManager() || child != this) child != this is not necessary. This does not work if you first add this to the parent, and then parent is added to widget. http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.h (right): http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:40: virtual void ViewHierarchyChanged(bool is_add, View* parent, View* child); any specific reason why they're in protected? http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:63: virtual VirtualKeyboardType GetKeyboardType(views::View* view); why virtual? can this be just a static function, possibly in anonymous namespace?
http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.h (right): http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:68: bool focus_listener_added_; put the bools next to each other so the compiler packs the struct.
http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:190: else nit: no else after return
http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right): http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:91: if (!GetFocusManager() || child != this) On 2011/02/04 10:15:27, oshima wrote: > child != this is not necessary. This does not work if you first add this to the > parent, and then parent is added to widget. Done. http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:190: else On 2011/02/04 15:57:22, Ben Goodger wrote: > nit: no else after return Done. http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... File chrome/browser/ui/touch/frame/touch_browser_frame_view.h (right): http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:40: virtual void ViewHierarchyChanged(bool is_add, View* parent, View* child); On 2011/02/04 10:15:27, oshima wrote: > any specific reason why they're in protected? Its listed as protected in views::View http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:63: virtual VirtualKeyboardType GetKeyboardType(views::View* view); On 2011/02/04 10:15:27, oshima wrote: > why virtual? can this be just a static function, possibly in anonymous > namespace? Done. http://codereview.chromium.org/6384004/diff/37007/chrome/browser/ui/touch/fra... chrome/browser/ui/touch/frame/touch_browser_frame_view.h:68: bool focus_listener_added_; On 2011/02/04 15:18:30, rjkroege wrote: > put the bools next to each other so the compiler packs the struct. Done.
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
