Bug 54937

Summary: Remove calls to deprecatedEditingOffset in SelectionController and VisibleSelection
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: darin, eric, leviw, ojan, rolandsteiner, tkent, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 52099    
Attachments:
Description Flags
cleanup
none
Fixed per Levi's comment tkent: review+, tkent: commit-queue+

Description Ryosuke Niwa 2011-02-21 22:28:25 PST
This is a cleanup
Comment 1 Ryosuke Niwa 2011-02-21 22:37:50 PST
Created attachment 83273 [details]
cleanup
Comment 2 Levi Weintraub 2011-02-22 11:18:13 PST
Comment on attachment 83273 [details]
cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=83273&action=review

Looks good to me besides the nit.

> Source/WebCore/editing/SelectionController.cpp:1442
> +    Node* startNode = start().containerNode();

containerNode can be null here when there's still a valid anchorNode and shadow ancestor. I don't think this will ever be a problem for input tags, but this smacks of an assumption that could change.
Comment 3 Ryosuke Niwa 2011-02-22 16:58:15 PST
Comment on attachment 83273 [details]
cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=83273&action=review

>> Source/WebCore/editing/SelectionController.cpp:1442
>> +    Node* startNode = start().containerNode();
> 
> containerNode can be null here when there's still a valid anchorNode and shadow ancestor. I don't think this will ever be a problem for input tags, but this smacks of an assumption that could change.

That should be fine because we're only trying to figure out whether we're inside a input[type=password] or not.  If we ever had a position like before/after the shadow DOM's root element, we'll be in a trouble anyways.
Comment 4 Levi Weintraub 2011-02-22 17:00:43 PST
(In reply to comment #3)
> (From update of attachment 83273 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83273&action=review
> 
> >> Source/WebCore/editing/SelectionController.cpp:1442
> >> +    Node* startNode = start().containerNode();
> > 
> > containerNode can be null here when there's still a valid anchorNode and shadow ancestor. I don't think this will ever be a problem for input tags, but this smacks of an assumption that could change.
> 
> That should be fine because we're only trying to figure out whether we're inside a input[type=password] or not.  If we ever had a position like before/after the shadow DOM's root element, we'll be in a trouble anyways.

Sounds like good rationale for an assert then :)
Comment 5 Ryosuke Niwa 2011-02-22 17:03:27 PST
Comment on attachment 83273 [details]
cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=83273&action=review

>>>> Source/WebCore/editing/SelectionController.cpp:1442
>>>> +    Node* startNode = start().containerNode();
>>> 
>>> containerNode can be null here when there's still a valid anchorNode and shadow ancestor. I don't think this will ever be a problem for input tags, but this smacks of an assumption that could change.
>> 
>> That should be fine because we're only trying to figure out whether we're inside a input[type=password] or not.  If we ever had a position like before/after the shadow DOM's root element, we'll be in a trouble anyways.
> 
> Sounds like good rationale for an assert then :)

What kind of assertion are you talking about?  I can't do ASSERT(startNode) here because start() could be null.
Comment 6 Levi Weintraub 2011-02-22 17:15:41 PST
(In reply to comment #5)
> (From update of attachment 83273 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83273&action=review
> 
> >>>> Source/WebCore/editing/SelectionController.cpp:1442
> >>>> +    Node* startNode = start().containerNode();
> >>> 
> >>> containerNode can be null here when there's still a valid anchorNode and shadow ancestor. I don't think this will ever be a problem for input tags, but this smacks of an assumption that could change.
> >> 
> >> That should be fine because we're only trying to figure out whether we're inside a input[type=password] or not.  If we ever had a position like before/after the shadow DOM's root element, we'll be in a trouble anyways.
> > 
> > Sounds like good rationale for an assert then :)
> 
> What kind of assertion are you talking about?  I can't do ASSERT(startNode) here because start() could be null.

Something like this perhaps?
ASSERT(start().isNull() || start().anchorType() == Position::PositionIsOffsetInAnchor);

Using containerNode to get something like shadow ancestor or document can be error prone since not all valid positions have a containerNode, i.e. before/after positions anchored to a shadow DOM's root element.
Comment 7 Ryosuke Niwa 2011-02-22 18:31:28 PST
(In reply to comment #6)
> Something like this perhaps?
> ASSERT(start().isNull() || start().anchorType() == Position::PositionIsOffsetInAnchor);

No this wouldn't work.  Because start() could be before/after a node.  The whole point of this function is to figure out whether or not we're inside an input[type=password].  Position could be anywhere.

> Using containerNode to get something like shadow ancestor or document can be error prone since not all valid positions have a containerNode, i.e. before/after positions anchored to a shadow DOM's root element.

But this can't happen inside an input element because we always have div, and we should never have a position before/after this div as that would break all sorts of assumptions in other parts of the editing code.  In my opinion, we should refactor shadow DOM code so that we always have a document-like ShadowRootElement.  Roland would know where we are now.
Comment 8 Roland Steiner 2011-02-22 21:43:27 PST
I intend to add a ShadowRoot object as Ryosuke described after we're finished with the transition from render-object based shadow DOM for all elements (until then it's too much of a life-time headache and might cause performance regressions).
Comment 9 Levi Weintraub 2011-02-23 11:47:50 PST
(In reply to comment #7)
> But this can't happen inside an input element because we always have div, and we should never have a position before/after this div...

This is the case I'm proposing we should assert, but if you're comfortable just saying it'll never happen you can go that route.
Comment 10 Ryosuke Niwa 2011-02-23 19:30:34 PST
(In reply to comment #9)
> (In reply to comment #7)
> > But this can't happen inside an input element because we always have div, and we should never have a position before/after this div...
> 
> This is the case I'm proposing we should assert, but if you're comfortable just saying it'll never happen you can go that route.

Maybe something like this?
ASSERT(start().isNull() || start().anchorType() == PositionIsInAnchorNode || start().containerNode() || !start().anchorNode()->shadowAncestorNode());
Comment 11 Ryosuke Niwa 2011-03-07 11:10:34 PST
Created attachment 84959 [details]
Fixed per Levi's comment
Comment 12 Levi Weintraub 2011-03-07 11:11:47 PST
(In reply to comment #11)
> Created an attachment (id=84959) [details]
> Fixed per Levi's comment

Thanks, looks good to me.
Comment 13 Ryosuke Niwa 2011-03-08 15:25:31 PST
Can someone review my patch?
Comment 14 Kent Tamura 2011-03-08 15:43:03 PST
Comment on attachment 84959 [details]
Fixed per Levi's comment

Looks ok.
Comment 15 Ryosuke Niwa 2011-03-08 16:50:13 PST
(In reply to comment #14)
> (From update of attachment 84959 [details])
> Looks ok.

Thanks for the review!  Landing it now.
Comment 16 Ryosuke Niwa 2011-03-08 16:51:17 PST
Committed r80604: <http://trac.webkit.org/changeset/80604>