Created attachment 83273 [details] cleanup

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

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

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

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

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).

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

(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());

Created attachment 84959 [details] Fixed per Levi's comment

(In reply to comment #11) > Created an attachment (id=84959) [details] > Fixed per Levi's comment Thanks, looks good to me.

Can someone review my patch?

Comment on attachment 84959 [details] Fixed per Levi's comment Looks ok.

(In reply to comment #14) > (From update of attachment 84959 [details]) > Looks ok. Thanks for the review! Landing it now.

Committed r80604: <http://trac.webkit.org/changeset/80604>