Bug 204809 – Add disabled highlight API skeleton

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Add disabled highlight API skeleton

[Megan Gardner]

Created attachment 384762 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/57605049>

[Tim Horton]

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 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 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]

Created attachment 384768 [details]
Patch

[Alex Christensen]

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 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]

Created attachment 384779 [details]
Patch

[Ryosuke Niwa]

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 on attachment 384779 [details]
Patch

Clearing flags on attachment: 384779

Committed r253093: <https://trac.webkit.org/changeset/253093>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.