Because of missing selectedRange() implementation following test doesn't pass: editing/selection/mixed-editability-10.html
Tests which uses textInputController.selectedRange: editing/selection/5825350-1.html editing/selection/5825350-2.html editing/selection/extend-by-sentence-002.html editing/selection/mixed-editability-10.html editing/selection/move-left-right.html platform/mac/editing/input/text-input-controller.html
Created attachment 170588 [details] patch
Comment on attachment 170588 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=170588&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3334 > + WebCore::ExceptionCode ec; Shouldn't we handle error case?
Comment on attachment 170588 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=170588&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3314 > +bool WebPage::selectedRangeForTesting(int* start, int* length) Is there a reason for not just moving WebPage::getSelectedRange() from WebPageMac.mm to here?
(In reply to comment #4) > (From update of attachment 170588 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170588&action=review > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3314 > > +bool WebPage::selectedRangeForTesting(int* start, int* length) > > Is there a reason for not just moving WebPage::getSelectedRange() from WebPageMac.mm to here? I'm afraid of NSRange usage in Mac port. There failure is marked as location = NSNotFound
(In reply to comment #5) > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3314 > > > +bool WebPage::selectedRangeForTesting(int* start, int* length) > > > > Is there a reason for not just moving WebPage::getSelectedRange() from WebPageMac.mm to here? > > I'm afraid of NSRange usage in Mac port. There failure is marked as location = NSNotFound There's only one user for this function: WKView selectedRange so assuming you modified getSelectedRange to return a boolean like your new function and not set NSNotFound, wouldn't something like this work? diff --git a/Source/WebKit2/UIProcess/API/mac/WKView.mm b/Source/WebKit2/UIProcess/API/mac/WKView.mm index 0f221c6..d0fda59 100644 --- a/Source/WebKit2/UIProcess/API/mac/WKView.mm +++ b/Source/WebKit2/UIProcess/API/mac/WKView.mm @@ -1427 +1427,2 @@ static const short kIOHIDEventTypeScroll = 6; - _data->_page->getSelectedRange(selectionStart, selectionLength); + if (!_data->_page->getSelectedRange(selectionStart, selectionLength)) + selectionStart = NSNotFound;
(In reply to comment #6) > (In reply to comment #5) > > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3314 > > > > +bool WebPage::selectedRangeForTesting(int* start, int* length) > > > > > > Is there a reason for not just moving WebPage::getSelectedRange() from WebPageMac.mm to here? > > > > I'm afraid of NSRange usage in Mac port. There failure is marked as location = NSNotFound > > There's only one user for this function: > WKView selectedRange > so assuming you modified getSelectedRange to return a boolean like your new function and not set NSNotFound, wouldn't something like this work? > > diff --git a/Source/WebKit2/UIProcess/API/mac/WKView.mm b/Source/WebKit2/UIProcess/API/mac/WKView.mm > index 0f221c6..d0fda59 100644 > --- a/Source/WebKit2/UIProcess/API/mac/WKView.mm > +++ b/Source/WebKit2/UIProcess/API/mac/WKView.mm > @@ -1427 +1427,2 @@ static const short kIOHIDEventTypeScroll = 6; > - _data->_page->getSelectedRange(selectionStart, selectionLength); > + if (!_data->_page->getSelectedRange(selectionStart, selectionLength)) > + selectionStart = NSNotFound; Ok, I'll try this. Will see what Mac developers say.
Created attachment 170910 [details] use getSelectedRange from WebPage's Mac port
Comment on attachment 170910 [details] use getSelectedRange from WebPage's Mac port Attachment 170910 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14551020
Comment on attachment 170910 [details] use getSelectedRange from WebPage's Mac port View in context: https://bugs.webkit.org/attachment.cgi?id=170910&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3346 > + if (range && TextIterator::getLocationAndLengthFromRange(frame->selection()->rootEditableElementOrDocumentElement(), range.get(), locationSize, lengthSize)) { > + location = static_cast<uint64_t>(locationSize); > + length = static_cast<uint64_t>(lengthSize); > + } Need to return false if the test here is not true, to reproduce the original mac version behaviour.
Comment on attachment 170910 [details] use getSelectedRange from WebPage's Mac port View in context: https://bugs.webkit.org/attachment.cgi?id=170910&action=review > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:70 > +WK_EXPORT bool WKBundlePageSelectedRange(WKBundlePageRef pageRef, uint64_t& location, uint64_t& length); I'm not sure other C-WK-API methods return a bool for getters. Maybe making location = -1 makes more sense? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3334 > +{ location = WTF::notFound ?
Created attachment 171187 [details] Applied Kenneth's comments
(In reply to comment #12) > Created an attachment (id=171187) [details] > Applied Kenneth's comments Looks good to me at least.
(In reply to comment #13) > (In reply to comment #12) > > Created an attachment (id=171187) [details] [details] > > Applied Kenneth's comments > > Looks good to me at least. I'm wondering if all these functions should be exported to API. If mac is testing such api in the future maybe all ports will do same. See https://bugs.webkit.org/show_bug.cgi?id=100219 comments.
Created attachment 176500 [details] Rebase and extend function to UIProcess
Adding chromium and qt reviewers since this change affects all ports.
Comment on attachment 176500 [details] Rebase and extend function to UIProcess View in context: https://bugs.webkit.org/attachment.cgi?id=176500&action=review > Source/WebKit2/ChangeLog:8 > + Implementation of getting selected range from webpage. IMO, it would be good if you say this patch moves the implementation of mac port to common place for all ports.
(In reply to comment #16) > Adding chromium and qt reviewers since this change affects all ports. This doesn't affect Chromium since Chromium doesn't use WebKit2 or WTR.
Created attachment 178994 [details] Rebased, added comment about moving mac implementation to common part
Comment on attachment 178994 [details] Rebased, added comment about moving mac implementation to common part View in context: https://bugs.webkit.org/attachment.cgi?id=178994&action=review > Source/WebKit2/UIProcess/API/mac/WKView.mm:1431 > NSRange result = NSMakeRange(selectionStart, selectionLength); > - if (result.location == NSNotFound) > - LOG(TextInput, "selectedRange -> (NSNotFound, %u)", result.length); > + if (result.location == WTF::notFound) > + LOG(TextInput, "selectedRange -> (notFound, %u)", result.length); Don't we need to pass in NSNotFound to NSMakeRange?
(In reply to comment #20) > (From update of attachment 178994 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178994&action=review > > > Source/WebKit2/UIProcess/API/mac/WKView.mm:1431 > > NSRange result = NSMakeRange(selectionStart, selectionLength); > > - if (result.location == NSNotFound) > > - LOG(TextInput, "selectedRange -> (NSNotFound, %u)", result.length); > > + if (result.location == WTF::notFound) > > + LOG(TextInput, "selectedRange -> (notFound, %u)", result.length); > > Don't we need to pass in NSNotFound to NSMakeRange? In my opinion: no. NSMakeRange just fills structure with passed arguments. Because of common implementation getSelectedRange returns WTF::notFound instead of NSNotFound. I suppose you are asking about following change: ...getSelectedRange(selectionStart, selectionLength); +if (selectionStart == WTF::notFound) + selectionStart = NSNotFound; if (result.location == NSNotFound) LOG(TextInput, "selectedRange -> (NSNotFound, %u)", result.length);
Comment on attachment 178994 [details] Rebased, added comment about moving mac implementation to common part Looks good to me in terms of sharing port implementation. But, I think mac port reviewer or WK2 reviewer needs to have a final review.
(In reply to comment #22) > (From update of attachment 178994 [details]) > Looks good to me in terms of sharing port implementation. But, I think mac port reviewer or WK2 reviewer needs to have a final review. If there is no comment or objection by next week, I'd like to land this patch. And, happy new year !!
Comment on attachment 178994 [details] Rebased, added comment about moving mac implementation to common part LGTM. But, someone might wanna have a final look.
Comment on attachment 178994 [details] Rebased, added comment about moving mac implementation to common part This codifies a sync message to the WebProcess, something we need to eliminate and which is a huge no-no, in API. r-. Please find another way to do
(In reply to comment #25) > (From update of attachment 178994 [details]) > This codifies a sync message to the WebProcess, something we need to eliminate and which is a huge no-no, in API. r-. Please find another way to do Code is only shared between all ports and was moved from existing mac's implementation.
(In reply to comment #25) > > (From update of attachment 178994 [details] [details]) > > This codifies a sync message to the WebProcess, something we need to eliminate and which is a huge no-no, in API. r-. Please find another way to do Sam, thank you for your review. But, we don't understand what is your point well. I also thought this patch is just share mac implementation with other ports. Could you let us know what something we missed in more detail ?
The Mac port does a bad thing here, it does a synchronous call to the WebProcess. This is something that should be fixed, not extended to all the other ports. By moving it into shared code and adding API, it makes it much harder to fix.
(In reply to comment #28) > The Mac port does a bad thing here, it does a synchronous call to the WebProcess. This is something that should be fixed, not extended to all the other ports. By moving it into shared code and adding API, it makes it much harder to fix. Are you proposing to make it using callbacks? What about changing API in mac? Who is using selectedRange there?
Created attachment 191781 [details] Rebased, make asynchronous
Comment on attachment 191781 [details] Rebased, make asynchronous View in context: https://bugs.webkit.org/attachment.cgi?id=191781&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:1014 > +void WebPageProxy::getSelectedRange(PassRefPtr<DataCallback> prpCallback) what is a prp callback? pass ref ptr... no, please don't name like this!
Benjamin, can you look at this?
Comment on attachment 191781 [details] Rebased, make asynchronous View in context: https://bugs.webkit.org/attachment.cgi?id=191781&action=review >> Source/WebKit2/UIProcess/WebPageProxy.cpp:1014 >> +void WebPageProxy::getSelectedRange(PassRefPtr<DataCallback> prpCallback) > > what is a prp callback? pass ref ptr... no, please don't name like this! All callbacks are named in this file in this manner.
Comment on attachment 191781 [details] Rebased, make asynchronous Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.