RESOLVED FIXED 204809
Add disabled highlight API skeleton
https://bugs.webkit.org/show_bug.cgi?id=204809
Summary Add disabled highlight API skeleton
Megan Gardner
Reported 2019-12-03 12:41:01 PST
Add disabled highlight API skeleton
Attachments
Patch (96.43 KB, patch)
2019-12-03 15:05 PST, Megan Gardner
no flags
Patch (45.03 KB, patch)
2019-12-03 16:55 PST, Megan Gardner
no flags
Patch (45.65 KB, patch)
2019-12-03 18:41 PST, Megan Gardner
no flags
Megan Gardner
Comment 1 2019-12-03 15:05:11 PST
Radar WebKit Bug Importer
Comment 2 2019-12-03 15:06:04 PST
Tim Horton
Comment 3 2019-12-03 15:12:33 PST
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.
Ryosuke Niwa
Comment 4 2019-12-03 15:43:07 PST
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?
Wenson Hsieh
Comment 5 2019-12-03 16:01:17 PST
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?
Megan Gardner
Comment 6 2019-12-03 16:55:24 PST
Alex Christensen
Comment 7 2019-12-03 18:02:07 PST
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?
Megan Gardner
Comment 8 2019-12-03 18:28:27 PST
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.
Megan Gardner
Comment 9 2019-12-03 18:41:01 PST
Ryosuke Niwa
Comment 10 2019-12-03 19:24:47 PST
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?
WebKit Commit Bot
Comment 11 2019-12-03 21:48:54 PST
Comment on attachment 384779 [details] Patch Clearing flags on attachment: 384779 Committed r253093: <https://trac.webkit.org/changeset/253093>
WebKit Commit Bot
Comment 12 2019-12-03 21:48:56 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.