| Summary: | Make the interface of locationAndLengthFromRange and rangeFromLocationAndLength consistent | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
| Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | ap, darin, dglazkov, enrica, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 68330 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Ryosuke Niwa
2011-10-12 14:36:50 PDT
Created attachment 110750 [details]
cleanup
Comment on attachment 110750 [details] cleanup Attachment 110750 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10044051 Created attachment 110755 [details]
Fixed qt & gtk build
Comment on attachment 110755 [details] Fixed qt & gtk build Attachment 110755 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10047048 Created attachment 110759 [details]
Fixed Chromium build
Comment on attachment 110759 [details]
Fixed Chromium build
I don't see the value of this change. We discussed this extensively in IRC and you know what is my opinion about this. TextIterator is a class used to iterate through text both in editable and non editable content. What is this adding as value? Any call will now have an additional parameter that will be either the root editable element or the document element and by looking at the code you can't really tell which one has been selected.
Where is the benefit?
(In reply to comment #6) > (From update of attachment 110759 [details]) >TextIterator is a class used to iterate through text both in editable and non editable content. What is this adding as value? The value is that TextIterator::locationAndLengthFromRange no longer depends on FrameSelection. As Darin said on the bug 68330. TextIterator is rather a low-level concept and I don't think it should depend on the current selection. >Any call will now have an additional parameter that will be either the root editable element or the document element and by looking at the code you can't really tell which one has been selected. > Where is the benefit? The benefit of this patch is remove the dependency on FrameSelection and make locationAndLengthFromRange and rangeFromLocationAndLength both take the scope element. It makes the API more generic. For example, I suspect many platforms other than Mac wants to use rootEditableElementOrDocumentElement. They just want to pass rootEditableElement and bail out in other cases. They probably copied Mac port's code without knowing why Mac port does this. I'd definitely would like to remove this odd behavior from Chromium code for example. Removing FrameSelection dependency sounds valuable to me. Also, it does feel a little too magical for these functions to decide what root to use all by themselves. I have some concerns about the suggested approach. First, the pain point has been just moved - we still have a place that confusingly does "this or that" without explanation. The new place isn't obviously better - why does FrameSelection class decide what offsets text input and editing code uses? The new API also looks confusingly generic. Do we actually need to imply that any element can be used as root? If I were investigating the code, that would send me in the wrong direction. As far as I can tell, root editable element or document are the two options, and they correspond to distinct use cases (editable vs. non-editable content). To address the second point, renaming the argument to "editableRoot", and passing 0 when there is none might suffice. Some assertions and comments need to be added to enforce correct use. (In reply to comment #8) > I have some concerns about the suggested approach. First, the pain point has been just moved - we still have a place that confusingly does "this or that" without explanation. The new place isn't obviously better - why does FrameSelection class decide what offsets text input and editing code uses? FrameSelection doesn't decide. That's why I gave a name that doesn't sound related to these two functions. My intent was that call sites of these functions will clearly document what it's doing and move on to just use rootEditableElement instead. > The new API also looks confusingly generic. Do we actually need to imply that any element can be used as root? I think this is valuable for testing purpose. Because there are few places where we start textIterator at an arbitrary element, it'll be helpful to test various cases but we don't necessarily want to add IDL for TextIterator etc... for testing purposes. > If I were investigating the code, that would send me in the wrong direction. As far as I can tell, root editable element or document are the two options, and they correspond to distinct use cases (editable vs. non-editable content). This is a valid point. Hopefully readers see where these functions are called and figure that out. Alternatively, I can add assertions at the beginning of these classes to assert that scope elements are root editable element or document element. > To address the second point, renaming the argument to "editableRoot", and passing 0 when there is none might suffice. Some assertions and comments need to be added to enforce correct use. So are you suggesting that we'll still magically fall back to documentElement when editableRoot is 0? That sounds too confusing to me. > FrameSelection doesn't decide. I don't see it that way. The function is called "ThisOrThat", meaning that there is a decision made inside. > So are you suggesting that we'll still magically fall back to documentElement when editableRoot is 0? That sounds too confusing to me. The idea is that if there is an editable root, it's used, and if there is no editable root, some default kind of offset is returned. The caller doesn't really care if it's relative to document element or what. Passing 0 for editing root is a reasonable wear to specify that there isn't one. Perhaps this suggestion can grow on you with a little time? Your point about testing is valid. However, this seems like a fairly involved area (few people even know why there are different roots for offsets used in different circumstances), so perhaps we can get away without making tests affect code clarity in this case. (In reply to comment #10) > > FrameSelection doesn't decide. > > I don't see it that way. The function is called "ThisOrThat", meaning that there is a decision made inside. Perhaps we just need a better name. > > So are you suggesting that we'll still magically fall back to documentElement when editableRoot is 0? That sounds too confusing to me. > > The idea is that if there is an editable root, it's used, and if there is no editable root, some default kind of offset is returned. This is the part I don't really like. It's not clear at all why we have some fallback ingrained in TextIterator code. > The caller doesn't really care if it's relative to document element or what. Passing 0 for editing root is a reasonable wear to specify that there isn't one. Perhaps this suggestion can grow on you with a little time? The part I don't get is why we need an offset rooted at document. As far as I'm concerned, there is no use case for this in Chromium port. > Your point about testing is valid. However, this seems like a fairly involved area (few people even know why there are different roots for offsets used in different circumstances), so perhaps we can get away without making tests affect code clarity in this case. I'd really like to understand why we need such a document fallback and Enrica pointed out on IRC that this hack is needed to support RTF copy & paste in Mac port. If that's the case, then it's not applicable to many other usage (e.g. firstRect... in Qt & GTK). NSTextInputClient methods like attributedSubstringForProposedRange:actualRange: can be called on non-editable content, that's simply API contract. Methods like firstRectForCharacterRange also obviously have uses beyond editing. At some point, Dictionary pop-up on Mac OS X was using NSTextInput (then, without "Client") to get text to highlight from WebKit. I don't know if it is still doing that. (In reply to comment #12) > NSTextInputClient methods like attributedSubstringForProposedRange:actualRange: can be called on non-editable content, that's simply API contract. Methods like firstRectForCharacterRange also obviously have uses beyond editing. > > At some point, Dictionary pop-up on Mac OS X was using NSTextInput (then, without "Client") to get text to highlight from WebKit. I don't know if it is still doing that. I see the value of removing implicit dependence on frame selection on a low level class like TextIterator even though there is a lot of code in TextIterator that is aimed at preserving the selection. The two methods we are discussing are the only ones that reference the selection object, but the concept still pervades the class (see comments throughout the file). The point I'm trying to make is that this change is not a step forward in making more clear what is the exact scope in every case these methods are called. Your argument, and I apologize if I'm misunderstanding you here, is that there should no need to default to the document element because this should only be used with editable content. I believe Alexey has explained correctly why we need that. I also want to stress again that TextIterator is not meant to deal only with editable content. I agree that is not desirable not to know exactly which scope is being used and when, but I honestly I don't see how this patch is making this more explicit. (In reply to comment #13) > The point I'm trying to make is that this change is not a step forward in making more clear what is the exact scope in every case these methods are called. > Your argument, and I apologize if I'm misunderstanding you here, is that there should no need to default to the document element because this should only be used with editable content. Not really. What I'm saying is that some callers of these functions is falling back to document needlessly. WebViewImpl::compositionRange in chromium/src/WebViewImpl.cpp for example shouldn't be falling back to Document when there is no editable content as this function is used only to deal with IME. So it appears to me that we need to address the use care where you want to call these functions with a scope element without falling back to document. > I also want to stress again that TextIterator is not meant to deal only with editable content. Agreed. > I agree that is not desirable not to know exactly which scope is being used and when, but I honestly I don't see how this patch is making this more explicit. This patch itself doesn't because I'm not changing any behaviors. But I'm intending to change some call sites of these functions to use rootEditableElement instead of rootEditableElementOrDocumentElement in follow ups. Comment on attachment 110759 [details]
Fixed Chromium build
I understand that this is the first step in the direction of making the code more understandable and removing knowledge about selection from the TextIterator class. I look forward to seeing the follow up.
(In reply to comment #15) > (From update of attachment 110759 [details]) > I understand that this is the first step in the direction of making the code more understandable and removing knowledge about selection from the TextIterator class. I look forward to seeing the follow up. Thanks the review :D I believe this and follow up patches will make TextIterator much more hackable and maintainable. Committed r97478: <http://trac.webkit.org/changeset/97478> |