Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3474007:
Forward textfield focus event to the browser. (Closed)
|
Created:
10 years, 3 months ago by varunjain Modified:
9 years, 7 months ago CC:
chromium-reviews, ben+cc_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org Base URL:
http://src.chromium.org/git/chromium.git Visibility:
Public. |
DescriptionForward textfield focus event to the browser.
Add two messages in IPC channel: 1. The renderer needs to inform the browser if an element that can accept use input gains focus. This is useful when the browser may want to take some action against such events. For example, in case a textfield is focused, the browser may want to show an on-screen keyboard. 2. The browser needs some way to inform the renderer to scroll the currently focused element into the view frame. This is useful when, for instance, the browser resize the frame and the focused element goes out of visible frame.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=64142
Patch Set 1 #
Total comments: 19
Patch Set 2 : Addressed reviewer's comments #
Total comments: 12
Patch Set 3 : Removed unnecessary IPC message. #Patch Set 4 : The WebKit part of this CL was changed to address reviewer's concerns. This patch accounts for that. #Patch Set 5 : Accomodated WebKit CL changes #Patch Set 6 : Accomodated minor changes in the WebKit patch #
Total comments: 12
Patch Set 7 : Addressed reviewer's comments #
Total comments: 2
Patch Set 8 : Addressed reviewer's comments #Patch Set 9 : Modified according to reviewer's comments #
Total comments: 10
Patch Set 10 : Addressed reviewer's comments #
Total comments: 8
Patch Set 11 : Minor changes to address reviewer's comments #Patch Set 12 : Fixed merge conflict #
Messages
Total messages: 21 (0 generated)
Please see the recent discussion on the team list about not checking in LOG messages of questionable usefulness. http://codereview.chromium.org/3474007/diff/1/2 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/3474007/diff/1/2#newcode2069 chrome/browser/renderer_host/render_view_host.cc:2069: // TODO(varunjain): implement this Things that are not implemented should generally have a NOTIMPLEMENTED(). Though maybe in this case that would be too annoying? http://codereview.chromium.org/3474007/diff/1/2#newcode2070 chrome/browser/renderer_host/render_view_host.cc:2070: LOG(INFO) << "Input element focussed"; this log should not get checked in http://codereview.chromium.org/3474007/diff/1/2#newcode2075 chrome/browser/renderer_host/render_view_host.cc:2075: Send(new ViewMsg_ScrollFocusedNodeIntoView(routing_id())); Should this be done here? If the keyboard is to slide into place, we should probably be waiting for the animation to finish before firing this event. http://codereview.chromium.org/3474007/diff/1/2#newcode2077 chrome/browser/renderer_host/render_view_host.cc:2077: LOG(INFO) << "No input element focused"; remove http://codereview.chromium.org/3474007/diff/1/3 File chrome/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/3474007/diff/1/3#newcode684 chrome/browser/renderer_host/render_view_host.h:684: void OnInputElementFocused(bool inputElementFocused); We might want to give some more thought to the naming, and run it by some of the Chrome-masters. We're not really firing this on every input element focus, right? e.g. if a checkbox gets focused, I'm assuming we are currently not firing this (nor do we want to be). http://codereview.chromium.org/3474007/diff/1/4 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/3474007/diff/1/4#newcode1070 chrome/common/render_messages_internal.h:1070: // when an input element is focused. This comment needs to be updated to describe the actual conditions when this message is sent, instead of the desired effect. http://codereview.chromium.org/3474007/diff/1/4#newcode1072 chrome/common/render_messages_internal.h:1072: bool /* input element is focused (true) or not (false) */) I'm a bit confused about why we need a boolean here. http://codereview.chromium.org/3474007/diff/1/5 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3474007/diff/1/5#newcode2212 chrome/renderer/render_view.cc:2212: LOG(INFO) << "send focus event to browser"; not sufficiently helpful to get checked in. http://codereview.chromium.org/3474007/diff/1/5#newcode2215 chrome/renderer/render_view.cc:2215: Send(new ViewHostMsg_InputElementFocused(routing_id_, false)); why are we sending it at all in this case?
http://codereview.chromium.org/3474007/diff/1/2 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/3474007/diff/1/2#newcode2069 chrome/browser/renderer_host/render_view_host.cc:2069: // TODO(varunjain): implement this On 2010/09/23 17:23:15, bryeung wrote: > Things that are not implemented should generally have a NOTIMPLEMENTED(). > Though maybe in this case that would be too annoying? yes.. LOG(ERROR) would be very annoying. Hence the LOG(INFO). I feel the TODO is enough. So I'll get rid of the LOG(INFO) http://codereview.chromium.org/3474007/diff/1/2#newcode2070 chrome/browser/renderer_host/render_view_host.cc:2070: LOG(INFO) << "Input element focussed"; On 2010/09/23 17:23:15, bryeung wrote: > this log should not get checked in Done. http://codereview.chromium.org/3474007/diff/1/2#newcode2075 chrome/browser/renderer_host/render_view_host.cc:2075: Send(new ViewMsg_ScrollFocusedNodeIntoView(routing_id())); On 2010/09/23 17:23:15, bryeung wrote: > Should this be done here? > > If the keyboard is to slide into place, we should probably be waiting for the > animation to finish before firing this event. Yes.. this will need to be moved to the right place once we have the keyboard code ready. Its here for now as a placeholder and so that I can test the complete workflow. http://codereview.chromium.org/3474007/diff/1/2#newcode2077 chrome/browser/renderer_host/render_view_host.cc:2077: LOG(INFO) << "No input element focused"; On 2010/09/23 17:23:15, bryeung wrote: > remove Done. http://codereview.chromium.org/3474007/diff/1/3 File chrome/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/3474007/diff/1/3#newcode684 chrome/browser/renderer_host/render_view_host.h:684: void OnInputElementFocused(bool inputElementFocused); On 2010/09/23 17:23:15, bryeung wrote: > We might want to give some more thought to the naming, and run it by some of the > Chrome-masters. > > We're not really firing this on every input element focus, right? e.g. if a > checkbox gets focused, I'm assuming we are currently not firing this (nor do we > want to be). I am fine with renaming. In fact, I agree that InputElement is too generic. Previously I name it "FocusedElementNeedsKeyboard". But it felt too long. Do you have any suggestions? http://codereview.chromium.org/3474007/diff/1/4 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/3474007/diff/1/4#newcode1070 chrome/common/render_messages_internal.h:1070: // when an input element is focused. On 2010/09/23 17:23:15, bryeung wrote: > This comment needs to be updated to describe the actual conditions when this > message is sent, instead of the desired effect. Done. http://codereview.chromium.org/3474007/diff/1/4#newcode1072 chrome/common/render_messages_internal.h:1072: bool /* input element is focused (true) or not (false) */) On 2010/09/23 17:23:15, bryeung wrote: > I'm a bit confused about why we need a boolean here. we need to identify when a non-input element is focused to be able to hide the keyboard. http://codereview.chromium.org/3474007/diff/1/5 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3474007/diff/1/5#newcode2212 chrome/renderer/render_view.cc:2212: LOG(INFO) << "send focus event to browser"; On 2010/09/23 17:23:15, bryeung wrote: > not sufficiently helpful to get checked in. Done. http://codereview.chromium.org/3474007/diff/1/5#newcode2215 chrome/renderer/render_view.cc:2215: Send(new ViewHostMsg_InputElementFocused(routing_id_, false)); On 2010/09/23 17:23:15, bryeung wrote: > why are we sending it at all in this case? To hide the keyboard.
http://codereview.chromium.org/3474007/diff/9001/7002 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/3474007/diff/9001/7002#newcode2068 chrome/browser/renderer_host/render_view_host.cc:2068: // Need to summon on-screen keyboard do you have code for this too? http://codereview.chromium.org/3474007/diff/9001/7003 File chrome/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/3474007/diff/9001/7003#newcode684 chrome/browser/renderer_host/render_view_host.h:684: void OnInputElementFocused(bool inputElementFocused); should be input_element_focused http://codereview.chromium.org/3474007/diff/9001/7005 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3474007/diff/9001/7005#newcode2212 chrome/renderer/render_view.cc:2212: Send(new ViewHostMsg_InputElementFocused(routing_id_, true)); why add a new ipc message? couldn't the FocusedNodeChanged message be augmented? http://codereview.chromium.org/3474007/diff/9001/7005#newcode5834 chrome/renderer/render_view.cc:5834: webview()->scrollFocusedNodeIntoView(); does this work for iframes, content overflow, etc.?
http://codereview.chromium.org/3474007/diff/1/2 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/3474007/diff/1/2#newcode2077 chrome/browser/renderer_host/render_view_host.cc:2077: LOG(INFO) << "No input element focused"; On 2010/09/23 18:12:25, varunjain wrote: > On 2010/09/23 17:23:15, bryeung wrote: > > remove > > Done. TODO should probably be addressed to me, and should note that what needs to happen here is hiding the keyboard. http://codereview.chromium.org/3474007/diff/9001/7002#newcode2068 chrome/browser/renderer_host/render_view_host.cc:2068: // Need to summon on-screen keyboard On 2010/09/23 18:29:14, rjkroege wrote: > do you have code for this too? This is code of mine that will be coming soon. Varun: the TODO should probably have my name, not yours. http://codereview.chromium.org/3474007/diff/9001/7003 File chrome/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/3474007/diff/9001/7003#newcode684 chrome/browser/renderer_host/render_view_host.h:684: void OnInputElementFocused(bool inputElementFocused); Maybe "OnElementNeedsInput"? http://codereview.chromium.org/3474007/diff/9001/7005 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3474007/diff/9001/7005#newcode2212 chrome/renderer/render_view.cc:2212: Send(new ViewHostMsg_InputElementFocused(routing_id_, true)); On 2010/09/23 18:29:14, rjkroege wrote: > why add a new ipc message? couldn't the FocusedNodeChanged message be augmented? This is a good point. I'm interested to hear the reasoning.
http://codereview.chromium.org/3474007/diff/9001/7002 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/3474007/diff/9001/7002#newcode2068 chrome/browser/renderer_host/render_view_host.cc:2068: // Need to summon on-screen keyboard On 2010/09/23 18:56:58, bryeung wrote: > On 2010/09/23 18:29:14, rjkroege wrote: > > do you have code for this too? > > This is code of mine that will be coming soon. > > Varun: the TODO should probably have my name, not yours. Done. http://codereview.chromium.org/3474007/diff/9001/7003 File chrome/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/3474007/diff/9001/7003#newcode684 chrome/browser/renderer_host/render_view_host.h:684: void OnInputElementFocused(bool inputElementFocused); On 2010/09/23 18:29:14, rjkroege wrote: > should be input_element_focused Done. http://codereview.chromium.org/3474007/diff/9001/7003#newcode684 chrome/browser/renderer_host/render_view_host.h:684: void OnInputElementFocused(bool inputElementFocused); On 2010/09/23 18:56:58, bryeung wrote: > Maybe "OnElementNeedsInput"? Done. http://codereview.chromium.org/3474007/diff/9001/7005 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3474007/diff/9001/7005#newcode2212 chrome/renderer/render_view.cc:2212: Send(new ViewHostMsg_InputElementFocused(routing_id_, true)); On 2010/09/23 18:29:14, rjkroege wrote: > why add a new ipc message? couldn't the FocusedNodeChanged message be augmented? I added a new one mostly because my message has a parameter which would be meaningless for the cases this message is addressing. But I just checked, and this message is only addressing this case.. so that reason doesnot apply anymore. Should have checked before and saved myself a couple of lines of code :( http://codereview.chromium.org/3474007/diff/9001/7005#newcode5834 chrome/renderer/render_view.cc:5834: webview()->scrollFocusedNodeIntoView(); On 2010/09/23 18:29:14, rjkroege wrote: > does this work for iframes, content overflow, etc.? yes... I tested for iframes and overflows.
LGTM: but you should probably have someone else on the team look it over, since you're changing messages across the IPC bridge.
sky, darin, can you please have a look. The WebKit part of this CL is submitted (https://bugs.webkit.org/show_bug.cgi?id=46296)
http://codereview.chromium.org/3474007/diff/26001/27001 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/3474007/diff/26001/27001#newcode1841 chrome/browser/renderer_host/render_view_host.cc:1841: Send(new ViewMsg_ScrollFocusedNodeIntoView(routing_id())); why do you want to route this over IPC? i mean, why not encode this logic directly in RenderView when you observe focus change? what if the focused node changes by the time you process this IPC here in the browser process? how much code do you expect to be adding here per these TODOs? note: we should be very careful about dumping code into Render{Widget,View}* as there is already a lot going on in these files. if you are going to be adding a lot of code, we should consider extracting it into a helper class that can be bolted on. http://codereview.chromium.org/3474007/diff/26001/27003 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/3474007/diff/26001/27003#newcode79 chrome/common/render_messages_internal.h:79: IPC_MESSAGE_ROUTED0(ViewMsg_ScrollFocusedNodeIntoView) how about putting this message nearby other messages that have something to do with focus? i know this file may seem like a massive dumping group (and it is), but we should try to keep some kind of organization. as is, this chnage makes the situation worse. http://codereview.chromium.org/3474007/diff/26001/27004 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3474007/diff/26001/27004#newcode2214 chrome/renderer/render_view.cc:2214: const WebElement* element = static_cast<const WebElement*>(&node); please use WebNode::toConst<T>() for this. const WebElement element = node.toConst<WebElement>(); element_needs_input = element.isTextFormControlElement(); http://codereview.chromium.org/3474007/diff/26001/27004#newcode2217 chrome/renderer/render_view.cc:2217: if (node.isContentEditable()) { it seems like you should evaluate this branch first since it overrides the previous branch. if (node.isContentEditable()) { element_needs_input = true; } else if (node.isElementNode()) { element_needs_input = node.toConst<WebElement>().isTextFormControlElement(); } http://codereview.chromium.org/3474007/diff/26001/27004#newcode5969 chrome/renderer/render_view.cc:5969: if (webview()) { nit: no need for the extra brackets here. the existing code only uses brackets when the body is multi-line or when there is an "else" clause. http://codereview.chromium.org/3474007/diff/26001/27005 File chrome/renderer/render_view.h (right): http://codereview.chromium.org/3474007/diff/26001/27005#newcode878 chrome/renderer/render_view.h:878: void OnScrollFocusedNodeIntoView(); this method should be listed in the same order as the IPCs listed in render_messages_internal.h
http://codereview.chromium.org/3474007/diff/26001/27001 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/3474007/diff/26001/27001#newcode1841 chrome/browser/renderer_host/render_view_host.cc:1841: Send(new ViewMsg_ScrollFocusedNodeIntoView(routing_id())); On 2010/10/14 22:15:24, darin wrote: > why do you want to route this over IPC? i mean, why not encode this logic > directly in RenderView when you observe focus change? focus change does not push the focused node out of view. It is the change in the view size cause by the browser (in this case, to display the on screen keyboard) that might do it. Hence, what part of the render view is visible can only be determined after the browser has done its thing. Then we need to scroll the focused node to make it visible. > > what if the focused node changes by the time you process this IPC here in the > browser process? That could end up scrolling the wrong node into view... but in that case, there will be another focusChanged IPC sent from the renderer and everything will come to order in processing the second message. > > how much code do you expect to be adding here per these TODOs? note: we should > be very careful about dumping code into Render{Widget,View}* as there is already > a lot going on in these files. if you are going to be adding a lot of code, we > should consider extracting it into a helper class that can be bolted on. perhaps Bryan can answer this question better... AFAIK, most of the code is already in helper classes and code needed here should not be more than ~5 lines. http://codereview.chromium.org/3474007/diff/26001/27003 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/3474007/diff/26001/27003#newcode79 chrome/common/render_messages_internal.h:79: IPC_MESSAGE_ROUTED0(ViewMsg_ScrollFocusedNodeIntoView) On 2010/10/14 22:15:24, darin wrote: > how about putting this message nearby other messages that have something to do > with focus? > > i know this file may seem like a massive dumping group (and it is), but we > should try to keep some kind of organization. as is, this chnage makes the > situation worse. I am fine with moving this message to anywhere. I looked around briefly before adding it and didnot find any other related message to group it with and hence I added it at the top. What I can do is put it with messages that set focus to a particular node (thats the only one I could find that is slightly related). If you have a better place in mind please let me know. http://codereview.chromium.org/3474007/diff/26001/27004 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3474007/diff/26001/27004#newcode2214 chrome/renderer/render_view.cc:2214: const WebElement* element = static_cast<const WebElement*>(&node); On 2010/10/14 22:15:24, darin wrote: > please use WebNode::toConst<T>() for this. > > const WebElement element = node.toConst<WebElement>(); > element_needs_input = element.isTextFormControlElement(); Done. http://codereview.chromium.org/3474007/diff/26001/27004#newcode2217 chrome/renderer/render_view.cc:2217: if (node.isContentEditable()) { On 2010/10/14 22:15:24, darin wrote: > it seems like you should evaluate this branch first since it overrides the > previous branch. > > if (node.isContentEditable()) { > element_needs_input = true; > } else if (node.isElementNode()) { > element_needs_input = > node.toConst<WebElement>().isTextFormControlElement(); > } Done. http://codereview.chromium.org/3474007/diff/26001/27004#newcode5969 chrome/renderer/render_view.cc:5969: if (webview()) { On 2010/10/14 22:15:24, darin wrote: > nit: no need for the extra brackets here. the existing code only uses brackets > when the body is multi-line or when there is an "else" clause. Done. http://codereview.chromium.org/3474007/diff/26001/27005 File chrome/renderer/render_view.h (right): http://codereview.chromium.org/3474007/diff/26001/27005#newcode878 chrome/renderer/render_view.h:878: void OnScrollFocusedNodeIntoView(); On 2010/10/14 22:15:24, darin wrote: > this method should be listed in the same order as the IPCs listed in > render_messages_internal.h The complete list seems to be out of order (for instance, OnZoom should have appeared before OnUpdateWebPref according to your sorting order). In general, I would not recommend formatting a file using a strategy dependent on another file since its hard to keep sync. I would suggest a simple alphabetical ordering and that reordering of all functions here and in the .cc file be done in a separate CL.
On Thu, Oct 14, 2010 at 7:12 PM, <varunjain@chromium.org> wrote: > perhaps Bryan can answer this question better... AFAIK, most of the code > is already in helper classes and code needed here should not be more > than ~5 lines. It shouldn't be much code, and if it is too much I can move it elsewhere when that CL comes. Bryan
On Thu, Oct 14, 2010 at 4:12 PM, <varunjain@chromium.org> wrote: > > http://codereview.chromium.org/3474007/diff/26001/27001 > File chrome/browser/renderer_host/render_view_host.cc (right): > > http://codereview.chromium.org/3474007/diff/26001/27001#newcode1841 > chrome/browser/renderer_host/render_view_host.cc:1841: Send(new > ViewMsg_ScrollFocusedNodeIntoView(routing_id())); > On 2010/10/14 22:15:24, darin wrote: > >> why do you want to route this over IPC? i mean, why not encode this >> > logic > >> directly in RenderView when you observe focus change? >> > > focus change does not push the focused node out of view. It is the > change in the view size cause by the browser (in this case, to display > the on screen keyboard) that might do it. Hence, what part of the render > view is visible can only be determined after the browser has done its > thing. Then we need to scroll the focused node to make it visible. I see... it was a bit hard to evaluate this patch given that key elements are absent. Just looking at the code you have presented, there doesn't seem to be any reason to have this IPC. But, now I think I understand a bit better. > > > > what if the focused node changes by the time you process this IPC here >> > in the > >> browser process? >> > > That could end up scrolling the wrong node into view... but in that > case, there will be another focusChanged IPC sent from the renderer and > everything will come to order in processing the second message. Isn't there still a race condition though? Imagine if focus is given to a text field, and then programmatically focus is given to some non text field. In that case, couldn't you end up scrolling the view to something that is not a text field? Maybe that doesn't matter, but it seems to be your intent to only scroll the view to the focused text field. > > > > how much code do you expect to be adding here per these TODOs? note: >> > we should > >> be very careful about dumping code into Render{Widget,View}* as there >> > is already > >> a lot going on in these files. if you are going to be adding a lot of >> > code, we > >> should consider extracting it into a helper class that can be bolted >> > on. > > perhaps Bryan can answer this question better... AFAIK, most of the code > is already in helper classes and code needed here should not be more > than ~5 lines. > > > http://codereview.chromium.org/3474007/diff/26001/27003 > File chrome/common/render_messages_internal.h (right): > > http://codereview.chromium.org/3474007/diff/26001/27003#newcode79 > chrome/common/render_messages_internal.h:79: > IPC_MESSAGE_ROUTED0(ViewMsg_ScrollFocusedNodeIntoView) > On 2010/10/14 22:15:24, darin wrote: > >> how about putting this message nearby other messages that have >> > something to do > >> with focus? >> > > i know this file may seem like a massive dumping group (and it is), >> > but we > >> should try to keep some kind of organization. as is, this chnage >> > makes the > >> situation worse. >> > > I am fine with moving this message to anywhere. I looked around briefly > before adding it and didnot find any other related message to group it > with and hence I added it at the top. > What I can do is put it with messages that set focus to a particular > node (thats the only one I could find that is slightly related). If you > have a better place in mind please let me know. That sounds like an improvement, thanks! > > > http://codereview.chromium.org/3474007/diff/26001/27004 > File chrome/renderer/render_view.cc (right): > > http://codereview.chromium.org/3474007/diff/26001/27004#newcode2214 > chrome/renderer/render_view.cc:2214: const WebElement* element = > static_cast<const WebElement*>(&node); > On 2010/10/14 22:15:24, darin wrote: > >> please use WebNode::toConst<T>() for this. >> > > const WebElement element = node.toConst<WebElement>(); >> element_needs_input = element.isTextFormControlElement(); >> > > Done. > > > http://codereview.chromium.org/3474007/diff/26001/27004#newcode2217 > chrome/renderer/render_view.cc:2217: if (node.isContentEditable()) { > On 2010/10/14 22:15:24, darin wrote: > >> it seems like you should evaluate this branch first since it overrides >> > the > >> previous branch. >> > > if (node.isContentEditable()) { >> element_needs_input = true; >> } else if (node.isElementNode()) { >> element_needs_input = >> node.toConst<WebElement>().isTextFormControlElement(); >> } >> > > Done. > > > http://codereview.chromium.org/3474007/diff/26001/27004#newcode5969 > chrome/renderer/render_view.cc:5969: if (webview()) { > On 2010/10/14 22:15:24, darin wrote: > >> nit: no need for the extra brackets here. the existing code only uses >> > brackets > >> when the body is multi-line or when there is an "else" clause. >> > > Done. > > > http://codereview.chromium.org/3474007/diff/26001/27005 > File chrome/renderer/render_view.h (right): > > http://codereview.chromium.org/3474007/diff/26001/27005#newcode878 > chrome/renderer/render_view.h:878: void OnScrollFocusedNodeIntoView(); > On 2010/10/14 22:15:24, darin wrote: > >> this method should be listed in the same order as the IPCs listed in >> render_messages_internal.h >> > > The complete list seems to be out of order (for instance, OnZoom should > have appeared before OnUpdateWebPref according to your sorting order). > Yes, I can believe that things are currently not in an ideal state. That doesn't mean that we shouldn't make improvements. > In general, I would not recommend formatting a file using a strategy > dependent on another file since its hard to keep sync. I would suggest a > simple alphabetical ordering and that reordering of all functions here > and in the .cc file be done in a separate CL. Hmm... I see what you mean, but this is sort of like a subclass overriding methods from a base class, only the base class in this case is the list of IPCs in render_messages_internal.h. If we were to sort purely on an alpha- betical basis, then we might lose some of the functional grouping that exists today. My preferred sorting is really about preserving those functional groups. You definitely don't have to fix all of the sorting in one shot in this CL. > > > http://codereview.chromium.org/3474007/show >
http://codereview.chromium.org/3474007/diff/36001/37004 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3474007/diff/36001/37004#newcode2221 chrome/renderer/render_view.cc:2221: Send(new ViewHostMsg_FocusedNodeChanged(routing_id_, element_needs_input)); looking at this some more, i think "element_needs_input" is not a very accurate description of this field. since FocusedNodeChanged is just a generic IPC, I think it would be best to make this parameter just be descriptive of the focused node. How about "is_text_field" or "is_text_input_field"?
On 2010/10/20 08:13:42, darin wrote: > On Thu, Oct 14, 2010 at 4:12 PM, <mailto:varunjain@chromium.org> wrote: > > > > > http://codereview.chromium.org/3474007/diff/26001/27001 > > File chrome/browser/renderer_host/render_view_host.cc (right): > > > > http://codereview.chromium.org/3474007/diff/26001/27001#newcode1841 > > chrome/browser/renderer_host/render_view_host.cc:1841: Send(new > > ViewMsg_ScrollFocusedNodeIntoView(routing_id())); > > On 2010/10/14 22:15:24, darin wrote: > > > >> why do you want to route this over IPC? i mean, why not encode this > >> > > logic > > > >> directly in RenderView when you observe focus change? > >> > > > > focus change does not push the focused node out of view. It is the > > change in the view size cause by the browser (in this case, to display > > the on screen keyboard) that might do it. Hence, what part of the render > > view is visible can only be determined after the browser has done its > > thing. Then we need to scroll the focused node to make it visible. > > > I see... it was a bit hard to evaluate this patch given that key elements > are > absent. Just looking at the code you have presented, there doesn't seem > to be any reason to have this IPC. But, now I think I understand a bit > better. > > > > > > > > > > > what if the focused node changes by the time you process this IPC here > >> > > in the > > > >> browser process? > >> > > > > That could end up scrolling the wrong node into view... but in that > > case, there will be another focusChanged IPC sent from the renderer and > > everything will come to order in processing the second message. > > > Isn't there still a race condition though? Imagine if focus is given to a > text > field, and then programmatically focus is given to some non text field. In > that case, couldn't you end up scrolling the view to something that is not > a text field? Maybe that doesn't matter, but it seems to be your intent to > only scroll the view to the focused text field. > yes... so there will be race condition, but as you mentioned, it would not matter. The message is a generic one... it says "ScrollFocused*Node*IntoView" and not "ScrollFocused*Textfield*IntoView". The expectation is that whatever node is focused last (regardless of whether its a text node or not) will be scrolled to view. > > > > > > > > > > > how much code do you expect to be adding here per these TODOs? note: > >> > > we should > > > >> be very careful about dumping code into Render{Widget,View}* as there > >> > > is already > > > >> a lot going on in these files. if you are going to be adding a lot of > >> > > code, we > > > >> should consider extracting it into a helper class that can be bolted > >> > > on. > > > > perhaps Bryan can answer this question better... AFAIK, most of the code > > is already in helper classes and code needed here should not be more > > than ~5 lines. > > > > > > http://codereview.chromium.org/3474007/diff/26001/27003 > > File chrome/common/render_messages_internal.h (right): > > > > http://codereview.chromium.org/3474007/diff/26001/27003#newcode79 > > chrome/common/render_messages_internal.h:79: > > IPC_MESSAGE_ROUTED0(ViewMsg_ScrollFocusedNodeIntoView) > > On 2010/10/14 22:15:24, darin wrote: > > > >> how about putting this message nearby other messages that have > >> > > something to do > > > >> with focus? > >> > > > > i know this file may seem like a massive dumping group (and it is), > >> > > but we > > > >> should try to keep some kind of organization. as is, this chnage > >> > > makes the > > > >> situation worse. > >> > > > > I am fine with moving this message to anywhere. I looked around briefly > > before adding it and didnot find any other related message to group it > > with and hence I added it at the top. > > What I can do is put it with messages that set focus to a particular > > node (thats the only one I could find that is slightly related). If you > > have a better place in mind please let me know. > > > That sounds like an improvement, thanks! > > > > > > > > http://codereview.chromium.org/3474007/diff/26001/27004 > > File chrome/renderer/render_view.cc (right): > > > > http://codereview.chromium.org/3474007/diff/26001/27004#newcode2214 > > chrome/renderer/render_view.cc:2214: const WebElement* element = > > static_cast<const WebElement*>(&node); > > On 2010/10/14 22:15:24, darin wrote: > > > >> please use WebNode::toConst<T>() for this. > >> > > > > const WebElement element = node.toConst<WebElement>(); > >> element_needs_input = element.isTextFormControlElement(); > >> > > > > Done. > > > > > > http://codereview.chromium.org/3474007/diff/26001/27004#newcode2217 > > chrome/renderer/render_view.cc:2217: if (node.isContentEditable()) { > > On 2010/10/14 22:15:24, darin wrote: > > > >> it seems like you should evaluate this branch first since it overrides > >> > > the > > > >> previous branch. > >> > > > > if (node.isContentEditable()) { > >> element_needs_input = true; > >> } else if (node.isElementNode()) { > >> element_needs_input = > >> node.toConst<WebElement>().isTextFormControlElement(); > >> } > >> > > > > Done. > > > > > > http://codereview.chromium.org/3474007/diff/26001/27004#newcode5969 > > chrome/renderer/render_view.cc:5969: if (webview()) { > > On 2010/10/14 22:15:24, darin wrote: > > > >> nit: no need for the extra brackets here. the existing code only uses > >> > > brackets > > > >> when the body is multi-line or when there is an "else" clause. > >> > > > > Done. > > > > > > http://codereview.chromium.org/3474007/diff/26001/27005 > > File chrome/renderer/render_view.h (right): > > > > http://codereview.chromium.org/3474007/diff/26001/27005#newcode878 > > chrome/renderer/render_view.h:878: void OnScrollFocusedNodeIntoView(); > > On 2010/10/14 22:15:24, darin wrote: > > > >> this method should be listed in the same order as the IPCs listed in > >> render_messages_internal.h > >> > > > > The complete list seems to be out of order (for instance, OnZoom should > > have appeared before OnUpdateWebPref according to your sorting order). > > > > Yes, I can believe that things are currently not in an ideal state. That > doesn't > mean that we shouldn't make improvements. > > > > > In general, I would not recommend formatting a file using a strategy > > dependent on another file since its hard to keep sync. I would suggest a > > simple alphabetical ordering and that reordering of all functions here > > and in the .cc file be done in a separate CL. > > > Hmm... I see what you mean, but this is sort of like a subclass overriding > methods from a base class, only the base class in this case is the list of > IPCs in render_messages_internal.h. If we were to sort purely on an alpha- > betical basis, then we might lose some of the functional grouping that > exists > today. My preferred sorting is really about preserving those functional > groups. > > You definitely don't have to fix all of the sorting in one shot in this CL. Done. > > > > > > > > > http://codereview.chromium.org/3474007/show > > >
http://codereview.chromium.org/3474007/diff/36001/37004 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3474007/diff/36001/37004#newcode2221 chrome/renderer/render_view.cc:2221: Send(new ViewHostMsg_FocusedNodeChanged(routing_id_, element_needs_input)); On 2010/10/20 08:15:05, darin wrote: > looking at this some more, i think "element_needs_input" is not a very accurate > description of this field. since FocusedNodeChanged is just a generic IPC, I > think it would be best to make this parameter just be descriptive of the focused > node. How about "is_text_field" or "is_text_input_field"? Done.
On Wed, Oct 20, 2010 at 9:21 AM, <varunjain@chromium.org> wrote: > On 2010/10/20 08:13:42, darin wrote: > > On Thu, Oct 14, 2010 at 4:12 PM, <mailto:varunjain@chromium.org> wrote: >> > > > >> > http://codereview.chromium.org/3474007/diff/26001/27001 >> > File chrome/browser/renderer_host/render_view_host.cc (right): >> > >> > http://codereview.chromium.org/3474007/diff/26001/27001#newcode1841 >> > chrome/browser/renderer_host/render_view_host.cc:1841: Send(new >> > ViewMsg_ScrollFocusedNodeIntoView(routing_id())); >> > On 2010/10/14 22:15:24, darin wrote: >> > >> >> why do you want to route this over IPC? i mean, why not encode this >> >> >> > logic >> > >> >> directly in RenderView when you observe focus change? >> >> >> > >> > focus change does not push the focused node out of view. It is the >> > change in the view size cause by the browser (in this case, to display >> > the on screen keyboard) that might do it. Hence, what part of the render >> > view is visible can only be determined after the browser has done its >> > thing. Then we need to scroll the focused node to make it visible. >> > > > I see... it was a bit hard to evaluate this patch given that key elements >> are >> absent. Just looking at the code you have presented, there doesn't seem >> to be any reason to have this IPC. But, now I think I understand a bit >> better. >> > > > > > >> > >> > >> > what if the focused node changes by the time you process this IPC here >> >> >> > in the >> > >> >> browser process? >> >> >> > >> > That could end up scrolling the wrong node into view... but in that >> > case, there will be another focusChanged IPC sent from the renderer and >> > everything will come to order in processing the second message. >> > > > Isn't there still a race condition though? Imagine if focus is given to a >> text >> field, and then programmatically focus is given to some non text field. >> In >> that case, couldn't you end up scrolling the view to something that is not >> a text field? Maybe that doesn't matter, but it seems to be your intent >> to >> only scroll the view to the focused text field. >> > > > yes... so there will be race condition, but as you mentioned, it would not > matter. The message is a generic one... it says > "ScrollFocused*Node*IntoView" > and not "ScrollFocused*Textfield*IntoView". The expectation is that > whatever > node is focused last (regardless of whether its a text node or not) will be > scrolled to view. > > > My point was that the page would scroll to some seemingly random scroll position. This seems undesirable from the point of view of the user, no? It seems like you should only attempt to scroll "the focused node" into view if it is still the focused node. It will be complicated to try to track a node from renderer to browser, and so I would try to avoid that. This is why I asked if there was a way to just move some of the smarts down into the renderer, so that you could make the "scroll focused node into view" decision synchronously in response to a node being focused. -Darin > > > > > > >> > >> > >> > how much code do you expect to be adding here per these TODOs? note: >> >> >> > we should >> > >> >> be very careful about dumping code into Render{Widget,View}* as there >> >> >> > is already >> > >> >> a lot going on in these files. if you are going to be adding a lot of >> >> >> > code, we >> > >> >> should consider extracting it into a helper class that can be bolted >> >> >> > on. >> > >> > perhaps Bryan can answer this question better... AFAIK, most of the code >> > is already in helper classes and code needed here should not be more >> > than ~5 lines. >> > >> > >> > http://codereview.chromium.org/3474007/diff/26001/27003 >> > File chrome/common/render_messages_internal.h (right): >> > >> > http://codereview.chromium.org/3474007/diff/26001/27003#newcode79 >> > chrome/common/render_messages_internal.h:79: >> > IPC_MESSAGE_ROUTED0(ViewMsg_ScrollFocusedNodeIntoView) >> > On 2010/10/14 22:15:24, darin wrote: >> > >> >> how about putting this message nearby other messages that have >> >> >> > something to do >> > >> >> with focus? >> >> >> > >> > i know this file may seem like a massive dumping group (and it is), >> >> >> > but we >> > >> >> should try to keep some kind of organization. as is, this chnage >> >> >> > makes the >> > >> >> situation worse. >> >> >> > >> > I am fine with moving this message to anywhere. I looked around briefly >> > before adding it and didnot find any other related message to group it >> > with and hence I added it at the top. >> > What I can do is put it with messages that set focus to a particular >> > node (thats the only one I could find that is slightly related). If you >> > have a better place in mind please let me know. >> > > > That sounds like an improvement, thanks! >> > > > > >> > >> > http://codereview.chromium.org/3474007/diff/26001/27004 >> >> > File chrome/renderer/render_view.cc (right): >> > >> > http://codereview.chromium.org/3474007/diff/26001/27004#newcode2214 >> > chrome/renderer/render_view.cc:2214: const WebElement* element = >> > static_cast<const WebElement*>(&node); >> > On 2010/10/14 22:15:24, darin wrote: >> > >> >> please use WebNode::toConst<T>() for this. >> >> >> > >> > const WebElement element = node.toConst<WebElement>(); >> >> element_needs_input = element.isTextFormControlElement(); >> >> >> > >> > Done. >> > >> > >> > http://codereview.chromium.org/3474007/diff/26001/27004#newcode2217 >> > chrome/renderer/render_view.cc:2217: if (node.isContentEditable()) { >> > On 2010/10/14 22:15:24, darin wrote: >> > >> >> it seems like you should evaluate this branch first since it overrides >> >> >> > the >> > >> >> previous branch. >> >> >> > >> > if (node.isContentEditable()) { >> >> element_needs_input = true; >> >> } else if (node.isElementNode()) { >> >> element_needs_input = >> >> node.toConst<WebElement>().isTextFormControlElement(); >> >> } >> >> >> > >> > Done. >> > >> > >> > http://codereview.chromium.org/3474007/diff/26001/27004#newcode5969 >> > chrome/renderer/render_view.cc:5969: if (webview()) { >> > On 2010/10/14 22:15:24, darin wrote: >> > >> >> nit: no need for the extra brackets here. the existing code only uses >> >> >> > brackets >> > >> >> when the body is multi-line or when there is an "else" clause. >> >> >> > >> > Done. >> > >> > >> > http://codereview.chromium.org/3474007/diff/26001/27005 >> > File chrome/renderer/render_view.h (right): >> > >> > http://codereview.chromium.org/3474007/diff/26001/27005#newcode878 >> > chrome/renderer/render_view.h:878: void OnScrollFocusedNodeIntoView(); >> > On 2010/10/14 22:15:24, darin wrote: >> > >> >> this method should be listed in the same order as the IPCs listed in >> >> render_messages_internal.h >> >> >> > >> > The complete list seems to be out of order (for instance, OnZoom should >> > have appeared before OnUpdateWebPref according to your sorting order). >> > >> > > Yes, I can believe that things are currently not in an ideal state. That >> doesn't >> mean that we shouldn't make improvements. >> > > > > > In general, I would not recommend formatting a file using a strategy >> > dependent on another file since its hard to keep sync. I would suggest a >> > simple alphabetical ordering and that reordering of all functions here >> > and in the .cc file be done in a separate CL. >> > > > Hmm... I see what you mean, but this is sort of like a subclass overriding >> methods from a base class, only the base class in this case is the list of >> IPCs in render_messages_internal.h. If we were to sort purely on an >> alpha- >> betical basis, then we might lose some of the functional grouping that >> exists >> today. My preferred sorting is really about preserving those functional >> groups. >> > > You definitely don't have to fix all of the sorting in one shot in this >> CL. >> > > Done. > > > > > > > > >> > >> > http://codereview.chromium.org/3474007/show >> > >> > > > > > http://codereview.chromium.org/3474007/show >
On 2010/10/20 16:55:19, darin wrote: > On Wed, Oct 20, 2010 at 9:21 AM, <mailto:varunjain@chromium.org> wrote: > > > On 2010/10/20 08:13:42, darin wrote: > > > > On Thu, Oct 14, 2010 at 4:12 PM, <mailto:varunjain@chromium.org> wrote: > >> > > > > > > >> > http://codereview.chromium.org/3474007/diff/26001/27001 > >> > File chrome/browser/renderer_host/render_view_host.cc (right): > >> > > >> > http://codereview.chromium.org/3474007/diff/26001/27001#newcode1841 > >> > chrome/browser/renderer_host/render_view_host.cc:1841: Send(new > >> > ViewMsg_ScrollFocusedNodeIntoView(routing_id())); > >> > On 2010/10/14 22:15:24, darin wrote: > >> > > >> >> why do you want to route this over IPC? i mean, why not encode this > >> >> > >> > logic > >> > > >> >> directly in RenderView when you observe focus change? > >> >> > >> > > >> > focus change does not push the focused node out of view. It is the > >> > change in the view size cause by the browser (in this case, to display > >> > the on screen keyboard) that might do it. Hence, what part of the render > >> > view is visible can only be determined after the browser has done its > >> > thing. Then we need to scroll the focused node to make it visible. > >> > > > > > > I see... it was a bit hard to evaluate this patch given that key elements > >> are > >> absent. Just looking at the code you have presented, there doesn't seem > >> to be any reason to have this IPC. But, now I think I understand a bit > >> better. > >> > > > > > > > > > > >> > > >> > > >> > what if the focused node changes by the time you process this IPC here > >> >> > >> > in the > >> > > >> >> browser process? > >> >> > >> > > >> > That could end up scrolling the wrong node into view... but in that > >> > case, there will be another focusChanged IPC sent from the renderer and > >> > everything will come to order in processing the second message. > >> > > > > > > Isn't there still a race condition though? Imagine if focus is given to a > >> text > >> field, and then programmatically focus is given to some non text field. > >> In > >> that case, couldn't you end up scrolling the view to something that is not > >> a text field? Maybe that doesn't matter, but it seems to be your intent > >> to > >> only scroll the view to the focused text field. > >> > > > > > > yes... so there will be race condition, but as you mentioned, it would not > > matter. The message is a generic one... it says > > "ScrollFocused*Node*IntoView" > > and not "ScrollFocused*Textfield*IntoView". The expectation is that > > whatever > > node is focused last (regardless of whether its a text node or not) will be > > scrolled to view. > > > > > > > My point was that the page would scroll to some seemingly random scroll > position. > This seems undesirable from the point of view of the user, no? > > It seems like you should only attempt to scroll "the focused node" into view > if it is > still the focused node. It will be complicated to try to track a node from > renderer > to browser, and so I would try to avoid that. > > This is why I asked if there was a way to just move some of the smarts down > into > the renderer, so that you could make the "scroll focused node into view" > decision > synchronously in response to a node being focused. > > -Darin > As discussed with Darin on offline chat, the current code will set up a race condition as follows: 1. user focuses a textfield 2. renderer sends focus event to the browser 3. focus is programatically changed to another meaningless node on the webpage (for example by javascript). 4. browser sends "scrollFocusedNodeIntoView" message to the renderer 5. the renderer scrolls the currently focused meaningless node into view (the user will feel like the page is scrolling to random positions for no reason) To solve this, as suggested by Darin, when the renderer receives the ScrollFocusedNodeIntoView message, it first checks if a textfield is focused and only then scrolls it into view. Since we would always want to scroll textfields into view, even if the focus programatically changes to another textfield, the scrolling behavior would be justified. > > > > > > > > > > > > > >> > > >> > > >> > how much code do you expect to be adding here per these TODOs? note: > >> >> > >> > we should > >> > > >> >> be very careful about dumping code into Render{Widget,View}* as there > >> >> > >> > is already > >> > > >> >> a lot going on in these files. if you are going to be adding a lot of > >> >> > >> > code, we > >> > > >> >> should consider extracting it into a helper class that can be bolted > >> >> > >> > on. > >> > > >> > perhaps Bryan can answer this question better... AFAIK, most of the code > >> > is already in helper classes and code needed here should not be more > >> > than ~5 lines. > >> > > >> > > >> > http://codereview.chromium.org/3474007/diff/26001/27003 > >> > File chrome/common/render_messages_internal.h (right): > >> > > >> > http://codereview.chromium.org/3474007/diff/26001/27003#newcode79 > >> > chrome/common/render_messages_internal.h:79: > >> > IPC_MESSAGE_ROUTED0(ViewMsg_ScrollFocusedNodeIntoView) > >> > On 2010/10/14 22:15:24, darin wrote: > >> > > >> >> how about putting this message nearby other messages that have > >> >> > >> > something to do > >> > > >> >> with focus? > >> >> > >> > > >> > i know this file may seem like a massive dumping group (and it is), > >> >> > >> > but we > >> > > >> >> should try to keep some kind of organization. as is, this chnage > >> >> > >> > makes the > >> > > >> >> situation worse. > >> >> > >> > > >> > I am fine with moving this message to anywhere. I looked around briefly > >> > before adding it and didnot find any other related message to group it > >> > with and hence I added it at the top. > >> > What I can do is put it with messages that set focus to a particular > >> > node (thats the only one I could find that is slightly related). If you > >> > have a better place in mind please let me know. > >> > > > > > > That sounds like an improvement, thanks! > >> > > > > > > > > >> > > >> > http://codereview.chromium.org/3474007/diff/26001/27004 > >> > >> > File chrome/renderer/render_view.cc (right): > >> > > >> > http://codereview.chromium.org/3474007/diff/26001/27004#newcode2214 > >> > chrome/renderer/render_view.cc:2214: const WebElement* element = > >> > static_cast<const WebElement*>(&node); > >> > On 2010/10/14 22:15:24, darin wrote: > >> > > >> >> please use WebNode::toConst<T>() for this. > >> >> > >> > > >> > const WebElement element = node.toConst<WebElement>(); > >> >> element_needs_input = element.isTextFormControlElement(); > >> >> > >> > > >> > Done. > >> > > >> > > >> > http://codereview.chromium.org/3474007/diff/26001/27004#newcode2217 > >> > chrome/renderer/render_view.cc:2217: if (node.isContentEditable()) { > >> > On 2010/10/14 22:15:24, darin wrote: > >> > > >> >> it seems like you should evaluate this branch first since it overrides > >> >> > >> > the > >> > > >> >> previous branch. > >> >> > >> > > >> > if (node.isContentEditable()) { > >> >> element_needs_input = true; > >> >> } else if (node.isElementNode()) { > >> >> element_needs_input = > >> >> node.toConst<WebElement>().isTextFormControlElement(); > >> >> } > >> >> > >> > > >> > Done. > >> > > >> > > >> > http://codereview.chromium.org/3474007/diff/26001/27004#newcode5969 > >> > chrome/renderer/render_view.cc:5969: if (webview()) { > >> > On 2010/10/14 22:15:24, darin wrote: > >> > > >> >> nit: no need for the extra brackets here. the existing code only uses > >> >> > >> > brackets > >> > > >> >> when the body is multi-line or when there is an "else" clause. > >> >> > >> > > >> > Done. > >> > > >> > > >> > http://codereview.chromium.org/3474007/diff/26001/27005 > >> > File chrome/renderer/render_view.h (right): > >> > > >> > http://codereview.chromium.org/3474007/diff/26001/27005#newcode878 > >> > chrome/renderer/render_view.h:878: void OnScrollFocusedNodeIntoView(); > >> > On 2010/10/14 22:15:24, darin wrote: > >> > > >> >> this method should be listed in the same order as the IPCs listed in > >> >> render_messages_internal.h > >> >> > >> > > >> > The complete list seems to be out of order (for instance, OnZoom should > >> > have appeared before OnUpdateWebPref according to your sorting order). > >> > > >> > > > > Yes, I can believe that things are currently not in an ideal state. That > >> doesn't > >> mean that we shouldn't make improvements. > >> > > > > > > > > > In general, I would not recommend formatting a file using a strategy > >> > dependent on another file since its hard to keep sync. I would suggest a > >> > simple alphabetical ordering and that reordering of all functions here > >> > and in the .cc file be done in a separate CL. > >> > > > > > > Hmm... I see what you mean, but this is sort of like a subclass overriding > >> methods from a base class, only the base class in this case is the list of > >> IPCs in render_messages_internal.h. If we were to sort purely on an > >> alpha- > >> betical basis, then we might lose some of the functional grouping that > >> exists > >> today. My preferred sorting is really about preserving those functional > >> groups. > >> > > > > You definitely don't have to fix all of the sorting in one shot in this > >> CL. > >> > > > > Done. > > > > > > > > > > > > > > > > >> > > >> > http://codereview.chromium.org/3474007/show > >> > > >> > > > > > > > > > > http://codereview.chromium.org/3474007/show > > >
http://codereview.chromium.org/3474007/diff/56001/57003 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/3474007/diff/56001/57003#newcode235 chrome/common/render_messages_internal.h:235: IPC_MESSAGE_ROUTED0(ViewMsg_ScrollFocusedTextNodeIntoView) nit: it occurs to me that using the term "text node" here could be confusing. in the DOM world, a text node is not an element. it is a type of node in the DOM that represents a string of text. saying "text input node" would be somewhat clearer, but then that suggests an <input> element. perhaps "editable" would be a better description: ScrollFocusedEditableNodeIntoView Alternatively, I think the term "text field" works since "field" suggests something editable: ScrollFocusedTextFieldIntoView Another choice is to add a bool parameter to the message: ScrollFocusedNodeIntoView(bool if_editable) http://codereview.chromium.org/3474007/diff/56001/57003#newcode1301 chrome/common/render_messages_internal.h:1301: IPC_MESSAGE_ROUTED1(ViewHostMsg_FocusedNodeChanged, bool) nit: please additionally document the bool parameter with a /* is_text_input_node */ comment. http://codereview.chromium.org/3474007/diff/56001/57004 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3474007/diff/56001/57004#newcode412 chrome/renderer/render_view.cc:412: static bool IsTextInputNode(const WebNode& node) { As I commented on the IPC, saying "text input node" is a bit misleading. It suggests a test for a <input type=text> node. IsEditableNode would be better. http://codereview.chromium.org/3474007/diff/56001/57004#newcode1324 chrome/renderer/render_view.cc:1324: if (webview()) { style nit: consider returning early to reduce overall indentation (as is the prevailing style in this file and elsewhere in chromium code). http://codereview.chromium.org/3474007/diff/56001/57004#newcode1331 chrome/renderer/render_view.cc:1331: webview()->scrollFocusedNodeIntoView(); It seems like you should go and modify the WebKit API to either be WebView::scrollNodeIntoView(const WebNode&) or WebElement::scrollIntoViewIfNeeded(). Having a WebKit API that is so explicitly about just scrolling the focused node into view seems suboptimal and it is not exactly what you need here.
http://codereview.chromium.org/3474007/diff/56001/57003 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/3474007/diff/56001/57003#newcode235 chrome/common/render_messages_internal.h:235: IPC_MESSAGE_ROUTED0(ViewMsg_ScrollFocusedTextNodeIntoView) On 2010/10/20 22:45:41, darin wrote: > nit: it occurs to me that using the term "text node" here could be confusing. > in the DOM world, a text node is not an element. it is a type of node in the > DOM that represents a string of text. saying "text input node" would be > somewhat clearer, but then that suggests an <input> element. > > perhaps "editable" would be a better description: > ScrollFocusedEditableNodeIntoView > > Alternatively, I think the term "text field" works since "field" suggests > something editable: > ScrollFocusedTextFieldIntoView > > Another choice is to add a bool parameter to the message: > ScrollFocusedNodeIntoView(bool if_editable) Done. http://codereview.chromium.org/3474007/diff/56001/57003#newcode1301 chrome/common/render_messages_internal.h:1301: IPC_MESSAGE_ROUTED1(ViewHostMsg_FocusedNodeChanged, bool) On 2010/10/20 22:45:41, darin wrote: > nit: please additionally document the bool parameter with a /* > is_text_input_node */ comment. Done. http://codereview.chromium.org/3474007/diff/56001/57004 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3474007/diff/56001/57004#newcode412 chrome/renderer/render_view.cc:412: static bool IsTextInputNode(const WebNode& node) { On 2010/10/20 22:45:41, darin wrote: > As I commented on the IPC, saying "text input node" is a bit misleading. It > suggests a test for a <input type=text> node. > > IsEditableNode would be better. Done. http://codereview.chromium.org/3474007/diff/56001/57004#newcode1324 chrome/renderer/render_view.cc:1324: if (webview()) { On 2010/10/20 22:45:41, darin wrote: > style nit: consider returning early to reduce overall indentation (as is the > prevailing style in this file and elsewhere in chromium code). Done. http://codereview.chromium.org/3474007/diff/56001/57004#newcode1331 chrome/renderer/render_view.cc:1331: webview()->scrollFocusedNodeIntoView(); On 2010/10/20 22:45:41, darin wrote: > It seems like you should go and modify the WebKit API to either be > WebView::scrollNodeIntoView(const WebNode&) or > WebElement::scrollIntoViewIfNeeded(). > > Having a WebKit API that is so explicitly about just scrolling the focused node > into view seems suboptimal and it is not exactly what you need here. Added TODO as discussed offline.
LGTM w/ nits: http://codereview.chromium.org/3474007/diff/62001/63001 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/3474007/diff/62001/63001#newcode1834 chrome/browser/renderer_host/render_view_host.cc:1834: void RenderViewHost::OnMsgFocusedNodeChanged(bool is_text_input_node) { is_editable_node http://codereview.chromium.org/3474007/diff/62001/63002 File chrome/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/3474007/diff/62001/63002#newcode515 chrome/browser/renderer_host/render_view_host.h:515: virtual void OnMsgFocusedNodeChanged(bool is_text_input_node); is_editable_node http://codereview.chromium.org/3474007/diff/62001/63003 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/3474007/diff/62001/63003#newcode1302 chrome/common/render_messages_internal.h:1302: bool /* is_text_input_node */) i'm going to be annoying and suggest that you change all instances of is_text_input_node to is_editable_node just to be consistent. http://codereview.chromium.org/3474007/diff/62001/63004 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3474007/diff/62001/63004#newcode413 chrome/renderer/render_view.cc:413: bool is_text_input_node = false; is_editable_node
http://codereview.chromium.org/3474007/diff/62001/63001 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/3474007/diff/62001/63001#newcode1834 chrome/browser/renderer_host/render_view_host.cc:1834: void RenderViewHost::OnMsgFocusedNodeChanged(bool is_text_input_node) { On 2010/10/25 05:40:57, darin wrote: > is_editable_node Done. http://codereview.chromium.org/3474007/diff/62001/63002 File chrome/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/3474007/diff/62001/63002#newcode515 chrome/browser/renderer_host/render_view_host.h:515: virtual void OnMsgFocusedNodeChanged(bool is_text_input_node); On 2010/10/25 05:40:57, darin wrote: > is_editable_node Done. http://codereview.chromium.org/3474007/diff/62001/63003 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/3474007/diff/62001/63003#newcode1302 chrome/common/render_messages_internal.h:1302: bool /* is_text_input_node */) On 2010/10/25 05:40:57, darin wrote: > i'm going to be annoying and suggest that you change all instances of > is_text_input_node to is_editable_node just to be consistent. Done. http://codereview.chromium.org/3474007/diff/62001/63004 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3474007/diff/62001/63004#newcode413 chrome/renderer/render_view.cc:413: bool is_text_input_node = false; On 2010/10/25 05:40:57, darin wrote: > is_editable_node Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
