Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7570001:
Implement touch selection for RWHVV. (Closed)
|
Created:
9 years, 4 months ago by varunjain Modified:
9 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, jam, darin-cc_chromium.org, brettw-cc_chromium.org Visibility:
Public. |
DescriptionImplement touch selection for RWHVV.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95698
Patch Set 1 #
Total comments: 14
Patch Set 2 : modified according to comments #Patch Set 3 : minor change #
Total comments: 5
Patch Set 4 : modified according to comments #
Total comments: 12
Patch Set 5 : modified according to comments #Patch Set 6 : synced and modified according to comments #
Total comments: 6
Patch Set 7 : modified according to comments #Patch Set 8 : sync #Messages
Total messages: 25 (0 generated)
http://codereview.chromium.org/7570001/diff/1/content/browser/renderer_host/r... File content/browser/renderer_host/render_widget_host_view.h (right): http://codereview.chromium.org/7570001/diff/1/content/browser/renderer_host/r... content/browser/renderer_host/render_widget_host_view.h:182: // Notifies the View that the renderer text selection has changed. Document start/end, especially what coordinate system the points are in. http://codereview.chromium.org/7570001/diff/1/content/common/view_messages.h File content/common/view_messages.h (right): http://codereview.chromium.org/7570001/diff/1/content/common/view_messages.h#... content/common/view_messages.h:1713: gfx::Point, gfx::Point /* visual selection range */) Put each arg on its own line. http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:988: webview()->focusedFrame()->selectRange(WebPoint(start.x(), start.y()), What happens if this doesn't result in a selection change? http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:1527: if (!handling_input_event_ && !selection_ack_pending_) Can the selection change be initiated on the webkit side so that this wouldn't send the change up to the browser? http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:1551: WebKit::WebPoint start, end; I'm not familiar enough with the webkit side to say if this is right. Make sure you get someone that knows webview to review this. http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:1565: p1, p2)); nit: indent one more space.
http://codereview.chromium.org/7570001/diff/1/content/browser/renderer_host/r... File content/browser/renderer_host/render_widget_host_view.h (right): http://codereview.chromium.org/7570001/diff/1/content/browser/renderer_host/r... content/browser/renderer_host/render_widget_host_view.h:182: // Notifies the View that the renderer text selection has changed. On 2011/08/03 17:38:29, sky wrote: > Document start/end, especially what coordinate system the points are in. Done. http://codereview.chromium.org/7570001/diff/1/content/common/view_messages.h File content/common/view_messages.h (right): http://codereview.chromium.org/7570001/diff/1/content/common/view_messages.h#... content/common/view_messages.h:1713: gfx::Point, gfx::Point /* visual selection range */) On 2011/08/03 17:38:29, sky wrote: > Put each arg on its own line. Done. http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:988: webview()->focusedFrame()->selectRange(WebPoint(start.x(), start.y()), On 2011/08/03 17:38:29, sky wrote: > What happens if this doesn't result in a selection change? I think selectRange() knows whether selection has changed or not. But its current definition does not return that value. I will send a webkit CL to change that and then change selection_ack_pending to set from selectRange(). I checked and the current implementation always changes selection... so this should not cause any problems till I checkin the webkit CL and change this. http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:1527: if (!handling_input_event_ && !selection_ack_pending_) On 2011/08/03 17:38:29, sky wrote: > Can the selection change be initiated on the webkit side so that this wouldn't > send the change up to the browser? As I understand, any selection change (whether originating in webkit or not) on the frame by definition call the WebViewClient::didChangeSelection. So the browser will be notified regardless. If its not then that would be a webkit bug that would need to be resolved then. http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:1551: WebKit::WebPoint start, end; On 2011/08/03 17:38:29, sky wrote: > I'm not familiar enough with the webkit side to say if this is right. Make sure > you get someone that knows webview to review this. added darin http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:1565: p1, p2)); On 2011/08/03 17:38:29, sky wrote: > nit: indent one more space. intent is to indent 4 spaces.. not align with the open brace.
http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:988: webview()->focusedFrame()->selectRange(WebPoint(start.x(), start.y()), On 2011/08/03 17:38:29, sky wrote: > What happens if this doesn't result in a selection change? nevermind my previous comment. I realized I can simply reset selection_ack_pending after selectRange regardless of whether selection changed or not.
http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7570001/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:1527: if (!handling_input_event_ && !selection_ack_pending_) On 2011/08/03 18:38:41, varunjain wrote: > On 2011/08/03 17:38:29, sky wrote: > > Can the selection change be initiated on the webkit side so that this wouldn't > > send the change up to the browser? > > As I understand, any selection change (whether originating in webkit or not) on > the frame by definition call the WebViewClient::didChangeSelection. So the > browser will be notified regardless. If its not then that would be a webkit bug > that would need to be resolved then. With this patch any change originating from webkit won't result in calling to the renderer. Is that intentional?
LGTM Varun clarified we only care about this message when the browser initiated the change. -Scott
http://codereview.chromium.org/7570001/diff/6001/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7570001/diff/6001/content/renderer/render_view... content/renderer/render_view.cc:988: webview()->focusedFrame()->selectRange(WebPoint(start.x(), start.y()), gfx::Point can be automagically converted to WebPoint. you don't need to explicitly construct WebPoint objects. more importantly though... this usage of selectRange seems inherently racy. the DOM could be changing asynchronously, such that the incoming 'start' and 'end' coordinates are meaningless. whatever you are trying to do here will be buggy, which makes me want to step back and look for alternative solutions.
http://codereview.chromium.org/7570001/diff/6001/chrome/browser/renderer_host... File chrome/browser/renderer_host/render_widget_host_view_views.cc (right): http://codereview.chromium.org/7570001/diff/6001/chrome/browser/renderer_host... chrome/browser/renderer_host/render_widget_host_view_views.cc:341: host_->Send(new ViewMsg_SelectRect(host_->routing_id(), start, end)); this IPC doesn't make sense. the DOM may have changed out from underneath you, leaving these 'start' and 'end' coordinates to be non-sense.
http://codereview.chromium.org/7570001/diff/6001/chrome/browser/renderer_host... File chrome/browser/renderer_host/render_widget_host_view_views.cc (right): http://codereview.chromium.org/7570001/diff/6001/chrome/browser/renderer_host... chrome/browser/renderer_host/render_widget_host_view_views.cc:341: host_->Send(new ViewMsg_SelectRect(host_->routing_id(), start, end)); On 2011/08/04 16:34:57, darin wrote: > this IPC doesn't make sense. the DOM may have changed out from underneath you, > leaving these 'start' and 'end' coordinates to be non-sense. how does it matter if the DOM has changed? all this IPC is doing is telling the renderer to selection whatever lies between those coordinates. http://codereview.chromium.org/7570001/diff/6001/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7570001/diff/6001/content/renderer/render_view... content/renderer/render_view.cc:988: webview()->focusedFrame()->selectRange(WebPoint(start.x(), start.y()), On 2011/08/04 16:33:43, darin wrote: > gfx::Point can be automagically converted to WebPoint. you don't need to > explicitly construct WebPoint objects. > ok will fix that. > more importantly though... this usage of selectRange seems inherently racy. the > DOM could be changing asynchronously, such that the incoming 'start' and 'end' > coordinates are meaningless. whatever you are trying to do here will be buggy, > which makes me want to step back and look for alternative solutions. the purpose of selectRect IPC is not to ensure what is being selected. This IPC is sent when the user places the touch selection handles at certain location on the screen. The IPC simply tells webkit to select between those end points regardless of whether the DOM is changing. If the DOM changes, the user will adjust the selection handles appropriately.
http://codereview.chromium.org/7570001/diff/6001/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7570001/diff/6001/content/renderer/render_view... content/renderer/render_view.cc:988: webview()->focusedFrame()->selectRange(WebPoint(start.x(), start.y()), On 2011/08/04 16:33:43, darin wrote: > gfx::Point can be automagically converted to WebPoint. you don't need to > explicitly construct WebPoint objects. Done. > > more importantly though... this usage of selectRange seems inherently racy. the > DOM could be changing asynchronously, such that the incoming 'start' and 'end' > coordinates are meaningless. whatever you are trying to do here will be buggy, > which makes me want to step back and look for alternative solutions.
On Thu, Aug 4, 2011 at 9:54 AM, <varunjain@chromium.org> wrote: > > http://codereview.chromium.**org/7570001/diff/6001/chrome/** > browser/renderer_host/render_**widget_host_view_views.cc<http://codereview.chromium.org/7570001/diff/6001/chrome/browser/renderer_host/render_widget_host_view_views.cc> > File chrome/browser/renderer_host/**render_widget_host_view_views.**cc > (right): > > http://codereview.chromium.**org/7570001/diff/6001/chrome/** > browser/renderer_host/render_**widget_host_view_views.cc#**newcode341<http://codereview.chromium.org/7570001/diff/6001/chrome/browser/renderer_host/render_widget_host_view_views.cc#newcode341> > chrome/browser/renderer_host/**render_widget_host_view_views.**cc:341: > host_->Send(new ViewMsg_SelectRect(host_->**routing_id(), start, end)); > On 2011/08/04 16:34:57, darin wrote: > >> this IPC doesn't make sense. the DOM may have changed out from >> > underneath you, > >> leaving these 'start' and 'end' coordinates to be non-sense. >> > > how does it matter if the DOM has changed? all this IPC is doing is > telling the renderer to selection whatever lies between those > coordinates. > > > http://codereview.chromium.**org/7570001/diff/6001/content/** > renderer/render_view.cc<http://codereview.chromium.org/7570001/diff/6001/content/renderer/render_view.cc> > File content/renderer/render_view.**cc (right): > > http://codereview.chromium.**org/7570001/diff/6001/content/** > renderer/render_view.cc#**newcode988<http://codereview.chromium.org/7570001/diff/6001/content/renderer/render_view.cc#newcode988> > content/renderer/render_view.**cc:988: > webview()->focusedFrame()->**selectRange(WebPoint(start.x()**, start.y()), > On 2011/08/04 16:33:43, darin wrote: > >> gfx::Point can be automagically converted to WebPoint. you don't need >> > to > >> explicitly construct WebPoint objects. >> > > > ok will fix that. > > > more importantly though... this usage of selectRange seems inherently >> > racy. the > >> DOM could be changing asynchronously, such that the incoming 'start' >> > and 'end' > >> coordinates are meaningless. whatever you are trying to do here will >> > be buggy, > >> which makes me want to step back and look for alternative solutions. >> > > the purpose of selectRect IPC is not to ensure what is being selected. > This IPC is sent when the user places the touch selection handles at > certain location on the screen. The IPC simply tells webkit to select > between those end points regardless of whether the DOM is changing. If > the DOM changes, the user will adjust the selection handles > appropriately. > > Ah, OK. -Darin > > http://codereview.chromium.**org/7570001/<http://codereview.chromium.org/7570... >
http://codereview.chromium.org/7570001/diff/11001/content/common/view_messages.h File content/common/view_messages.h (right): http://codereview.chromium.org/7570001/diff/11001/content/common/view_message... content/common/view_messages.h:1713: gfx::Point, probably a single gfx::Rect would be better here http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... File content/renderer/render_view.cc (right): http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... content/renderer/render_view.cc:983: void RenderView::OnSelectRect(const gfx::Point& start, const gfx::Point& end) { I wonder... since the IPC is called SelectRect, shouldn't the parameter just be a gfx::Rect? why are you using two points? http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... content/renderer/render_view.cc:1529: selection_ack_pending_ = false; this seems like the wrong name for this member variable. we normally use the "ack" notion for IPC acknowledgements. this is not that. handling_select_rect_ might be a better name. also, it is not clear to me why you want to do this only for OS_POSIX. the SelectRect IPC is not restricted to OS_POSIX. http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... content/renderer/render_view.cc:1556: // TODO(varunjain): remove this check once that is fixed. instead of adding this hack here, wouldn't it be better to write a webkit patch first? http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... content/renderer/render_view.cc:1559: gfx::Point scroll_offset = GetScrollOffset(); it seems like you are trying to correct for scroll offset. does that imply that WebView::selectionRange() does not return coordinates in the view port space? if so, then your code here is failing to take into account iframes that might be scrolled. it is pretty difficult to adjust for scroll offset from outside of Webkit. maybe we need to revise the selectionRange() API? http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... content/renderer/render_view.cc:1565: p1, p2)); nit: please indent "p1, p2));" so that p1 starts at the same column as routing_id_.
http://codereview.chromium.org/7570001/diff/11001/content/common/view_messages.h File content/common/view_messages.h (right): http://codereview.chromium.org/7570001/diff/11001/content/common/view_message... content/common/view_messages.h:1713: gfx::Point, On 2011/08/04 18:15:57, darin wrote: > probably a single gfx::Rect would be better here As I mentioned in the other comment, selection may not necessarily be a rectangle. What we are interested in is the end points of selected region so we can show the touch selection handles there. http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... File content/renderer/render_view.cc (right): http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... content/renderer/render_view.cc:983: void RenderView::OnSelectRect(const gfx::Point& start, const gfx::Point& end) { On 2011/08/04 18:15:57, darin wrote: > I wonder... since the IPC is called SelectRect, shouldn't the parameter just be > a gfx::Rect? why are you using two points? mostly because the selected region may not be a rectangle (such as most selection on a webpage). the IPC is called SelectRect because of the naming of the function upstream in the call heirarchy. I can change the name of the IPC to SelectRange if this is unacceptable. http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... content/renderer/render_view.cc:1529: selection_ack_pending_ = false; On 2011/08/04 18:15:57, darin wrote: > this seems like the wrong name for this member variable. we normally use the > "ack" notion for IPC acknowledgements. this is not that. handling_select_rect_ > might be a better name. > Done > also, it is not clear to me why you want to do this only for OS_POSIX. the > SelectRect IPC is not restricted to OS_POSIX. the SelectionChanged IPC is only handled for OS_POSIX. For others its a no-op. SelectRect is only sent for touchui http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... content/renderer/render_view.cc:1556: // TODO(varunjain): remove this check once that is fixed. On 2011/08/04 18:15:57, darin wrote: > instead of adding this hack here, wouldn't it be better to write a webkit patch > first? I need to first check with clang team as they were the original authors of the selectionRange() implementation. Perhaps they want to keep it that way. Hence the TODO. http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... content/renderer/render_view.cc:1559: gfx::Point scroll_offset = GetScrollOffset(); On 2011/08/04 18:15:57, darin wrote: > it seems like you are trying to correct for scroll offset. does that imply that > WebView::selectionRange() does not return coordinates in the view port space? > if so, then your code here is failing to take into account iframes that might be > scrolled. it is pretty difficult to adjust for scroll offset from outside of > Webkit. maybe we need to revise the selectionRange() API? selectionRange API may need to be revised. I intended this CL to contain the boiler plate code and basic requirements to get touch selection to work (IPC communication etc.). With this CL, I have text selection on simple webpages working fine. I intend to refine the code to work with complex webpages in subsequent CLs. http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... content/renderer/render_view.cc:1565: p1, p2)); On 2011/08/04 18:15:57, darin wrote: > nit: please indent "p1, p2));" so that p1 starts at the same column as > routing_id_. Done.
On 2011/08/04 19:25:21, varunjain wrote: > http://codereview.chromium.org/7570001/diff/11001/content/common/view_messages.h > File content/common/view_messages.h (right): > > http://codereview.chromium.org/7570001/diff/11001/content/common/view_message... > content/common/view_messages.h:1713: gfx::Point, > On 2011/08/04 18:15:57, darin wrote: > > probably a single gfx::Rect would be better here > > As I mentioned in the other comment, selection may not necessarily be a > rectangle. What we are interested in is the end points of selected region so we > can show the touch selection handles there. OK, I see. Sorry for missing that. > http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... > File content/renderer/render_view.cc (right): > > http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... > content/renderer/render_view.cc:983: void RenderView::OnSelectRect(const > gfx::Point& start, const gfx::Point& end) { > On 2011/08/04 18:15:57, darin wrote: > > I wonder... since the IPC is called SelectRect, shouldn't the parameter just > be > > a gfx::Rect? why are you using two points? > > mostly because the selected region may not be a rectangle (such as most > selection on a webpage). the IPC is called SelectRect because of the naming of > the function upstream in the call heirarchy. I can change the name of the IPC to > SelectRange if this is unacceptable. OK, so if selecting text, the idea is that this would select all rows between the start.y and end.y, right? The start.x and end.x would only impact the first and last row? It might help to document this on the WebKit API. I think that I would definitely rename the IPCs and methods to make this clearer. SelectRect sounds like it is selecting only a rectangular region. SelectRange sounds better. > > also, it is not clear to me why you want to do this only for OS_POSIX. the > > SelectRect IPC is not restricted to OS_POSIX. > > the SelectionChanged IPC is only handled for OS_POSIX. For others its a no-op. > SelectRect is only sent for touchui OK, it just looks a bit odd for only part of the code to be #ifdef'd. > http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... > content/renderer/render_view.cc:1556: // TODO(varunjain): remove this check once > that is fixed. > On 2011/08/04 18:15:57, darin wrote: > > instead of adding this hack here, wouldn't it be better to write a webkit > patch > > first? > > I need to first check with clang team as they were the original authors of the > selectionRange() implementation. Perhaps they want to keep it that way. Hence > the TODO. s/clang/clank/? OK, but please don't let this TODO linger too long.
On 2011/08/04 23:25:15, darin wrote: > On 2011/08/04 19:25:21, varunjain wrote: > > > http://codereview.chromium.org/7570001/diff/11001/content/common/view_messages.h > > File content/common/view_messages.h (right): > > > > > http://codereview.chromium.org/7570001/diff/11001/content/common/view_message... > > content/common/view_messages.h:1713: gfx::Point, > > On 2011/08/04 18:15:57, darin wrote: > > > probably a single gfx::Rect would be better here > > > > As I mentioned in the other comment, selection may not necessarily be a > > rectangle. What we are interested in is the end points of selected region so > we > > can show the touch selection handles there. > > OK, I see. Sorry for missing that. > > > > > http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... > > File content/renderer/render_view.cc (right): > > > > > http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... > > content/renderer/render_view.cc:983: void RenderView::OnSelectRect(const > > gfx::Point& start, const gfx::Point& end) { > > On 2011/08/04 18:15:57, darin wrote: > > > I wonder... since the IPC is called SelectRect, shouldn't the parameter just > > be > > > a gfx::Rect? why are you using two points? > > > > mostly because the selected region may not be a rectangle (such as most > > selection on a webpage). the IPC is called SelectRect because of the naming of > > the function upstream in the call heirarchy. I can change the name of the IPC > to > > SelectRange if this is unacceptable. > > OK, so if selecting text, the idea is that this would select all rows between > the start.y and end.y, right? The start.x and end.x would only impact the first > and last row? It might help to document this on the WebKit API. > > I think that I would definitely rename the IPCs and methods to make this > clearer. SelectRect sounds like it is selecting only a rectangular region. > SelectRange sounds better. Done. > > > > > also, it is not clear to me why you want to do this only for OS_POSIX. the > > > SelectRect IPC is not restricted to OS_POSIX. > > > > the SelectionChanged IPC is only handled for OS_POSIX. For others its a no-op. > > SelectRect is only sent for touchui > > OK, it just looks a bit odd for only part of the code to be #ifdef'd. > > > > > http://codereview.chromium.org/7570001/diff/11001/content/renderer/render_vie... > > content/renderer/render_view.cc:1556: // TODO(varunjain): remove this check > once > > that is fixed. > > On 2011/08/04 18:15:57, darin wrote: > > > instead of adding this hack here, wouldn't it be better to write a webkit > > patch > > > first? > > > > I need to first check with clang team as they were the original authors of the > > selectionRange() implementation. Perhaps they want to keep it that way. Hence > > the TODO. > > s/clang/clank/? > > OK, but please don't let this TODO linger too long.
just nits, so LGTM with those nits fixed. http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view... content/renderer/render_view.cc:1564: // TODO(varunjain): add other hooks for SelectionChanged. nit: this comment is quite tantalizing and leaves me wondering what "other hooks" you might be referring to. could you elaborate a bit on this? maybe link to a bug? http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h File content/renderer/render_view.h (right): http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view... content/renderer/render_view.h:1201: // Set if we received a ViewMsg_SelectRange from the browser and are waiting nit: this comment can be a bit more brief: // Used to inform didChangeSelection() when it is called in the context // of handling a ViewMsg_SelectRange IPC. http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view... content/renderer/render_view.h:1207: bool handling_select_rect_; nit: handling_select_range_
http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view... content/renderer/render_view.cc:1564: // TODO(varunjain): add other hooks for SelectionChanged. On 2011/08/05 03:51:16, darin wrote: > nit: this comment is quite tantalizing and leaves me wondering what "other > hooks" you might be referring to. could you elaborate a bit on this? maybe > link to a bug? by other hooks, I meant make selection handles work for all cases. For instance, if a render view is scrolled, webkit doesnot call didSelectionChange. So there will be a hook needed to update the selection handles for scrolling. There is no bug associated, but I am incrementally working on this. http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h File content/renderer/render_view.h (right): http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view... content/renderer/render_view.h:1201: // Set if we received a ViewMsg_SelectRange from the browser and are waiting On 2011/08/05 03:51:16, darin wrote: > nit: this comment can be a bit more brief: > > // Used to inform didChangeSelection() when it is called in the context > // of handling a ViewMsg_SelectRange IPC. Done. http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view... content/renderer/render_view.h:1207: bool handling_select_rect_; On 2011/08/05 03:51:16, darin wrote: > nit: handling_select_range_ Done.
On Fri, Aug 5, 2011 at 8:57 AM, <varunjain@chromium.org> wrote: > > http://codereview.chromium.**org/7570001/diff/4018/content/** > renderer/render_view.cc<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc> > File content/renderer/render_view.**cc (right): > > http://codereview.chromium.**org/7570001/diff/4018/content/** > renderer/render_view.cc#**newcode1564<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc#newcode1564> > content/renderer/render_view.**cc:1564: // TODO(varunjain): add other > hooks for SelectionChanged. > On 2011/08/05 03:51:16, darin wrote: > >> nit: this comment is quite tantalizing and leaves me wondering what >> > "other > >> hooks" you might be referring to. could you elaborate a bit on this? >> > maybe > >> link to a bug? >> > > by other hooks, I meant make selection handles work for all cases. For > instance, if a render view is scrolled, webkit doesnot call > didSelectionChange. So there will be a hook needed to update the > selection handles for scrolling. There is no bug associated, but I am > incrementally working on this. That seems to imply that you should have the selection handles rendered by WebKit. That way the selection handles will move seamlessly with the page when it is scrolled. Otherwise, the best case outcome will be selection handles that appear to bounce as the page is scrolled. This CL seems like the wrong approach to this problem. > > > http://codereview.chromium.**org/7570001/diff/4018/content/** > renderer/render_view.h<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h> > File content/renderer/render_view.h (right): > > http://codereview.chromium.**org/7570001/diff/4018/content/** > renderer/render_view.h#**newcode1201<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1201> > content/renderer/render_view.**h:1201: // Set if we received a > ViewMsg_SelectRange from the browser and are waiting > On 2011/08/05 03:51:16, darin wrote: > >> nit: this comment can be a bit more brief: >> > > // Used to inform didChangeSelection() when it is called in the >> > context > >> // of handling a ViewMsg_SelectRange IPC. >> > > Done. > > > http://codereview.chromium.**org/7570001/diff/4018/content/** > renderer/render_view.h#**newcode1207<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1207> > content/renderer/render_view.**h:1207: bool handling_select_rect_; > On 2011/08/05 03:51:16, darin wrote: > >> nit: handling_select_range_ >> > > Done. > > > http://codereview.chromium.**org/7570001/<http://codereview.chromium.org/7570... >
On 2011/08/05 16:15:40, darin wrote: > On Fri, Aug 5, 2011 at 8:57 AM, <mailto:varunjain@chromium.org> wrote: > > > > > http://codereview.chromium.**org/7570001/diff/4018/content/** > > > renderer/render_view.cc<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc> > > File content/renderer/render_view.**cc (right): > > > > http://codereview.chromium.**org/7570001/diff/4018/content/** > > > renderer/render_view.cc#**newcode1564<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc#newcode1564> > > content/renderer/render_view.**cc:1564: // TODO(varunjain): add other > > hooks for SelectionChanged. > > On 2011/08/05 03:51:16, darin wrote: > > > >> nit: this comment is quite tantalizing and leaves me wondering what > >> > > "other > > > >> hooks" you might be referring to. could you elaborate a bit on this? > >> > > maybe > > > >> link to a bug? > >> > > > > by other hooks, I meant make selection handles work for all cases. For > > instance, if a render view is scrolled, webkit doesnot call > > didSelectionChange. So there will be a hook needed to update the > > selection handles for scrolling. There is no bug associated, but I am > > incrementally working on this. > > > That seems to imply that you should have the selection handles rendered by > WebKit. That way the selection handles will move seamlessly with the page > when it is scrolled. Otherwise, the best case outcome will be selection > handles > that appear to bounce as the page is scrolled. This CL seems like the wrong > approach to this problem. > selection handles are not restricted to webpages. They are also shown on views textfields. Hence they are implemented in the browser. The views version of this CL has already been reviewed by sky and submitted... but if you think this requires further discussion please un-check the commit box for my last patch so commit-bot does not commit it. > > > > > > > > http://codereview.chromium.**org/7570001/diff/4018/content/** > > > renderer/render_view.h<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h> > > File content/renderer/render_view.h (right): > > > > http://codereview.chromium.**org/7570001/diff/4018/content/** > > > renderer/render_view.h#**newcode1201<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1201> > > content/renderer/render_view.**h:1201: // Set if we received a > > ViewMsg_SelectRange from the browser and are waiting > > On 2011/08/05 03:51:16, darin wrote: > > > >> nit: this comment can be a bit more brief: > >> > > > > // Used to inform didChangeSelection() when it is called in the > >> > > context > > > >> // of handling a ViewMsg_SelectRange IPC. > >> > > > > Done. > > > > > > http://codereview.chromium.**org/7570001/diff/4018/content/** > > > renderer/render_view.h#**newcode1207<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1207> > > content/renderer/render_view.**h:1207: bool handling_select_rect_; > > On 2011/08/05 03:51:16, darin wrote: > > > >> nit: handling_select_range_ > >> > > > > Done. > > > > > > > http://codereview.chromium.**org/7570001/%3Chttp://codereview.chromium.org/75...> > >
On Fri, Aug 5, 2011 at 9:21 AM, <varunjain@chromium.org> wrote: > On 2011/08/05 16:15:40, darin wrote: > > On Fri, Aug 5, 2011 at 8:57 AM, <mailto:varunjain@chromium.org**> wrote: >> > > > >> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >> > >> > > renderer/render_view.cc<http:/**/codereview.chromium.org/** > 7570001/diff/4018/content/**renderer/render_view.cc<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc> > > > > > File content/renderer/render_view.****cc (right): >> > >> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >> > >> > > renderer/render_view.cc#****newcode1564<http://codereview.** > chromium.org/7570001/diff/**4018/content/renderer/render_** > view.cc#newcode1564<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc#newcode1564> > > > > > content/renderer/render_view.****cc:1564: // TODO(varunjain): add other >> > hooks for SelectionChanged. >> > On 2011/08/05 03:51:16, darin wrote: >> > >> >> nit: this comment is quite tantalizing and leaves me wondering what >> >> >> > "other >> > >> >> hooks" you might be referring to. could you elaborate a bit on this? >> >> >> > maybe >> > >> >> link to a bug? >> >> >> > >> > by other hooks, I meant make selection handles work for all cases. For >> > instance, if a render view is scrolled, webkit doesnot call >> > didSelectionChange. So there will be a hook needed to update the >> > selection handles for scrolling. There is no bug associated, but I am >> > incrementally working on this. >> > > > That seems to imply that you should have the selection handles rendered by >> WebKit. That way the selection handles will move seamlessly with the page >> when it is scrolled. Otherwise, the best case outcome will be selection >> handles >> that appear to bounce as the page is scrolled. This CL seems like the >> wrong >> approach to this problem. >> > > > selection handles are not restricted to webpages. They are also shown on > views > textfields. Hence they are implemented in the browser. The views version of > this > CL has already been reviewed by sky and submitted... but if you think this > requires further discussion please un-check the commit box for my last > patch so > commit-bot does not commit it. > > I don't know which patch you are referring to. I'm just saying that the design for selection handles is not going to work well for web pages if you intend selection handles to remain visible while the page is scrolling. Don't you agree? -Darin > > > > > > >> > >> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >> > >> > > renderer/render_view.h<http://**codereview.chromium.org/** > 7570001/diff/4018/content/**renderer/render_view.h<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h> > > > > > File content/renderer/render_view.h (right): >> > >> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >> > >> > > renderer/render_view.h#****newcode1201<http://codereview.** > chromium.org/7570001/diff/**4018/content/renderer/render_** > view.h#newcode1201<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1201> > > > > > content/renderer/render_view.****h:1201: // Set if we received a >> > ViewMsg_SelectRange from the browser and are waiting >> > On 2011/08/05 03:51:16, darin wrote: >> > >> >> nit: this comment can be a bit more brief: >> >> >> > >> > // Used to inform didChangeSelection() when it is called in the >> >> >> > context >> > >> >> // of handling a ViewMsg_SelectRange IPC. >> >> >> > >> > Done. >> > >> > >> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >> > >> > > renderer/render_view.h#****newcode1207<http://codereview.** > chromium.org/7570001/diff/**4018/content/renderer/render_** > view.h#newcode1207<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1207> > > > > > content/renderer/render_view.****h:1207: bool handling_select_rect_; >> > On 2011/08/05 03:51:16, darin wrote: >> > >> >> nit: handling_select_range_ >> >> >> > >> > Done. >> > >> > >> > >> > > http://codereview.chromium.****org/7570001/%3Chttp://coderevi** > ew.chromium.org/7570001/ <http://codereview.chromium.org/7570001/>> > > > >> > > > > http://codereview.chromium.**org/7570001/<http://codereview.chromium.org/7570... >
On Fri, Aug 5, 2011 at 12:22 PM, Darin Fisher <darin@chromium.org> wrote: > > > On Fri, Aug 5, 2011 at 9:21 AM, <varunjain@chromium.org> wrote: > >> On 2011/08/05 16:15:40, darin wrote: >> >> On Fri, Aug 5, 2011 at 8:57 AM, <mailto:varunjain@chromium.org**> wrote: >>> >> >> > >>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >>> > >>> >> >> renderer/render_view.cc<http:/**/codereview.chromium.org/** >> 7570001/diff/4018/content/**renderer/render_view.cc<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc> >> > >> >> > File content/renderer/render_view.****cc (right): >>> > >>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >>> > >>> >> >> renderer/render_view.cc#****newcode1564<http://codereview.** >> chromium.org/7570001/diff/**4018/content/renderer/render_** >> view.cc#newcode1564<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc#newcode1564> >> > >> >> > content/renderer/render_view.****cc:1564: // TODO(varunjain): add >>> other >>> > hooks for SelectionChanged. >>> > On 2011/08/05 03:51:16, darin wrote: >>> > >>> >> nit: this comment is quite tantalizing and leaves me wondering what >>> >> >>> > "other >>> > >>> >> hooks" you might be referring to. could you elaborate a bit on this? >>> >> >>> > maybe >>> > >>> >> link to a bug? >>> >> >>> > >>> > by other hooks, I meant make selection handles work for all cases. For >>> > instance, if a render view is scrolled, webkit doesnot call >>> > didSelectionChange. So there will be a hook needed to update the >>> > selection handles for scrolling. There is no bug associated, but I am >>> > incrementally working on this. >>> >> >> >> That seems to imply that you should have the selection handles rendered >>> by >>> WebKit. That way the selection handles will move seamlessly with the >>> page >>> when it is scrolled. Otherwise, the best case outcome will be selection >>> handles >>> that appear to bounce as the page is scrolled. This CL seems like the >>> wrong >>> approach to this problem. >>> >> >> >> selection handles are not restricted to webpages. They are also shown on >> views >> textfields. Hence they are implemented in the browser. The views version >> of this >> CL has already been reviewed by sky and submitted... but if you think this >> requires further discussion please un-check the commit box for my last >> patch so >> commit-bot does not commit it. >> >> > I don't know which patch you are referring to. I'm just saying that the > design for > selection handles is not going to work well for web pages if you intend > selection > handles to remain visible while the page is scrolling. Don't you agree? > my reply was more for the question "why not implement selection handles in webkit". But why do you think this will not work if I update the selection handle position accordingly when the page scrolls? > > -Darin > > > >> >> >> >> >> > >>> > >>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >>> > >>> >> >> renderer/render_view.h<http://**codereview.chromium.org/** >> 7570001/diff/4018/content/**renderer/render_view.h<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h> >> > >> >> > File content/renderer/render_view.h (right): >>> > >>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >>> > >>> >> >> renderer/render_view.h#****newcode1201<http://codereview.** >> chromium.org/7570001/diff/**4018/content/renderer/render_** >> view.h#newcode1201<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1201> >> > >> >> > content/renderer/render_view.****h:1201: // Set if we received a >>> > ViewMsg_SelectRange from the browser and are waiting >>> > On 2011/08/05 03:51:16, darin wrote: >>> > >>> >> nit: this comment can be a bit more brief: >>> >> >>> > >>> > // Used to inform didChangeSelection() when it is called in the >>> >> >>> > context >>> > >>> >> // of handling a ViewMsg_SelectRange IPC. >>> >> >>> > >>> > Done. >>> > >>> > >>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >>> > >>> >> >> renderer/render_view.h#****newcode1207<http://codereview.** >> chromium.org/7570001/diff/**4018/content/renderer/render_** >> view.h#newcode1207<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1207> >> > >> >> > content/renderer/render_view.****h:1207: bool handling_select_rect_; >>> > On 2011/08/05 03:51:16, darin wrote: >>> > >>> >> nit: handling_select_range_ >>> >> >>> > >>> > Done. >>> > >>> > >>> > >>> >> >> http://codereview.chromium.****org/7570001/%3Chttp://coderevi** >> ew.chromium.org/7570001/ <http://codereview.chromium.org/7570001/>> >> >> > >>> >> >> >> >> http://codereview.chromium.**org/7570001/<http://codereview.chromium.org/7570... >> > >
On Fri, Aug 5, 2011 at 9:26 AM, Varun Jain <varunjain@google.com> wrote: > > > On Fri, Aug 5, 2011 at 12:22 PM, Darin Fisher <darin@chromium.org> wrote: > >> >> >> On Fri, Aug 5, 2011 at 9:21 AM, <varunjain@chromium.org> wrote: >> >>> On 2011/08/05 16:15:40, darin wrote: >>> >>> On Fri, Aug 5, 2011 at 8:57 AM, <mailto:varunjain@chromium.org**> >>>> wrote: >>>> >>> >>> > >>>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >>>> > >>>> >>> >>> renderer/render_view.cc<http:/**/codereview.chromium.org/** >>> 7570001/diff/4018/content/**renderer/render_view.cc<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc> >>> > >>> >>> > File content/renderer/render_view.****cc (right): >>>> > >>>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >>>> > >>>> >>> >>> renderer/render_view.cc#****newcode1564<http://codereview.** >>> chromium.org/7570001/diff/**4018/content/renderer/render_** >>> view.cc#newcode1564<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc#newcode1564> >>> > >>> >>> > content/renderer/render_view.****cc:1564: // TODO(varunjain): add >>>> other >>>> > hooks for SelectionChanged. >>>> > On 2011/08/05 03:51:16, darin wrote: >>>> > >>>> >> nit: this comment is quite tantalizing and leaves me wondering what >>>> >> >>>> > "other >>>> > >>>> >> hooks" you might be referring to. could you elaborate a bit on this? >>>> >> >>>> > maybe >>>> > >>>> >> link to a bug? >>>> >> >>>> > >>>> > by other hooks, I meant make selection handles work for all cases. For >>>> > instance, if a render view is scrolled, webkit doesnot call >>>> > didSelectionChange. So there will be a hook needed to update the >>>> > selection handles for scrolling. There is no bug associated, but I am >>>> > incrementally working on this. >>>> >>> >>> >>> That seems to imply that you should have the selection handles rendered >>>> by >>>> WebKit. That way the selection handles will move seamlessly with the >>>> page >>>> when it is scrolled. Otherwise, the best case outcome will be selection >>>> handles >>>> that appear to bounce as the page is scrolled. This CL seems like the >>>> wrong >>>> approach to this problem. >>>> >>> >>> >>> selection handles are not restricted to webpages. They are also shown on >>> views >>> textfields. Hence they are implemented in the browser. The views version >>> of this >>> CL has already been reviewed by sky and submitted... but if you think >>> this >>> requires further discussion please un-check the commit box for my last >>> patch so >>> commit-bot does not commit it. >>> >>> >> I don't know which patch you are referring to. I'm just saying that the >> design for >> selection handles is not going to work well for web pages if you intend >> selection >> handles to remain visible while the page is scrolling. Don't you agree? >> > > my reply was more for the question "why not implement selection handles in > webkit". But why do you think this will not work if I update the selection > handle position accordingly when the page scrolls? > because it will bounce around. the page is scrolling asynchronously to the browser UI thread. your selection handles will be playing catch up to the page as it scrolls out from underneath you. -darin > > > >> >> -Darin >> >> >> >>> >>> >>> >>> >>> > >>>> > >>>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >>>> > >>>> >>> >>> renderer/render_view.h<http://**codereview.chromium.org/** >>> 7570001/diff/4018/content/**renderer/render_view.h<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h> >>> > >>> >>> > File content/renderer/render_view.h (right): >>>> > >>>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >>>> > >>>> >>> >>> renderer/render_view.h#****newcode1201<http://codereview.** >>> chromium.org/7570001/diff/**4018/content/renderer/render_** >>> view.h#newcode1201<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1201> >>> > >>> >>> > content/renderer/render_view.****h:1201: // Set if we received a >>>> > ViewMsg_SelectRange from the browser and are waiting >>>> > On 2011/08/05 03:51:16, darin wrote: >>>> > >>>> >> nit: this comment can be a bit more brief: >>>> >> >>>> > >>>> > // Used to inform didChangeSelection() when it is called in the >>>> >> >>>> > context >>>> > >>>> >> // of handling a ViewMsg_SelectRange IPC. >>>> >> >>>> > >>>> > Done. >>>> > >>>> > >>>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** >>>> > >>>> >>> >>> renderer/render_view.h#****newcode1207<http://codereview.** >>> chromium.org/7570001/diff/**4018/content/renderer/render_** >>> view.h#newcode1207<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1207> >>> > >>> >>> > content/renderer/render_view.****h:1207: bool handling_select_rect_; >>>> > On 2011/08/05 03:51:16, darin wrote: >>>> > >>>> >> nit: handling_select_range_ >>>> >> >>>> > >>>> > Done. >>>> > >>>> > >>>> > >>>> >>> >>> http://codereview.chromium.****org/7570001/%3Chttp://coderevi** >>> ew.chromium.org/7570001/ <http://codereview.chromium.org/7570001/>> >>> >>> > >>>> >>> >>> >>> >>> http://codereview.chromium.**org/7570001/<http://codereview.chromium.org/7570... >>> >> >> >
On 2011/08/05 17:00:49, darin wrote: > On Fri, Aug 5, 2011 at 9:26 AM, Varun Jain <mailto:varunjain@google.com> wrote: > > > > > > > On Fri, Aug 5, 2011 at 12:22 PM, Darin Fisher <mailto:darin@chromium.org> wrote: > > > >> > >> > >> On Fri, Aug 5, 2011 at 9:21 AM, <mailto:varunjain@chromium.org> wrote: > >> > >>> On 2011/08/05 16:15:40, darin wrote: > >>> > >>> On Fri, Aug 5, 2011 at 8:57 AM, <mailto:varunjain@chromium.org**> > >>>> wrote: > >>>> > >>> > >>> > > >>>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** > >>>> > > >>>> > >>> > >>> renderer/render_view.cc<http:/**/codereview.chromium.org/** > >>> > 7570001/diff/4018/content/**renderer/render_view.cc<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc> > >>> > > >>> > >>> > File content/renderer/render_view.****cc (right): > >>>> > > >>>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** > >>>> > > >>>> > >>> > >>> renderer/render_view.cc#****newcode1564<http://codereview.** > >>> chromium.org/7570001/diff/**4018/content/renderer/render_** > >>> > view.cc#newcode1564<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc#newcode1564> > >>> > > >>> > >>> > content/renderer/render_view.****cc:1564: // TODO(varunjain): add > >>>> other > >>>> > hooks for SelectionChanged. > >>>> > On 2011/08/05 03:51:16, darin wrote: > >>>> > > >>>> >> nit: this comment is quite tantalizing and leaves me wondering what > >>>> >> > >>>> > "other > >>>> > > >>>> >> hooks" you might be referring to. could you elaborate a bit on this? > >>>> >> > >>>> > maybe > >>>> > > >>>> >> link to a bug? > >>>> >> > >>>> > > >>>> > by other hooks, I meant make selection handles work for all cases. For > >>>> > instance, if a render view is scrolled, webkit doesnot call > >>>> > didSelectionChange. So there will be a hook needed to update the > >>>> > selection handles for scrolling. There is no bug associated, but I am > >>>> > incrementally working on this. > >>>> > >>> > >>> > >>> That seems to imply that you should have the selection handles rendered > >>>> by > >>>> WebKit. That way the selection handles will move seamlessly with the > >>>> page > >>>> when it is scrolled. Otherwise, the best case outcome will be selection > >>>> handles > >>>> that appear to bounce as the page is scrolled. This CL seems like the > >>>> wrong > >>>> approach to this problem. > >>>> > >>> > >>> > >>> selection handles are not restricted to webpages. They are also shown on > >>> views > >>> textfields. Hence they are implemented in the browser. The views version > >>> of this > >>> CL has already been reviewed by sky and submitted... but if you think > >>> this > >>> requires further discussion please un-check the commit box for my last > >>> patch so > >>> commit-bot does not commit it. > >>> > >>> > >> I don't know which patch you are referring to. I'm just saying that the > >> design for > >> selection handles is not going to work well for web pages if you intend > >> selection > >> handles to remain visible while the page is scrolling. Don't you agree? > >> > > > > my reply was more for the question "why not implement selection handles in > > webkit". But why do you think this will not work if I update the selection > > handle position accordingly when the page scrolls? > > > > because it will bounce around. the page is scrolling asynchronously to the > browser UI thread. your selection handles will be playing catch up to the > page as it scrolls out from underneath you. had discussion with Rob about this. We believe that in the context of baklava when the selection handle which is a NativeWidgetViews will be composited on top of the webpage and hence would be less likely to be out of sync, this would not matter. It could still be a problem and in Rob's own words "While we think it would be a non-issue with baklava.. if the problem persists, we will fix it" :) > > -darin > > > > > > > > > > >> > >> -Darin > >> > >> > >> > >>> > >>> > >>> > >>> > >>> > > >>>> > > >>>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** > >>>> > > >>>> > >>> > >>> renderer/render_view.h<http://**codereview.chromium.org/** > >>> > 7570001/diff/4018/content/**renderer/render_view.h<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h> > >>> > > >>> > >>> > File content/renderer/render_view.h (right): > >>>> > > >>>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** > >>>> > > >>>> > >>> > >>> renderer/render_view.h#****newcode1201<http://codereview.** > >>> chromium.org/7570001/diff/**4018/content/renderer/render_** > >>> > view.h#newcode1201<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1201> > >>> > > >>> > >>> > content/renderer/render_view.****h:1201: // Set if we received a > >>>> > ViewMsg_SelectRange from the browser and are waiting > >>>> > On 2011/08/05 03:51:16, darin wrote: > >>>> > > >>>> >> nit: this comment can be a bit more brief: > >>>> >> > >>>> > > >>>> > // Used to inform didChangeSelection() when it is called in the > >>>> >> > >>>> > context > >>>> > > >>>> >> // of handling a ViewMsg_SelectRange IPC. > >>>> >> > >>>> > > >>>> > Done. > >>>> > > >>>> > > >>>> > http://codereview.chromium.****org/7570001/diff/4018/content/**** > >>>> > > >>>> > >>> > >>> renderer/render_view.h#****newcode1207<http://codereview.** > >>> chromium.org/7570001/diff/**4018/content/renderer/render_** > >>> > view.h#newcode1207<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1207> > >>> > > >>> > >>> > content/renderer/render_view.****h:1207: bool handling_select_rect_; > >>>> > On 2011/08/05 03:51:16, darin wrote: > >>>> > > >>>> >> nit: handling_select_range_ > >>>> >> > >>>> > > >>>> > Done. > >>>> > > >>>> > > >>>> > > >>>> > >>> > >>> http://codereview.chromium.****org/7570001/%253Chttp://coderevi** > >>> ew.chromium.org/7570001/ <http://codereview.chromium.org/7570001/>> > >>> > >>> > > >>>> > >>> > >>> > >>> > >>> > http://codereview.chromium.**org/7570001/%3Chttp://codereview.chromium.org/75...> > >>> > >> > >> > >
On Fri, Aug 5, 2011 at 11:57 AM, <varunjain@chromium.org> wrote: > On 2011/08/05 17:00:49, darin wrote: > >> On Fri, Aug 5, 2011 at 9:26 AM, Varun Jain <mailto:varunjain@google.com> >> > wrote: > > > >> > >> > On Fri, Aug 5, 2011 at 12:22 PM, Darin Fisher <mailto: >> darin@chromium.org> >> > wrote: > > > >> >> >> >> >> >> On Fri, Aug 5, 2011 at 9:21 AM, <mailto:varunjain@chromium.org**> >> wrote: >> >> >> >>> On 2011/08/05 16:15:40, darin wrote: >> >>> >> >>> On Fri, Aug 5, 2011 at 8:57 AM, <mailto:varunjain@chromium.org****> >> >>>> wrote: >> >>>> >> >>> >> >>> > >> >>>> > http://codereview.chromium.******org/7570001/diff/4018/** >> content/**** >> >>>> > >> >>>> >> >>> >> >>> renderer/render_view.cc<http:/****/codereview.chromium.org/** >> >>> >> > > 7570001/diff/4018/content/****renderer/render_view.cc<http:/** > /codereview.chromium.org/**7570001/diff/4018/content/** > renderer/render_view.cc<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc> > > > > >>> > >> >>> >> >>> > File content/renderer/render_view.******cc (right): >> >>>> > >> >>>> > http://codereview.chromium.******org/7570001/diff/4018/** >> content/**** >> >>>> > >> >>>> >> >>> >> >>> renderer/render_view.cc#******newcode1564<http://codereview.**** >> >>> chromium.org/7570001/diff/****4018/content/renderer/render_****<http://chromium.org/7570001/diff/**4018/content/renderer/render_**> >> >>> >> > > view.cc#newcode1564<http://**codereview.chromium.org/** > 7570001/diff/4018/content/**renderer/render_view.cc#**newcode1564<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.cc#newcode1564> > > > > >>> > >> >>> >> >>> > content/renderer/render_view.******cc:1564: // TODO(varunjain): >> add >> >>>> other >> >>>> > hooks for SelectionChanged. >> >>>> > On 2011/08/05 03:51:16, darin wrote: >> >>>> > >> >>>> >> nit: this comment is quite tantalizing and leaves me wondering >> what >> >>>> >> >> >>>> > "other >> >>>> > >> >>>> >> hooks" you might be referring to. could you elaborate a bit on >> this? >> >>>> >> >> >>>> > maybe >> >>>> > >> >>>> >> link to a bug? >> >>>> >> >> >>>> > >> >>>> > by other hooks, I meant make selection handles work for all cases. >> For >> >>>> > instance, if a render view is scrolled, webkit doesnot call >> >>>> > didSelectionChange. So there will be a hook needed to update the >> >>>> > selection handles for scrolling. There is no bug associated, but I >> am >> >>>> > incrementally working on this. >> >>>> >> >>> >> >>> >> >>> That seems to imply that you should have the selection handles >> rendered >> >>>> by >> >>>> WebKit. That way the selection handles will move seamlessly with the >> >>>> page >> >>>> when it is scrolled. Otherwise, the best case outcome will be >> selection >> >>>> handles >> >>>> that appear to bounce as the page is scrolled. This CL seems like >> the >> >>>> wrong >> >>>> approach to this problem. >> >>>> >> >>> >> >>> >> >>> selection handles are not restricted to webpages. They are also shown >> on >> >>> views >> >>> textfields. Hence they are implemented in the browser. The views >> version >> >>> of this >> >>> CL has already been reviewed by sky and submitted... but if you think >> >>> this >> >>> requires further discussion please un-check the commit box for my last >> >>> patch so >> >>> commit-bot does not commit it. >> >>> >> >>> >> >> I don't know which patch you are referring to. I'm just saying that >> the >> >> design for >> >> selection handles is not going to work well for web pages if you intend >> >> selection >> >> handles to remain visible while the page is scrolling. Don't you >> agree? >> >> >> > >> > my reply was more for the question "why not implement selection handles >> in >> > webkit". But why do you think this will not work if I update the >> selection >> > handle position accordingly when the page scrolls? >> > >> > > because it will bounce around. the page is scrolling asynchronously to >> the >> browser UI thread. your selection handles will be playing catch up to the >> page as it scrolls out from underneath you. >> > > had discussion with Rob about this. We believe that in the context of > baklava > when the selection handle which is a NativeWidgetViews will be composited > on > top of the webpage and hence would be less likely to be out of sync, this > would > not matter. It could still be a problem and in Rob's own words "While we > think > it would be a non-issue with baklava.. if the problem persists, we will fix > it" > :) > > I don't see how that can make a difference. Baklava simply takes the output from the WebKit compositor, and so Baklava is not aware of any "scrolling" of layers that the WebKit compositor may be doing. I think you are working toward a design that is fundamentally flawed. I'm anticipating that you would need to revert this CL if you cared about having the selection handles move with the page as it is scrolled. -Darin > > > -darin >> > > > > > >> > >> > >> >> >> >> -Darin >> >> >> >> >> >> >> >>> >> >>> >> >>> >> >>> >> >>> > >> >>>> > >> >>>> > http://codereview.chromium.******org/7570001/diff/4018/** >> content/**** >> >>>> > >> >>>> >> >>> >> >>> renderer/render_view.h<http://****codereview.chromium.org/** >> >>> >> > > 7570001/diff/4018/content/****renderer/render_view.h<http://** > codereview.chromium.org/**7570001/diff/4018/content/** > renderer/render_view.h<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h> > > > > >>> > >> >>> >> >>> > File content/renderer/render_view.h (right): >> >>>> > >> >>>> > http://codereview.chromium.******org/7570001/diff/4018/** >> content/**** >> >>>> > >> >>>> >> >>> >> >>> renderer/render_view.h#******newcode1201<http://codereview.**** >> >>> chromium.org/7570001/diff/****4018/content/renderer/render_****<http://chromium.org/7570001/diff/**4018/content/renderer/render_**> >> >>> >> > > view.h#newcode1201<http://**codereview.chromium.org/** > 7570001/diff/4018/content/**renderer/render_view.h#**newcode1201<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1201> > > > > >>> > >> >>> >> >>> > content/renderer/render_view.******h:1201: // Set if we received a >> >>>> > ViewMsg_SelectRange from the browser and are waiting >> >>>> > On 2011/08/05 03:51:16, darin wrote: >> >>>> > >> >>>> >> nit: this comment can be a bit more brief: >> >>>> >> >> >>>> > >> >>>> > // Used to inform didChangeSelection() when it is called in the >> >>>> >> >> >>>> > context >> >>>> > >> >>>> >> // of handling a ViewMsg_SelectRange IPC. >> >>>> >> >> >>>> > >> >>>> > Done. >> >>>> > >> >>>> > >> >>>> > http://codereview.chromium.******org/7570001/diff/4018/** >> content/**** >> >>>> > >> >>>> >> >>> >> >>> renderer/render_view.h#******newcode1207<http://codereview.**** >> >>> chromium.org/7570001/diff/****4018/content/renderer/render_****<http://chromium.org/7570001/diff/**4018/content/renderer/render_**> >> >>> >> > > view.h#newcode1207<http://**codereview.chromium.org/** > 7570001/diff/4018/content/**renderer/render_view.h#**newcode1207<http://codereview.chromium.org/7570001/diff/4018/content/renderer/render_view.h#newcode1207> > > > >> >>> > >> >>> >> >>> > content/renderer/render_view.******h:1207: bool >> handling_select_rect_; >> >>>> > On 2011/08/05 03:51:16, darin wrote: >> >>>> > >> >>>> >> nit: handling_select_range_ >> >>>> >> >> >>>> > >> >>>> > Done. >> >>>> > >> >>>> > >> >>>> > >> >>>> >> >>> >> >>> http://codereview.chromium.******org/7570001/%253Chttp://**coderevi** >> >> >>> ew.chromium.org/7570001/ <http://codereview.chromium.**org/7570001/<http://codereview.chromium.org/7570... >> >> >> >>> >> >>> > >> >>>> >> >>> >> >>> >> >>> >> >>> >> > > http://codereview.chromium.****org/7570001/%3Chttp://coderevi** > ew.chromium.org/7570001/ <http://codereview.chromium.org/7570001/>> > >> >>> >> >> >> >> >> > >> > > > > http://codereview.chromium.**org/7570001/<http://codereview.chromium.org/7570... >
Change committed as 95698 |
