Bug 68330 – Expose rangeFromLocationAndLength and locationAndLengthFromRange via internals object

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 107802 [details]
Patch

[Ryosuke Niwa]

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]

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]

(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 on attachment 107802 [details]
Patch

Attachment 107802 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9735802

[Alexey Proskuryakov]

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]

Created attachment 107803 [details]
Fixed Windows build

[Ryosuke Niwa]

(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 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]

Any reviewers?

[Alexey Proskuryakov]

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]

(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]

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]

Created attachment 107893 [details]
Fixed GTK build

[Ryosuke Niwa]

(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]

Webkit-dev is also fine, but we need to document usage guidelines for exposed rangeFromLocationAndLength and locationAndLengthFromRange in this bug for posterity.

[Ryosuke Niwa]

(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]

Any reviewers?

[Darin Adler]

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]

(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]

(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 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]

(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]

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]

(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]

Created attachment 111076 [details]
new patch

[Ryosuke Niwa]

(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 on attachment 111076 [details]
new patch

Attachment 111076 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10074206

[Ryosuke Niwa]

Created attachment 111157 [details]
Adds internals binding

[Ryosuke Niwa]

Comment on attachment 111157 [details]
Adds internals binding

Yay! Thanks for the review.

[WebKit Review Bot]

Comment on attachment 111157 [details]
Adds internals binding

Clearing flags on attachment: 111157

Committed r97587: <http://trac.webkit.org/changeset/97587>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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 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]

Build fixes have been landed.