WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(45.03 KB, patch)
2019-12-03 16:55 PST
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(45.65 KB, patch)
2019-12-03 18:41 PST
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Megan Gardner
Comment 1
2019-12-03 15:05:11 PST
Created
attachment 384762
[details]
Patch
Radar WebKit Bug Importer
Comment 2
2019-12-03 15:06:04 PST
<
rdar://problem/57605049
>
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
Created
attachment 384768
[details]
Patch
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
Created
attachment 384779
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug