Add disabled highlight API skeleton
Created attachment 384762 [details] Patch
<rdar://problem/57605049>
Comment on attachment 384762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384762&action=review > Source/WebCore/Configurations/FeatureDefines.xcconfig:194 > +ENABLE_HIGHLIGHT_API_iphoneos = ENABLE_HIGHLIGHT_API; Why not just ENABLE_HIGHLIGHT_API = ENABLE_HIGHLIGHT_API;? I don't think you need this to be platform-dependent, and certainly you don't want it to be off on watchOS/tvOS.
Comment on attachment 384762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384762&action=review > Source/WebCore/Modules/highlight/HighlightMap.h:54 > + ExceptionOr<void> setNamedItem(const String& name, const HighlightRangeGroup& value); > + bool deleteNamedProperty(const String& name); > + > + ExceptionOr<void> set(const String& name, const HighlightRangeGroup& value); Do we really need both?? > Source/WebCore/Modules/highlight/HighlightMap.h:65 > + Nit: blank line. > Source/WebCore/Modules/highlight/HighlightMap.idl:35 > + // FIXME: add any additional functionality as specified by the full spec This comment seems unnecessary. We obviously need to do that but like any other DOM API. > Source/WebCore/Modules/highlight/HighlightRangeGroup.h:59 > + Ref<HighlightRangeGroup> m_group; > + size_t m_index { 0 }; // FIXME: There needs to be a mechanism to handle when ranges are added or removed from the middle of the HighlightRangeGroup. This iterator probably needs to refer back to HighlightRangeGroup and iterate over the real list. > Source/WebCore/Modules/highlight/HighlightRangeGroup.idl:32 > + iterable<StaticRange>; // TODO: should be setlike Use FIXME instead. > Source/WebCore/Modules/highlight/HighlightRangeGroup.idl:33 > +// readonly attribute CSSStyleDeclaration style; // TODO: add inline style support Ditto. Also, this is doubly commented out. Why not just : FIXME: Add readonly attribute CSSStyleDeclaration style? > LayoutTests/highlight/highlight-interfaces.html:9 > +debug("\nTesting Highlight:"); Why \n at the beginning?
Comment on attachment 384762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384762&action=review > Source/WebCore/Modules/highlight/HighlightMap.idl:34 > + [MayThrowException] void set (DOMString style, HighlightRangeGroup group); Nit - extra space between set and ( > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:64 > + Nit - extra newline. > Source/WebCore/Modules/highlight/HighlightRangeGroup.h:65 > + Nit - extra newline > Source/WebCore/css/DOMCSSNamespace.idl:37 > + [CallWith=Document] static readonly attribute HighlightMap highlights; Does this need an EnabledAtRuntime too?
Created attachment 384768 [details] Patch
Comment on attachment 384768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384768&action=review > Source/WebCore/Modules/highlight/HighlightMap.cpp:2 > +* Copyright (C) 2019 Apple Inc. All rights reserved. There should be a space before the * Same for a few files in this. > Source/WebCore/Modules/highlight/HighlightMap.cpp:36 > + extra space > Source/WebCore/Modules/highlight/HighlightMap.cpp:49 > + ditto > Source/WebCore/Modules/highlight/HighlightRangeGroup.h:34 > + ditto > Source/WebKitLegacy/mac/WebView/WebView.mm:3135 > + RuntimeEnabledFeatures::sharedFeatures().setHighlightAPIEnabled([preferences highlightAPIEnabled]); Windows WebKitLegacy will be running these tests. Do we want to skip all the tests or implement it on Windows?
Comment on attachment 384768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384768&action=review >> Source/WebKitLegacy/mac/WebView/WebView.mm:3135 >> + RuntimeEnabledFeatures::sharedFeatures().setHighlightAPIEnabled([preferences highlightAPIEnabled]); > > Windows WebKitLegacy will be running these tests. Do we want to skip all the tests or implement it on Windows? Is there much we need to do to implement on Windows? This is mostly work in WebCore.
Created attachment 384779 [details] Patch
Comment on attachment 384779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384779&action=review > Source/WebCore/Modules/highlight/HighlightRangeGroup.idl:31 > + iterable<StaticRange>; // FIXME: should be setlike Use a single space?
Comment on attachment 384779 [details] Patch Clearing flags on attachment: 384779 Committed r253093: <https://trac.webkit.org/changeset/253093>
All reviewed patches have been landed. Closing bug.