Summary: | [chromium] More Android changes to WebFrameImpl::selectRange | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Iain Merrick <husky> | ||||||
Component: | WebKit API | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, husky, jamesr, rniwa, tkent+wkapi, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 92513, 93108 | ||||||||
Bug Blocks: | 66687, 96508 | ||||||||
Attachments: |
|
Description
Iain Merrick
2012-08-14 10:41:03 PDT
Created attachment 160944 [details]
Patch
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. rniwa is the ideal person to review this patch, but he's on vacation. I did a first pass, and it looked reasonable to me, but I'm not an editing expert. Let me see if I can find someone more expert in this area to take a look. Adam, did you manage to find anyone? Nope. I'll take a look on Tuesday. (Today is a national holiday in the US.) Comment on attachment 160944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160944&action=review I'd really like rniwa to take a look at this patch when he gets back from vacation. Based on the discussion in the previous bug, this patch looks plausible to me, but I'm far from an expert here. > Source/WebKit/chromium/src/WebFrameImpl.cpp:-1482 > - if (frame()->selection()->shouldChangeSelection(newSelection)) Looks like we lost this call. Is this no longer needed? > Source/WebKit/chromium/src/WebFrameImpl.cpp:1486 > + if (selection.isNone()) > + return false; Should this ASSERT? Is it an error to call this function without a current selection? Comment on attachment 160944 [details] Patch Rejecting attachment 160944 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: romium/src/WebFrameImpl.cpp patching file Source/WebKit/chromium/src/WebFrameImpl.h patching file Source/WebKit/chromium/tests/WebFrameTest.cpp Hunk #6 FAILED at 1066. 1 out of 6 hunks FAILED -- saving rejects to file Source/WebKit/chromium/tests/WebFrameTest.cpp.rej patching file Source/WebKit/chromium/tests/data/text_selection.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/13764349 Comment on attachment 160944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160944&action=review >> Source/WebKit/chromium/src/WebFrameImpl.cpp:-1482 >> - if (frame()->selection()->shouldChangeSelection(newSelection)) > > Looks like we lost this call. Is this no longer needed? Oops, good catch. setNonDirectionalSelectionIfNeeded() calls that internally, but setSelection() doesn't. I'll put it back in. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1486 >> + return false; > > Should this ASSERT? Is it an error to call this function without a current selection? I thought it best to return a bool result code, for consistency with the existing selectWordAroundCaret. This is going to be called from way over in Java-land, so it seems good to be defensive about the exact editing state, rather than just asserting that it's correct. For example, a Javascript hook might clear the selection concurrently with the Java UI code. Created attachment 162521 [details]
Patch
Comment on attachment 162521 [details]
Patch
Setting cq+.
Comment on attachment 162521 [details] Patch Clearing flags on attachment: 162521 Committed r127776: <http://trac.webkit.org/changeset/127776> All reviewed patches have been landed. Closing bug. |