Upstream the iOS related changes to WebCore/html.
Created attachment 219294 [details] Patch
Comment on attachment 219294 [details] Patch Attachment 219294 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/47808242
Comment on attachment 219294 [details] Patch Attachment 219294 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/49638148
Comment on attachment 219294 [details] Patch Attachment 219294 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/49568144
Comment on attachment 219294 [details] Patch Attachment 219294 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/47808246
Comment on attachment 219294 [details] Patch Attachment 219294 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/46378183
Comment on attachment 219294 [details] Patch Attachment 219294 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/44578096
Created attachment 219324 [details] For EWS: Concatenated patches from Bug 125646 And Bug 125765 Please don't review this patch. It's for the EWS bots. This patch represents the concatenation of the patch on Bug 125646 and the patch on this bug (bug #125765).
Attachment 219324 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/dom/ActiveDOMObject.h', u'Source/WebCore/dom/DOMImplementation.cpp', u'Source/WebCore/dom/DeviceMotionClient.h', u'Source/WebCore/dom/DeviceMotionController.cpp', u'Source/WebCore/dom/DeviceMotionController.h', u'Source/WebCore/dom/DeviceOrientationClient.h', u'Source/WebCore/dom/DeviceOrientationController.cpp', u'Source/WebCore/dom/DeviceOrientationController.h', u'Source/WebCore/dom/DeviceOrientationData.cpp', u'Source/WebCore/dom/DeviceOrientationData.h', u'Source/WebCore/dom/DeviceOrientationEvent.idl', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/Document.idl', u'Source/WebCore/dom/DocumentMarker.h', u'Source/WebCore/dom/DocumentMarkerController.cpp', u'Source/WebCore/dom/DocumentMarkerController.h', u'Source/WebCore/dom/Element.cpp', u'Source/WebCore/dom/Element.h', u'Source/WebCore/dom/EventContext.cpp', u'Source/WebCore/dom/EventContext.h', u'Source/WebCore/dom/EventDispatcher.cpp', u'Source/WebCore/dom/EventNames.h', u'Source/WebCore/dom/EventNames.in', u'Source/WebCore/dom/MouseRelatedEvent.cpp', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/dom/Node.h', u'Source/WebCore/dom/Position.h', u'Source/WebCore/dom/Range.cpp', u'Source/WebCore/dom/Range.h', u'Source/WebCore/dom/ScriptExecutionContext.cpp', u'Source/WebCore/dom/TreeScope.cpp', u'Source/WebCore/dom/ViewportArguments.cpp', u'Source/WebCore/dom/ViewportArguments.h', u'Source/WebCore/dom/make_names.pl', u'Source/WebCore/html/Autocapitalize.cpp', u'Source/WebCore/html/Autocapitalize.h', u'Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp', u'Source/WebCore/html/BaseDateAndTimeInputType.cpp', u'Source/WebCore/html/BaseDateAndTimeInputType.h', u'Source/WebCore/html/FileInputType.cpp', u'Source/WebCore/html/FileInputType.h', u'Source/WebCore/html/FormController.cpp', u'Source/WebCore/html/FormController.h', u'Source/WebCore/html/HTMLAnchorElement.cpp', u'Source/WebCore/html/HTMLAppletElement.cpp', u'Source/WebCore/html/HTMLAreaElement.cpp', u'Source/WebCore/html/HTMLAreaElement.h', u'Source/WebCore/html/HTMLAttributeNames.in', u'Source/WebCore/html/HTMLBodyElement.cpp', u'Source/WebCore/html/HTMLButtonElement.cpp', u'Source/WebCore/html/HTMLCanvasElement.cpp', u'Source/WebCore/html/HTMLCanvasElement.h', u'Source/WebCore/html/HTMLDocument.cpp', u'Source/WebCore/html/HTMLDocument.h', u'Source/WebCore/html/HTMLElement.cpp', u'Source/WebCore/html/HTMLElement.h', u'Source/WebCore/html/HTMLFormControlElement.cpp', u'Source/WebCore/html/HTMLFormControlElement.h', u'Source/WebCore/html/HTMLFormElement.cpp', u'Source/WebCore/html/HTMLFormElement.h', u'Source/WebCore/html/HTMLFormElement.idl', u'Source/WebCore/html/HTMLIFrameElement.h', u'Source/WebCore/html/HTMLInputElement.cpp', u'Source/WebCore/html/HTMLInputElement.h', u'Source/WebCore/html/HTMLInputElement.idl', u'Source/WebCore/html/HTMLLabelElement.cpp', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/html/HTMLMediaElement.idl', u'Source/WebCore/html/HTMLMetaElement.cpp', u'Source/WebCore/html/HTMLObjectElement.cpp', u'Source/WebCore/html/HTMLPlugInElement.h', u'Source/WebCore/html/HTMLPlugInImageElement.cpp', u'Source/WebCore/html/HTMLPlugInImageElement.h', u'Source/WebCore/html/HTMLSelectElement.cpp', u'Source/WebCore/html/HTMLSelectElement.h', u'Source/WebCore/html/HTMLSummaryElement.cpp', u'Source/WebCore/html/HTMLTextAreaElement.cpp', u'Source/WebCore/html/HTMLTextAreaElement.h', u'Source/WebCore/html/HTMLTextAreaElement.idl', u'Source/WebCore/html/HTMLTextFormControlElement.cpp', u'Source/WebCore/html/HTMLTextFormControlElement.h', u'Source/WebCore/html/HTMLVideoElement.cpp', u'Source/WebCore/html/HTMLVideoElement.h', u'Source/WebCore/html/HTMLVideoElement.idl', u'Source/WebCore/html/ImageDocument.cpp', u'Source/WebCore/html/InputType.cpp', u'Source/WebCore/html/InputType.h', u'Source/WebCore/html/PluginDocument.cpp', u'Source/WebCore/html/RangeInputType.cpp', u'Source/WebCore/html/RangeInputType.h', u'Source/WebCore/html/SearchInputType.cpp', u'Source/WebCore/html/TextFieldInputType.cpp', u'Source/WebCore/html/TextFieldInputType.h', u'Source/WebCore/html/WebAutocapitalize.h', u'Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp', u'Source/WebCore/html/parser/HTMLConstructionSite.h', u'Source/WebCore/html/parser/HTMLParserScheduler.h', u'Source/WebCore/html/parser/HTMLTreeBuilder.cpp', u'Source/WebCore/html/parser/HTMLTreeBuilder.h', u'Source/WebCore/html/shadow/MediaControlElements.cpp', u'Source/WebCore/html/shadow/MediaControlElements.h', u'Source/WebCore/html/shadow/MediaControls.h', u'Source/WebCore/html/shadow/SliderThumbElement.cpp', u'Source/WebCore/html/shadow/SliderThumbElement.h', u'Source/WebCore/html/shadow/TextControlInnerElements.cpp', u'Source/WebCore/html/shadow/TextControlInnerElements.h', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/Settings.h', u'Source/WebCore/page/Settings.in', '--commit-queue']" exit_code: 1 ERROR: Source/WebCore/dom/EventNames.h:268: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/EventNames.h:271: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/EventNames.h:273: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/EventNames.h:277: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 4 in 114 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 219294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=219294&action=review > Source/WebCore/html/Autocapitalize.cpp:36 > +static const AtomicString& valueDefault() > +{ > + return nullAtom; > +} No reason for this function to exist. Caller should just return nullAtom. > Source/WebCore/html/Autocapitalize.h:30 > +#include <wtf/text/AtomicString.h> No reason to include the AtomicString header just to compile const AtomicString&. This should instead be a forward declaration. > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:58 > +#if !PLATFORM(IOS) > m_dateTimeChooser = element().document().page()->chrome().openDateTimeChooser(this, parameters); > +#endif Why not on iOS? Needs a comment. And ideally a better #if in the future. > Source/WebCore/html/FileInputType.cpp:91 > +#if PLATFORM(IOS) > + , m_displayString(String()) > +#endif This is not needed. m_displayString will get initialized to the null string without explicit code to do so. > Source/WebCore/html/FileInputType.cpp:103 > +#if !PLATFORM(IOS) > if (m_fileIconLoader) > m_fileIconLoader->invalidate(); > +#endif Why not on iOS? Needs a comment. And ideally a better #if in the future. > Source/WebCore/html/FileInputType.cpp:339 > +#if !PLATFORM(IOS) > Chrome* chrome = this->chrome(); The entire function should be #if'd out, not just the end of the function. > Source/WebCore/html/FileInputType.cpp:452 > +#if ENABLE(DRAG_SUPPORT) I don’t understand why we are putting this #if only around the body of the function rather than around the entire function. > Source/WebCore/html/FormController.cpp:386 > + for (auto it = m_formElementsWithState.begin(), end = m_formElementsWithState.end(); it != end; ++it) { > + HTMLFormControlElementWithState* element = it->get(); Should write this with new style for loop: for (auto element : m_formElementsWithState.begin) > Source/WebCore/html/HTMLAnchorElement.cpp:619 > + // FIXME: We should reconcile the difference in disjunct ordering between iOS and OpenSource. > +#if PLATFORM(IOS) > + return HTMLElement::willRespondToMouseClickEvents() || isLink(); > +#else > return isLink() || HTMLElement::willRespondToMouseClickEvents(); > +#endif Yes. Reconcile by always calling isLink first. > Source/WebCore/html/HTMLButtonElement.cpp:163 > +#if PLATFORM(IOS) > + return !isDisabledFormControl(); > +#else > if (!isDisabledFormControl() && form() && (m_type == SUBMIT || m_type == RESET)) > return true; > return HTMLFormControlElement::willRespondToMouseClickEvents(); > +#endif Why the difference? > Source/WebCore/html/HTMLCanvasElement.h:132 > + void setMaxiumDecodedImageSize(float maximumDecodedImageSize) { m_maximumDecodedImageSize = maximumDecodedImageSize; } Typo: "setMaxium" > Source/WebCore/html/HTMLElement.cpp:1100 > +#if PLATFORM(IOS) > +bool HTMLElement::willRespondToMouseMoveEvents() > +{ > + return !isDisabledFormControl() && Element::willRespondToMouseMoveEvents(); > +} > + > +bool HTMLElement::willRespondToMouseWheelEvents() > +{ > + return !isDisabledFormControl() && Element::willRespondToMouseWheelEvents(); > +} > + > +bool HTMLElement::willRespondToMouseClickEvents() > +{ > + return !isDisabledFormControl() && Element::willRespondToMouseClickEvents(); > +} > +#endif Why are these iOS-specific? I think we want these on all platforms. > Source/WebCore/html/HTMLFormElement.cpp:394 > + const AtomicString& autocorrectValue = fastGetAttribute(autocorrectAttr); > + if (!autocorrectValue.isEmpty()) > + return !equalIgnoringCase(autocorrectValue, "off"); > + if (HTMLFormElement* form = this->form()) > + return form->autocorrect(); > + return true; A real shame that this (and all the other related functions) repeats the code from HTMLFormControlElement. We should find a way to share code. > Source/WebCore/html/HTMLInputElement.cpp:1207 > + // FIXME: Is this code necessary given that we check evt->isTouchEvent() (above)? > + if (evt->eventInterface() == TouchEventInterfaceType) { This check is not needed. The call to eventInterface is irrelevant. > Source/WebCore/html/HTMLInputElement.cpp:1224 > +#if PLATFORM(IOS) > + return !isDisabledFormControl(); > +#else Why different on iOS? > Source/WebCore/html/HTMLInputElement.cpp:1662 > +#endif > + > + > bool HTMLInputElement::isTextButton() const Please don’t add the extra blank line. > Source/WebCore/html/HTMLInputElement.idl:128 > +#if WTF_PLATFORM_IOS This is not the correct way to do the #if in the IDL file, I don’t think. > Source/WebCore/html/HTMLLabelElement.cpp:148 > +#if PLATFORM(IOS) > + return control() || HTMLElement::willRespondToMouseClickEvents(); > +#else > + return (control() && control()->willRespondToMouseClickEvents()) || HTMLElement::willRespondToMouseClickEvents(); > +#endif Why the difference on iOS? > Source/WebCore/html/HTMLMediaElement.cpp:1926 > +#if PLATFORM(IOS) > + bool shouldUpdatePlayState = false; > +#endif Makes no sense to check in code with a boolean that is always false. > Source/WebCore/html/HTMLMediaElement.cpp:1955 > +#if PLATFORM(IOS) > + if (shouldUpdatePlayState) > + updatePlayState(); > +#endif This code always does nothing since shouldUpdatePlayState is always false. > Source/WebCore/html/HTMLPlugInElement.h:86 > +#if PLATFORM(IOS) > + virtual bool willRespondToMouseMoveEvents() OVERRIDE { return false; } > +#endif Not sure we should have mousemove events at all on iOS. It seems strange to override this function like this rather than turning off additional code regarding mouse move events. > Source/WebCore/html/HTMLSummaryElement.cpp:167 > +#if PLATFORM(IOS) > + return HTMLElement::willRespondToMouseClickEvents() || (isMainSummary() && renderer() && detailsElement()); > +#else > if (isMainSummary() && renderer()) > return true; > > return HTMLElement::willRespondToMouseClickEvents(); > +#endif Doesn’t seem right to have a different case for iOS here. > Source/WebCore/html/HTMLTextAreaElement.cpp:553 > +#if PLATFORM(IOS) > +bool HTMLTextAreaElement::willRespondToMouseClickEvents() > +{ > + return !isDisabledFormControl(); > } > +#endif Seems like we want this code on all platforms, not just iOS. > Source/WebCore/html/ImageDocument.cpp:258 > + return; No need for this return statement. > Source/WebCore/html/ImageDocument.cpp:279 > + return; No need for this return statement. > Source/WebCore/html/InputType.cpp:564 > +#if PLATFORM(IOS) > + if (element().isReadOnly()) > + return false; > +#endif Is this rule really iOS-specific? > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:68 > +#if PLATFORM(IOS) > +#include "SoftLinking.h" > + > +#ifdef __has_include > +#if __has_include(<DataDetectorsCore/DDDFACache.h>) > +#include <DataDetectorsCore/DDDFACache.h> > +#else > +typedef void* DDDFACacheRef; > +#endif > + > +#if __has_include(<DataDetectorsCore/DDDFAScanner.h>) > +#include <DataDetectorsCore/DDDFAScanner.h> > +#else > +typedef void* DDDFAScannerRef; > +#endif > +#endif > + > +SOFT_LINK_PRIVATE_FRAMEWORK_OPTIONAL(DataDetectorsCore) > +SOFT_LINK(DataDetectorsCore, DDDFACacheCreateFromFramework, DDDFACacheRef, (), ()) > +SOFT_LINK(DataDetectorsCore, DDDFAScannerCreateFromCache, DDDFAScannerRef, (DDDFACacheRef cache), (cache)) > +SOFT_LINK(DataDetectorsCore, DDDFAScannerFirstResultInUnicharArray, Boolean, (DDDFAScannerRef scanner, const UniChar* str, unsigned length, int* startPos, int* endPos), (scanner, str, length, startPos, endPos)) > +#endif I’d like to see this code in a separate file, not in HTMLTreeBuilder.cpp itself. > Source/WebCore/html/shadow/MediaControlElements.h:298 > -#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) > +#if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) Why is this change needed? Doesn’t PLATFORM(IOS) also imply PLATFORM(MAC) at this time? > Source/WebCore/html/shadow/SliderThumbElement.h:47 > +#if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS) > +class TouchEvent; > +#endif Seems pointless to put this inside an #if. Just make it an unconditional forward declaration, please. > Source/WebCore/html/shadow/SliderThumbElement.h:62 > + virtual void didAttachRenderers() OVERRIDE; > + void handleTouchEvent(TouchEvent*); > + > + void disabledAttributeChanged(); Can one or more of these be private instead of public?
Comment on attachment 219294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=219294&action=review >> Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:58 >> +#endif > > Why not on iOS? Needs a comment. And ideally a better #if in the future. The reason for this specific #if is that iOS ifdef'd out Chrome::openDateTimeChooser, and this is a call site. This is the only call site of openDateTimeChooser, so unifdefing the method on iOS and keeping it empty sounds like it would be fine. FWIW, I don't know if iOS actually reaches this code anyways. We handle form controls above WebCore, so tapping on an <input type="date"> probably never even runs this code.
Comment on attachment 219294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=219294&action=review >> Source/WebCore/html/Autocapitalize.cpp:36 >> +} > > No reason for this function to exist. Caller should just return nullAtom. Will inline nullAtom in caller and remove this function. >> Source/WebCore/html/Autocapitalize.h:30 >> +#include <wtf/text/AtomicString.h> > > No reason to include the AtomicString header just to compile const AtomicString&. This should instead be a forward declaration. Yes, I agree. When I forward declare this it caused many compiler errors when merging this patch back to iOS. I hope you don't mind that I add a FIXME comment for now and defer fixing this until more of the iOS port is upstream given the directory-specific nature of this patch. >> Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:58 >> +#endif > > Why not on iOS? Needs a comment. And ideally a better #if in the future. I don't know the reason for this change. When I have a moment I'll blame/look at the history of this file. Will add a FIXME comment for now. >> Source/WebCore/html/FileInputType.cpp:91 >> +#endif > > This is not needed. m_displayString will get initialized to the null string without explicit code to do so. Will remove. >> Source/WebCore/html/FileInputType.cpp:103 >> +#endif > > Why not on iOS? Needs a comment. And ideally a better #if in the future. I don't know the reason for this change. When I have a moment I'll blame/look at the history of this file. Will add a FIXME comment for now. >> Source/WebCore/html/FileInputType.cpp:339 >> Chrome* chrome = this->chrome(); > > The entire function should be #if'd out, not just the end of the function. Will fix. >> Source/WebCore/html/FileInputType.cpp:452 >> +#if ENABLE(DRAG_SUPPORT) > > I don’t understand why we are putting this #if only around the body of the function rather than around the entire function. Ditto. >> Source/WebCore/html/FormController.cpp:386 >> + HTMLFormControlElementWithState* element = it->get(); > > Should write this with new style for loop: > > for (auto element : m_formElementsWithState.begin) Will write using C++11 range-based for loop. >> Source/WebCore/html/HTMLAnchorElement.cpp:619 >> +#endif > > Yes. Reconcile by always calling isLink first. Will fix. >> Source/WebCore/html/HTMLButtonElement.cpp:163 >> +#endif > > Why the difference? Ditto. >> Source/WebCore/html/HTMLCanvasElement.h:132 >> + void setMaxiumDecodedImageSize(float maximumDecodedImageSize) { m_maximumDecodedImageSize = maximumDecodedImageSize; } > > Typo: "setMaxium" Ditto. >> Source/WebCore/html/HTMLElement.cpp:1100 >> +#endif > > Why are these iOS-specific? I think we want these on all platforms. Will make these available to all platforms. >> Source/WebCore/html/HTMLFormElement.cpp:394 >> + return true; > > A real shame that this (and all the other related functions) repeats the code from HTMLFormControlElement. We should find a way to share code. For now, I'll add a FIXME comment to this file and in file HTMLFormControlElement.cpp. >> Source/WebCore/html/HTMLInputElement.cpp:1207 >> + if (evt->eventInterface() == TouchEventInterfaceType) { > > This check is not needed. The call to eventInterface is irrelevant. Then I'll remove this code since we have existing logic in this function for touch events. >> Source/WebCore/html/HTMLInputElement.cpp:1224 >> +#else > > Why different on iOS? Will fix. >> Source/WebCore/html/HTMLInputElement.cpp:1662 >> bool HTMLInputElement::isTextButton() const > > Please don’t add the extra blank line. Will remove extraneous empty line such that there is exactly one empty line between the #endif (line 1659) and the prototype for HTMLInputElement::isTextButton() (line 1662). >> Source/WebCore/html/HTMLInputElement.idl:128 >> +#if WTF_PLATFORM_IOS > > This is not the correct way to do the #if in the IDL file, I don’t think. From reading WebCore/bindings/scripts/CodeGenerator.pm, the IDL attribute, [Conditional=X], only works for ENABLE_X defines (*). That is, we cannot use this attribute to generate "#if WTF_PLATFORM_IOS". I'll define an enable macro, called IOS_AUTOCORRECT_AND_AUTOCAPITALIZE, and then use it to guard the iOS autocorrect and autocapitalization code, including our IDL attributes. (*) This is also consistent with the documentation for the IDL attribute Conditional: <http://trac.webkit.org/wiki/WebKitIDL#Conditional>. >> Source/WebCore/html/HTMLLabelElement.cpp:148 >> +#endif > > Why the difference on iOS? Will fix. >> Source/WebCore/html/HTMLMediaElement.cpp:1926 >> +#endif > > Makes no sense to check in code with a boolean that is always false. Will remove. >> Source/WebCore/html/HTMLMediaElement.cpp:1955 >> +#endif > > This code always does nothing since shouldUpdatePlayState is always false. Ditto. >> Source/WebCore/html/HTMLPlugInElement.h:86 >> +#endif > > Not sure we should have mousemove events at all on iOS. It seems strange to override this function like this rather than turning off additional code regarding mouse move events. We talked about this in person today. This code is related to our tap highlight logic on iOS. >> Source/WebCore/html/HTMLSummaryElement.cpp:167 >> +#endif > > Doesn’t seem right to have a different case for iOS here. Will fix. >> Source/WebCore/html/HTMLTextAreaElement.cpp:553 >> +#endif > > Seems like we want this code on all platforms, not just iOS. Will compile this code for all platforms. >> Source/WebCore/html/ImageDocument.cpp:258 >> + return; > > No need for this return statement. Will remove. >> Source/WebCore/html/ImageDocument.cpp:279 >> + return; > > No need for this return statement. Ditto. >> Source/WebCore/html/InputType.cpp:564 >> +#endif > > Is this rule really iOS-specific? This seems applicable to non-iOS ports. Will remove PLATFORM(iOS) guards. >> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:68 >> +#endif > > I’d like to see this code in a separate file, not in HTMLTreeBuilder.cpp itself. As we discussed in person today, I will defer such a clean up at this time and add a FIXME comment to this file. >> Source/WebCore/html/shadow/MediaControlElements.h:298 >> +#if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) > > Why is this change needed? Doesn’t PLATFORM(IOS) also imply PLATFORM(MAC) at this time? It isn't needed and I'll remove it. Additional remarks: When I put together this patch I thought to keep it towards separating the concept of PLATFORM(IOS) from PLATFORM(MAC) in the future (if deemed desirable). Having said that, we'll need to audit all uses of PLATFORM(MAC) should we choose to separate these concepts, including its use here. So, I'll remove PLATFORM(IOS). >> Source/WebCore/html/shadow/SliderThumbElement.h:47 >> +#endif > > Seems pointless to put this inside an #if. Just make it an unconditional forward declaration, please. Will make an forward declaration unconditional. >> Source/WebCore/html/shadow/SliderThumbElement.h:62 >> + void disabledAttributeChanged(); > > Can one or more of these be private instead of public? Will make SliderThumbElement::didAttachRenderers() private. As it turns out SliderThumbElement::{handleTouchEvent, disabledAttributeChanged}() have exactly one caller: RangeInputType::{handleTouchEvent, disabledAttributeChanged}(), respectively. Unless we make RangeInputType::{handleTouchEvent, disabledAttributeChanged}() friend functions or RangeInputType a friend class, SliderThumbElement::{handleTouchEvent, disabledAttributeChanged}() will need to be public.
Committed r160733: <http://trac.webkit.org/changeset/160733>