RESOLVED FIXED 96508
[Chromium] Merge moveSelectionStart, moveSelectionEnd, and moveCaret into selectRange
https://bugs.webkit.org/show_bug.cgi?id=96508
Summary [Chromium] Merge moveSelectionStart, moveSelectionEnd, and moveCaret into sel...
Iain Merrick
Reported 2012-09-12 06:53:12 PDT
[chromium] WebViewImpl::selectionRange should return start/end on Android
Attachments
husky@chromium.org (1.98 KB, patch)
2012-09-12 06:57 PDT, Iain Merrick
no flags
husky@chromium.org (1.98 KB, patch)
2012-09-12 07:09 PDT, Iain Merrick
no flags
husky@chromium.org (24.01 KB, patch)
2012-09-17 10:31 PDT, Iain Merrick
no flags
husky@chromium.org (24.25 KB, patch)
2012-09-18 06:14 PDT, Iain Merrick
no flags
husky@chromium.org (24.23 KB, patch)
2012-09-18 09:37 PDT, Iain Merrick
no flags
Iain Merrick
Comment 1 2012-09-12 06:57:13 PDT
Iain Merrick
Comment 2 2012-09-12 07:06:11 PDT
There's an unfortunate conflict between https://bugs.webkit.org/show_bug.cgi?id=66973 (WebViewImpl::selectionBounds) and my recent patch https://bugs.webkit.org/show_bug.cgi?id=93998 (WebFrameImpl::selectRange). 66973 makes selectionBounds return base/extent rather than start/end. But on Android we do want start/end, in order to draw the selection handles correctly. Why do we need base/extent on other platforms? 93998 interacts badly because it tries to handle base/extent correctly on Android: "extent" is always the selection handle currently being moved, "base" is the stationary one. This means isBaseFirst() can flip back and forth as the user adjusts the selection -- that's correct, but it means the values returned by selectionBounds will flip back and forth too. I've uploaded a patch that just skips the isBaseFirst check on OS(ANDROID). That obviously isn't ideal. Varun, is there anything else we could do here? Have I just taken the wrong approach in 93998?
Iain Merrick
Comment 3 2012-09-12 07:09:27 PDT
Ryosuke Niwa
Comment 4 2012-09-12 08:54:39 PDT
Comment on attachment 163621 [details] husky@chromium.org View in context: https://bugs.webkit.org/attachment.cgi?id=163621&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:2266 > +// "Start" always precedes "end". "Base" is where the user's selection gesture > +// started, "end" is where it finished. Most platforms want base/extent here. > +// Android wants start/end even if base/extent are reversed. > +// FIXME: This shouldn't be OS-specific. http://webkit.org/b/96508 This is very confusing. Why are arguments called start and end, and yet they expect it to be base and extent? Furthermore, they aren't base and extent. They're anchor and focus. The terminology here is all messed up. Please fix them. Also, we should probably add a new enum like ShouldUseAnchorAndFocus with values UseAnchorAndFocus and UseStartAndEnd instead of hard-coding if-endif like this.
Iain Merrick
Comment 5 2012-09-12 08:59:02 PDT
Comment on attachment 163621 [details] husky@chromium.org View in context: https://bugs.webkit.org/attachment.cgi?id=163621&action=review >> Source/WebKit/chromium/src/WebViewImpl.cpp:2266 >> +// FIXME: This shouldn't be OS-specific. http://webkit.org/b/96508 > > This is very confusing. Why are arguments called start and end, and yet they expect it to be base and extent? Furthermore, they aren't base and extent. They're anchor and focus. > The terminology here is all messed up. Please fix them. > > Also, we should probably add a new enum like ShouldUseAnchorAndFocus with values UseAnchorAndFocus and UseStartAndEnd instead of hard-coding if-endif like this. I could just remove this swizzle completely, instead of #ifdef'ing it, but I assume there was some good reason for doing it in the first place. Any idea what that might be? Do you think I could just go ahead and delete these 4 lines?
Iain Merrick
Comment 6 2012-09-12 09:01:19 PDT
I would also appreciate any comments you have on https://bugs.webkit.org/show_bug.cgi?id=93998, which was submitted in your absence.
Iain Merrick
Comment 7 2012-09-13 08:41:27 PDT
Just had a chat offline about this with olilan (who wrote our text selection code for Android). There's a similar start/end swizzle in our Java code as well. I think we could use *either* start/end or base/extent, we just need to be much clearer about which it is, and make sure we do the same thing consistently in all the APIs. The smallest change would be to use base/extent, as that's what most of the code already does (despite parameter names to the contrary). That means: WebFrameImpl::selectRange() should take base and extent parameters WebViewImpl::selectionBounds() should return base and extent My new moveSelectionStart / moveSelection end methods wouldn't be needed. I can go ahead and make a patch for that. Ryosuke, does that sound like a reasonable approach? Also, can you explain the distinction between base/extent and anchor/focus? It looks to me like those are just the WebKit and HTML names for the same concepts, but maybe there's an important difference.
Ryosuke Niwa
Comment 8 2012-09-13 11:01:47 PDT
(In reply to comment #7) > I think we could use *either* start/end or base/extent, we just need to be much clearer about which it is, and make sure we do the same thing consistently in all the APIs. Yes. > The smallest change would be to use base/extent, as that's what most of the code already does (despite parameter names to the contrary). That means: > > WebFrameImpl::selectRange() should take base and extent parameters > WebViewImpl::selectionBounds() should return base and extent Okay but from the code, it appears that you're using anchor/focus instead. > My new moveSelectionStart / moveSelection end methods wouldn't be needed. > > I can go ahead and make a patch for that. Ryosuke, does that sound like a reasonable approach? Sure. I'm happy as long as there isn't obnoxious if-endif depending on Android. > Also, can you explain the distinction between base/extent and anchor/focus? It looks to me like those are just the WebKit and HTML names for the same concepts, but maybe there's an important difference. Base/extent are different from anchor/focus when the user double or triple clicks on a page and creates a selection with granularity other than character. In such case, base/extent are at where the user clicked whereas anchor/focus will be extended to where the selection ends are.
Iain Merrick
Comment 9 2012-09-17 10:31:31 PDT
WebKit Review Bot
Comment 10 2012-09-17 10:34:19 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Ryosuke Niwa
Comment 11 2012-09-17 11:03:03 PDT
Comment on attachment 164414 [details] husky@chromium.org View in context: https://bugs.webkit.org/attachment.cgi?id=164414&action=review > Source/WebKit/chromium/src/WebFrameImpl.cpp:1477 > + TextGranularity granularity = frameSelection->isNone() ? CharacterGranularity : frameSelection->granularity(); This doesn't look right. Why do we want to select a word just because a word was selected elsewhere before?
Ryosuke Niwa
Comment 12 2012-09-17 13:59:32 PDT
Comment on attachment 164414 [details] husky@chromium.org View in context: https://bugs.webkit.org/attachment.cgi?id=164414&action=review > Source/WebKit/chromium/ChangeLog:4 We also need to rename the bug as these patches do much more than renaming argument variables.
Iain Merrick
Comment 13 2012-09-18 05:50:32 PDT
Comment on attachment 164414 [details] husky@chromium.org View in context: https://bugs.webkit.org/attachment.cgi?id=164414&action=review >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1477 >> + TextGranularity granularity = frameSelection->isNone() ? CharacterGranularity : frameSelection->granularity(); > > This doesn't look right. Why do we want to select a word just because a word was selected elsewhere before? Hmm, that felt like the right way to handle an existing selection, but you're right, there isn't really a use case for this and it isn't being tested. Will just use character granularity.
Iain Merrick
Comment 14 2012-09-18 06:14:58 PDT
Ryosuke Niwa
Comment 15 2012-09-18 09:32:23 PDT
Comment on attachment 164541 [details] husky@chromium.org View in context: https://bugs.webkit.org/attachment.cgi?id=164541&action=review > Source/WebKit/chromium/ChangeLog:3 > + [chromium] Fix parameter names selectionBounds and selectRange, remove moveSelectionStart/End/Caret I've just updated the bug title to be something more appropriate. Please update this line.
Ryosuke Niwa
Comment 16 2012-09-18 09:32:46 PDT
Comment on attachment 164541 [details] husky@chromium.org Ugh... I mean cq-.
Iain Merrick
Comment 17 2012-09-18 09:36:41 PDT
Comment on attachment 164541 [details] husky@chromium.org View in context: https://bugs.webkit.org/attachment.cgi?id=164541&action=review >> Source/WebKit/chromium/ChangeLog:3 >> + [chromium] Fix parameter names selectionBounds and selectRange, remove moveSelectionStart/End/Caret > > I've just updated the bug title to be something more appropriate. Please update this line. Done
Iain Merrick
Comment 18 2012-09-18 09:37:53 PDT
Iain Merrick
Comment 19 2012-09-18 09:40:46 PDT
Comment on attachment 164579 [details] husky@chromium.org Updated ChangeLog headline and "reviewed by" only
WebKit Review Bot
Comment 20 2012-09-18 10:23:14 PDT
Comment on attachment 164579 [details] husky@chromium.org Clearing flags on attachment: 164579 Committed r128903: <http://trac.webkit.org/changeset/128903>
WebKit Review Bot
Comment 21 2012-09-18 10:23:18 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.