This bug track the work to refactor the selection gestures on iOS.
Created attachment 241522 [details] Patch
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.
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;
> 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.
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.
> > "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.
I've addressed the last comments from Ben and landed the change. http://trac.webkit.org/changeset/176337