RESOLVED FIXED 68330
Expose rangeFromLocationAndLength and locationAndLengthFromRange via internals object
https://bugs.webkit.org/show_bug.cgi?id=68330
Summary Expose rangeFromLocationAndLength and locationAndLengthFromRange via internal...
Ryosuke Niwa
Reported 2011-09-18 20:10:03 PDT
TextIterator is one of the most important class in the editing component yet it's very under-tested as exemplified by the number of security and non-security bugs we had in this class. To mitigate this issue, I'd like to expose rangeFromLocationAndLength and locationAndLengthFromRange via window.internals. This allows us to test common usage of TextIterator throughly (Source/WebKit code only use TextIterator through these two functions) and fuzz-test them.
Attachments
Patch (21.17 KB, patch)
2011-09-18 20:15 PDT, Ryosuke Niwa
no flags
Fixed Windows build (23.04 KB, patch)
2011-09-18 21:31 PDT, Ryosuke Niwa
no flags
Fixed GTK build (24.67 KB, patch)
2011-09-19 11:29 PDT, Ryosuke Niwa
no flags
new patch (11.43 KB, patch)
2011-10-14 14:25 PDT, Ryosuke Niwa
enrica: review+
Adds internals binding (15.17 KB, patch)
2011-10-15 17:33 PDT, Ryosuke Niwa
no flags
Ryosuke Niwa
Comment 1 2011-09-18 20:15:02 PDT
Ryosuke Niwa
Comment 2 2011-09-18 20:17:37 PDT
Comment on attachment 107802 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107802&action=review > LayoutTests/editing/text-iterator/range-to-from-location-and-length-expected.txt:12 > +PASS locationAndLengthFromRange(range(p.childNodes[2], 6, p, 3)) is [11, 1] > +FAIL internals.rangeFromLocationAndLength(document, 11, 2).toArray() should be [object Text],6,[object HTMLDivElement],0. Was [object Text],6,[object HTMLParagraphElement],3. Notice that the the last case contradicts the previous case (locationAndLengthFromRange returns location=11, length=1 for what rangeFromLocationAndLength returned).
Hajime Morrita
Comment 3 2011-09-18 20:23:20 PDT
If you have a plan to expose more TextIterator API to JS, It better have separate TextIterator.idl for that. Well, it's not only for TextIterator though. Maybe we should have NodeInternals, ElementInternals or something.
Ryosuke Niwa
Comment 4 2011-09-18 20:35:40 PDT
(In reply to comment #3) > If you have a plan to expose more TextIterator API to JS, > It better have separate TextIterator.idl for that. I had thought about it but I think that's an overkill. These two functions will probably provide a pretty decent test coverage.
Collabora GTK+ EWS bot
Comment 5 2011-09-18 20:59:53 PDT
Alexey Proskuryakov
Comment 6 2011-09-18 21:31:01 PDT
These functions are invoked through editing APIs almost directly. Is unit testing necessary here, or should we just make textInputController based tests cross platform?
Ryosuke Niwa
Comment 7 2011-09-18 21:31:55 PDT
Created attachment 107803 [details] Fixed Windows build
Ryosuke Niwa
Comment 8 2011-09-18 21:36:49 PDT
(In reply to comment #6) > These functions are invoked through editing APIs almost directly. Is unit testing necessary here, or should we just make textInputController based tests cross platform? True but given the number of and severity of bugs we've had, I'd like to test these functions directly. In my experience, it's quite tricky to get textInputController call these two functions with arbitrary range/location/length.
Collabora GTK+ EWS bot
Comment 9 2011-09-18 22:00:31 PDT
Comment on attachment 107803 [details] Fixed Windows build Attachment 107803 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9734848
Ryosuke Niwa
Comment 10 2011-09-19 10:22:08 PDT
Any reviewers?
Alexey Proskuryakov
Comment 11 2011-09-19 10:38:04 PDT
It might be useful to provide some concrete examples where having an internals method makes a big difference. Also, some guidelines for when it's appropriate to use these would be great. Unit testing doesn't work with frequent refactoring well, so adding unit tests for internal methods should be carefully considered. For instance, what does one do if they want to remove or significantly refactor rangeFromLocationAndLength and locationAndLengthFromRange functions in the future? Most practically, all tests that used these shortcuts will be simply removed.
Ryosuke Niwa
Comment 12 2011-09-19 10:43:11 PDT
(In reply to comment #11) > It might be useful to provide some concrete examples where having an internals method makes a big difference. Also, some guidelines for when it's appropriate to use these would be great. > > Unit testing doesn't work with frequent refactoring well, so adding unit tests for internal methods should be carefully considered. For instance, what does one do if they want to remove or significantly refactor rangeFromLocationAndLength and locationAndLengthFromRange functions in the future? Most practically, all tests that used these shortcuts will be simply removed. I think these two functions have pretty clear interface, and I don't see any reason we should be modifying them in near future. Also TextIterator is exposed as WebTextIterator in mac port so it seems unlikely that we ever get rid of TextIterator.
Alexey Proskuryakov
Comment 13 2011-09-19 10:50:28 PDT
I actually don't see the interface of these functions as very clear - it's more like "do whatever it takes to make OS X input methods happy". WebTextIterator works with DOMRanges, not with locations and lengths, so I don't see how it's relevant here. Anyway, it's not very productive to discuss this kind of costs, let's discuss the benefits. What are the specific cases where having an internals method makes a big difference. Also, some guidelines for when it's appropriate to use these would be great.
Ryosuke Niwa
Comment 14 2011-09-19 11:29:24 PDT
Created attachment 107893 [details] Fixed GTK build
Ryosuke Niwa
Comment 15 2011-09-19 12:07:51 PDT
(In reply to comment #13) > Anyway, it's not very productive to discuss this kind of costs, let's discuss the benefits. What are the specific cases where having an internals method makes a big difference. Also, some guidelines for when it's appropriate to use these would be great. That sounds like a discussion more appropriately done on webkit-dev.
Alexey Proskuryakov
Comment 16 2011-09-19 12:35:09 PDT
Webkit-dev is also fine, but we need to document usage guidelines for exposed rangeFromLocationAndLength and locationAndLengthFromRange in this bug for posterity.
Ryosuke Niwa
Comment 17 2011-09-19 13:08:04 PDT
(In reply to comment #16) > Webkit-dev is also fine, but we need to document usage guidelines for exposed rangeFromLocationAndLength and locationAndLengthFromRange in this bug for posterity. Oh sure. I think internals.rangeFromLocationAndLength and internals.locationFromRange and internals.lengthFromRange should only be used to test those functions and other editing tests where relative locations of text need to be queried.
Ryosuke Niwa
Comment 18 2011-09-21 13:36:30 PDT
Any reviewers?
Darin Adler
Comment 19 2011-09-21 13:57:18 PDT
Comment on attachment 107893 [details] Fixed GTK build View in context: https://bugs.webkit.org/attachment.cgi?id=107893&action=review > Source/WebCore/editing/TextIterator.cpp:2388 > +PassRefPtr<Range> TextIterator::rangeFromLocationAndLength(Frame* frame, int rangeLocation, int rangeLength) > +{ > + Element* selectionRoot = frame->selection()->rootEditableElement(); > + Element* scope = selectionRoot ? selectionRoot : frame->document()->documentElement(); > + > + frame->document()->updateLayoutIgnorePendingStylesheets(); > + > + return rangeFromLocationAndLength(scope, rangeLocation, rangeLength); > +} Exposing these to internals seems OK. But the base for the location and length here seems an unclear concept. The heuristic for this came from WebFrame.mm originally. It was also copied into code for other ports and I’m not sure it was right for them. Now with the code in here, at a level I think might possibly be a bit too low, I suspect the name of the function is not specific enough. Further, the comment with the rationale was left behind in WebFrame.mm.
Ryosuke Niwa
Comment 20 2011-09-21 14:09:38 PDT
(In reply to comment #19) > But the base for the location and length here seems an unclear concept. The heuristic for this came from WebFrame.mm originally. It was also copied into code for other ports and I’m not sure it was right for them. Yeah, I don't quite understand why it's needed. These functions are (or at least appears to be) only used for IME but IME won't show up on non-editable region so I don't understand why we'd need to fallback to document when there's no root editable element. > Now with the code in here, at a level I think might possibly be a bit too low, I suspect the name of the function is not specific enough. Further, the comment with the rationale was left behind in WebFrame.mm. Yeah, I don't like leaving the comment behind either but I think it's better than duplicating the same code in every single port as done today :( Also I really wanted to make interface for rangeFromLocationAndLength and locationAndLengthFromRange symmetric. Do you have suggestions as to how to proceed?
Darin Adler
Comment 21 2011-09-21 14:57:30 PDT
(In reply to comment #20) > Do you have suggestions as to how to proceed? A few thoughts: 1) Change the name of the function. Since it’s not just a map of a range to a location and length within a document, but rather a map of a range to a location and length within an editable region. Maybe there should also be a function that maps a range to the range of the editable region? 2) Consider a different location for the function. I’m not sure this makes sense as a TextIterator function. Generally speaking TextIterator seems a little lower level than editable region boundaries. But I could be wrong on this one. 3) Make sure the comment stays with the function. Move it inside. The name of the function should allude to what the comment says anyway.
Hajime Morrita
Comment 22 2011-09-28 19:25:48 PDT
Comment on attachment 107893 [details] Fixed GTK build FYI: TestWEbKitAPI is coming. I think it's worth considering to locate these internal API test there. http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI
Ryosuke Niwa
Comment 23 2011-09-28 19:28:18 PDT
(In reply to comment #22) > (From update of attachment 107893 [details]) > FYI: TestWEbKitAPI is coming. I think it's worth considering to locate these internal API test there. > http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI But TextIterator requires DOM nodes, Document, Frame, renderers, etc... so I don't think we can unit-test it.
Ryosuke Niwa
Comment 24 2011-10-12 12:24:47 PDT
On my second thought, this automatic determination of the "scope" element is very odd. I think we should modify locationAndLengthFromRange to take "scope" element instead like rangeFromLocationAndLength. Even though this would mean we'll duplicate some code, it'll be a cleaner API.
Enrica Casucci
Comment 25 2011-10-14 10:03:58 PDT
(In reply to comment #24) > On my second thought, this automatic determination of the "scope" element is very odd. I think we should modify locationAndLengthFromRange to take "scope" element instead like rangeFromLocationAndLength. > > Even though this would mean we'll duplicate some code, it'll be a cleaner API. This patch is no longer valid if you land the fix for https://bugs.webkit.org/show_bug.cgi?id=69964
Ryosuke Niwa
Comment 26 2011-10-14 14:25:01 PDT
Created attachment 111076 [details] new patch
Ryosuke Niwa
Comment 27 2011-10-14 14:36:15 PDT
(In reply to comment #26) > Created an attachment (id=111076) [details] > new patch This patch is not going to build on Win/GTK so no review flag.
Collabora GTK+ EWS bot
Comment 28 2011-10-14 23:19:03 PDT
Ryosuke Niwa
Comment 29 2011-10-15 17:33:51 PDT
Created attachment 111157 [details] Adds internals binding
Ryosuke Niwa
Comment 30 2011-10-16 21:50:42 PDT
Comment on attachment 111157 [details] Adds internals binding Yay! Thanks for the review.
WebKit Review Bot
Comment 31 2011-10-16 23:04:08 PDT
Comment on attachment 111157 [details] Adds internals binding Clearing flags on attachment: 111157 Committed r97587: <http://trac.webkit.org/changeset/97587>
WebKit Review Bot
Comment 32 2011-10-16 23:04:16 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 33 2011-10-17 02:05:23 PDT
It broke the GTK build: ./.libs/libWebCoreInternals.a(libWebCoreInternals_la-Internals.o): In function `WebCore::Internals::locationFromRange(WebCore::Element*, WebCore::Range const*, int&)': Internals.cpp:(.text+0x10fc): undefined reference to `WebCore::TextIterator::getLocationAndLengthFromRange(WebCore::Element*, WebCore::Range const*, unsigned int&, unsigned int&)' ./.libs/libWebCoreInternals.a(libWebCoreInternals_la-Internals.o): In function `WebCore::Internals::lengthFromRange(WebCore::Element*, WebCore::Range const*, int&)': Internals.cpp:(.text+0x116c): undefined reference to `WebCore::TextIterator::getLocationAndLengthFromRange(WebCore::Element*, WebCore::Range const*, unsigned int&, unsigned int&)' Reopen to fix the build.
Enrica Casucci
Comment 34 2011-10-17 09:31:47 PDT
Comment on attachment 111076 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=111076&action=review Look good to me. > LayoutTests/editing/text-iterator/range-to-from-location-and-length.html:18 > + testFailed('This test requires internals object'); I prefer the early return.
Ryosuke Niwa
Comment 35 2011-10-17 11:27:57 PDT
Build fixes have been landed.
Note You need to log in before you can comment on or make changes to this bug.