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.
Created attachment 107802 [details] Patch
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).
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.
(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.
Comment on attachment 107802 [details] Patch Attachment 107802 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9735802
These functions are invoked through editing APIs almost directly. Is unit testing necessary here, or should we just make textInputController based tests cross platform?
Created attachment 107803 [details] Fixed Windows build
(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.
Comment on attachment 107803 [details] Fixed Windows build Attachment 107803 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9734848
Any reviewers?
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.
(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.
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.
Created attachment 107893 [details] Fixed GTK build
(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.
Webkit-dev is also fine, but we need to document usage guidelines for exposed rangeFromLocationAndLength and locationAndLengthFromRange in this bug for posterity.
(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.
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.
(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?
(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.
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
(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.
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.
(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
Created attachment 111076 [details] new patch
(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.
Comment on attachment 111076 [details] new patch Attachment 111076 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10074206
Created attachment 111157 [details] Adds internals binding
Comment on attachment 111157 [details] Adds internals binding Yay! Thanks for the review.
Comment on attachment 111157 [details] Adds internals binding Clearing flags on attachment: 111157 Committed r97587: <http://trac.webkit.org/changeset/97587>
All reviewed patches have been landed. Closing bug.
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.
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.
Build fixes have been landed.