Bug 138720 – Refactor iOS selection gestures

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Enrica Casucci]

This bug track the work to refactor the selection gestures on iOS.

[Enrica Casucci]

Created attachment 241522 [details]
Patch

[WebKit Commit Bot]

Attachment 241522 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:428:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:429:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:430:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:431:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:432:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:438:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:449:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:460:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:471:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:482:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 10 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 241522 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1786
> +    _page->selectPositionAtPoint(WebCore::IntPoint(point), [selectionHandler](CallbackBase::Error error) {
> +        selectionHandler();

No need to account for errors? (same for the ones below).

For example, if the WebProcess dies during a gesture, you may want to fallback safely?

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2658
> +    [self reloadInputViews];
> +    

This change is odd. I would expect [reloadInputViews] to be the last thing done after updating the internal state.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:303
> +    , m_selectionBaseIsStart(false)

This should be along the other state booleans of WebPage.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1617
> +    m_selectionBaseIsStart = baseIsStart; // FIXME: do we need to flip this for RTL?
> +    send(Messages::WebPageProxy::UnsignedCallback(baseIsStart, callbackID));

This does not look right.

There is one message just to set a boolean state on WebPage, but that boolean is only used when interpreting other messages.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1641
> +            m_selectionBaseIsStart = !m_selectionBaseIsStart;

m_selectionBaseIsStart = false;

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1650
> +            m_selectionBaseIsStart = !m_selectionBaseIsStart;

m_selectionBaseIsStart = true;

[Enrica Casucci]

> No need to account for errors? (same for the ones below).
> 
> For example, if the WebProcess dies during a gesture, you may want to
> fallback safely?
> 
what is important is to call the completion handler, which will prevent any deadlocks.

> > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2658
> > +    [self reloadInputViews];
> > +    
> 
> This change is odd. I would expect [reloadInputViews] to be the last thing
> done after updating the internal state.
> 
Actually it was wrong to call _startAssistingKeyboard before calling reloadInputViews.
It could affect how gesture recognizers are installed.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:303
> > +    , m_selectionBaseIsStart(false)
> 
> This should be along the other state booleans of WebPage.
Ok, will do.
> 
> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1617
> > +    m_selectionBaseIsStart = baseIsStart; // FIXME: do we need to flip this for RTL?
> > +    send(Messages::WebPageProxy::UnsignedCallback(baseIsStart, callbackID));
> 
> This does not look right.
> 
> There is one message just to set a boolean state on WebPage, but that
> boolean is only used when interpreting other messages.
> 
> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1641
> > +            m_selectionBaseIsStart = !m_selectionBaseIsStart;
> 
> m_selectionBaseIsStart = false;
> 
> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1650
> > +            m_selectionBaseIsStart = !m_selectionBaseIsStart;
> 
> m_selectionBaseIsStart = true;

At the beginning I need to know the direction of the gesture and I use it later to decide in which direction to extend the gesture and when to flip.

[Benjamin Poulain]

Comment on attachment 241522 [details]
Patch

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

Ok, rs=me.

Still not a fan of m_selectionBaseIsStart.
What about an enum?

    enum class SelectorAnchor {
        Start,
        End
    }
? (assuming "selection anchor" does not have another meaning in the selection code)

"beginSelectionInDirection" -> the name is also confusing. The "InDirection" is only loosely coupled with baseIsStart.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1616
> +    m_selectionBaseIsStart = baseIsStart; // FIXME: do we need to flip this for RTL?

I would find out and fix it instead of having a FIXME.

[Enrica Casucci]

> 
> "beginSelectionInDirection" -> the name is also confusing. The "InDirection"
> is only loosely coupled with baseIsStart.
> 
> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1616
> > +    m_selectionBaseIsStart = baseIsStart; // FIXME: do we need to flip this for RTL?
> 
> I would find out and fix it instead of having a FIXME.

I agree with you. I'll change it to be direction (Forward/Backward in the interface) and change the bool to an enum to indicate which end of the selection is moving.

[Enrica Casucci]

I've addressed the last comments from Ben and landed the change.
http://trac.webkit.org/changeset/176337