Bug 193314

Created attachment 358788 [details] Patch

<rdar://problem/47176519>

Comment on attachment 358788 [details] Patch Attachment 358788 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10697353 New failing tests: imported/w3c/web-platform-tests/webrtc/simplecall.https.html

Created attachment 358795 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 358788 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358788&action=review > Source/WebCore/css/StyleBuilderConverter.h:1359 > + auto& primitiveValue = downcast<CSSPrimitiveValue>(currentValue.get()); Is this always safe? > Source/WebCore/css/StyleBuilderConverter.h:1361 > + if (primitiveValueID != CSSValuePanX && primitiveValueID != CSSValuePanY && primitiveValueID != CSSValuePinchZoom) So "none pan-x" evaluates to be "none" (initial)? > Source/WebCore/css/parser/CSSPropertyParser.cpp:1349 > + RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated(); auto? > Source/WebCore/css/parser/CSSPropertyParser.cpp:1351 > + RefPtr<CSSPrimitiveValue> ident = consumeIdent<CSSValuePanX, CSSValuePanY, CSSValuePinchZoom>(range); auto? > Source/WebCore/dom/Element.cpp:106 > +#include "TouchAction.h" Is it used in this file? > Source/WebCore/platform/TouchAction.h:2 > + * Copyright (C) 2018 Apple Inc. All rights reserved. 2019. > Source/WebCore/platform/TouchAction.h:17 > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Library General Public > + * License as published by the Free Software Foundation; either > + * version 2 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Library General Public License for more details. > + * > + * You should have received a copy of the GNU Library General Public License > + * along with this library; see the file COPYING.LIB. If not, write to > + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, > + * Boston, MA 02110-1301, USA. Why are you using the GPL? Use the normal license. > Source/WebCore/rendering/style/RenderStyle.h:1227 > + void setTouchAction(OptionSet<TouchAction> ta) { SET_VAR(m_rareNonInheritedData, touchAction, ta.toRaw()); } ugh. why not use "touchAction" rather than "ta"? (or "touchActions")

(In reply to Dean Jackson from comment #5) > Comment on attachment 358788 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=358788&action=review > > > Source/WebCore/css/StyleBuilderConverter.h:1359 > > + auto& primitiveValue = downcast<CSSPrimitiveValue>(currentValue.get()); > > Is this always safe? Antti (on IRC) thinks so since we create the value list in consumeTouchAction(). > > Source/WebCore/css/StyleBuilderConverter.h:1361 > > + if (primitiveValueID != CSSValuePanX && primitiveValueID != CSSValuePanY && primitiveValueID != CSSValuePinchZoom) > > So "none pan-x" evaluates to be "none" (initial)? It should evaluate to "auto", the initial value. > > Source/WebCore/css/parser/CSSPropertyParser.cpp:1349 > > + RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated(); > > auto? Will update. > > Source/WebCore/css/parser/CSSPropertyParser.cpp:1351 > > + RefPtr<CSSPrimitiveValue> ident = consumeIdent<CSSValuePanX, CSSValuePanY, CSSValuePinchZoom>(range); > > auto? Will update. > > Source/WebCore/dom/Element.cpp:106 > > +#include "TouchAction.h" > > Is it used in this file? Yes, in Element::allowsDoubleTapGesture(). Since TouchAction now is specified in a separate file, I figured it would be better to explicitly include it. > > Source/WebCore/platform/TouchAction.h:2 > > + * Copyright (C) 2018 Apple Inc. All rights reserved. > > 2019. Will update. > > Source/WebCore/platform/TouchAction.h:17 > > + * This library is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Library General Public > > + * License as published by the Free Software Foundation; either > > + * version 2 of the License, or (at your option) any later version. > > + * > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Library General Public License for more details. > > + * > > + * You should have received a copy of the GNU Library General Public License > > + * along with this library; see the file COPYING.LIB. If not, write to > > + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, > > + * Boston, MA 02110-1301, USA. > > Why are you using the GPL? Use the normal license. Copy/pasted from the wrong file without thinking twice. > > Source/WebCore/rendering/style/RenderStyle.h:1227 > > + void setTouchAction(OptionSet<TouchAction> ta) { SET_VAR(m_rareNonInheritedData, touchAction, ta.toRaw()); } > > ugh. why not use "touchAction" rather than "ta"? > > (or "touchActions") Will update.

You might also consider renaming all OptionSet<TouchAction> functions/fields/variables 'touchAction' -> 'touchActions' (unless you really want to match the property name exactly).

(In reply to Antti Koivisto from comment #7) > You might also consider renaming all OptionSet<TouchAction> > functions/fields/variables 'touchAction' -> 'touchActions' (unless you > really want to match the property name exactly). I will do that, I think that's the right thing to do.

Created attachment 358935 [details] Patch

Created attachment 358937 [details] Patch

Comment on attachment 358937 [details] Patch rs=me

Comment on attachment 358937 [details] Patch Attachment 358937 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10718323 New failing tests: css3/touch-action/touch-action-parsing.html imported/w3c/web-platform-tests/pointerevents/pointerevent_touch-action-illegal.html imported/w3c/web-platform-tests/pointerevents/extension/pointerevent_touch-action-verification.html css3/touch-action/touch-action-computed-style.html imported/w3c/web-platform-tests/pointerevents/pointerevent_touch-action-verification.html

Created attachment 358958 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Committed r239978: <https://trac.webkit.org/changeset/239978>