Bug 219365 – Create and draw app highlights

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Create and draw app highlights

[Megan Gardner]

Created attachment 415058 [details]
Patch

[Wenson Hsieh]

Comment on attachment 415058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415058&action=review

> Source/WebCore/dom/Document.cpp:2784
> +void Document::collectRageDataFromRegister(Vector<WeakPtr<HighlightRangeData>>& rangesData, RefPtr<HighlightRegister> highlightRegister)

Nit - `collectRageDataFromRegister`

[Peng Liu]

Comment on attachment 415058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415058&action=review

> Source/WebCore/rendering/InlineTextBox.cpp:849
> +    case MarkedText::AppHighlight: {

Nit. Do we need the "{" here? Seems inconsistent with other cases.

> Source/WebCore/rendering/RenderThemeMac.mm:481
> +    LocalDefaultSystemAppearance localAppearance(options.contains(StyleColor::Options::UseDarkAppearance));

What's the purpose of this local variable?

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:553
> +

Nit. This empty line can be removed.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7056
> +

Nit. This empty line can be removed.

[Megan Gardner]

Comment on attachment 415058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415058&action=review

>> Source/WebCore/rendering/InlineTextBox.cpp:849
>> +    case MarkedText::AppHighlight: {
> 
> Nit. Do we need the "{" here? Seems inconsistent with other cases.

Yes, the cases that this is like are lower down, like MarkedText::TextMatch and MarkedText::Selection.

>> Source/WebCore/rendering/RenderThemeMac.mm:481
>> +    LocalDefaultSystemAppearance localAppearance(options.contains(StyleColor::Options::UseDarkAppearance));
> 
> What's the purpose of this local variable?

It switches the state to dark appearance if it is set. This is very similar to the other color calculation functions above and below.

[Megan Gardner]

Created attachment 415075 [details]
Patch

[Tim Horton]

Comment on attachment 415058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415058&action=review

>>> Source/WebCore/rendering/InlineTextBox.cpp:849
>>> +    case MarkedText::AppHighlight: {
>> 
>> Nit. Do we need the "{" here? Seems inconsistent with other cases.
> 
> Yes, the cases that this is like are lower down, like MarkedText::TextMatch and MarkedText::Selection.

(It's needed because of the local variable, and indeed this is the "normal" place to put it)

[Megan Gardner]

Created attachment 415081 [details]
Patch

[Megan Gardner]

Created attachment 415084 [details]
Patch

[Megan Gardner]

Created attachment 415085 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 415085 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415085&action=review

> Source/WebCore/Modules/highlight/HighlightRegister.cpp:56
> +ASCIILiteral HighlightRegister::appHighlightKey()

"app" is a bit vague here. It's more like "built-in highlights" or "client highlights" where "client" is the client of WebKit.

> Source/WebCore/Modules/highlight/HighlightRegister.cpp:66
> +        m_map.get(appHighlightKey())->addToSetLike(value);
> +    else
> +        setFromMapLike(appHighlightKey(), Highlight::create(value));

WTFMove(value)?

> Source/WebCore/Modules/highlight/HighlightRegister.h:39
>  class HighlightRegister : public RefCounted<HighlightRegister> {

Should this be a Registry, rather than a Register?

> Source/WebCore/css/CSSValueKeywords.in:262
> +-apple-system-app-highlight-background

built-in-highlight? Not sure.

> Source/WebCore/dom/Document.cpp:2777
> +HighlightRegister& Document::appHighlightRegister()

registry?

> Source/WebCore/dom/Document.cpp:2784
> +void Document::collectRangeDataFromRegister(Vector<WeakPtr<HighlightRangeData>>& rangesData, RefPtr<HighlightRegister> highlightRegister)

Pass the register as a const HighlightRegister& ?

> Source/WebCore/rendering/InlineTextBox.cpp:1078
> +    for (auto& highlight : renderer().document().appHighlightRegister().map()) {

Isn't this going to create the appHighlightRegister even when there are no app highlights?

> Source/WebKit/UIProcess/WebPageProxy.h:1827
> +    void createAppHighlightInSelectedRange(bool);

Use an enum instead of the bool. Can't tell what the bool means here.

[Megan Gardner]

Comment on attachment 415085 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415085&action=review

>> Source/WebCore/Modules/highlight/HighlightRegister.cpp:56
>> +ASCIILiteral HighlightRegister::appHighlightKey()
> 
> "app" is a bit vague here. It's more like "built-in highlights" or "client highlights" where "client" is the client of WebKit.

App Highlight is what I have been using for everything already, so changing the name now I fear would be more confusing. If Everything needs to switch, I'll do it in a followup patch.

>> Source/WebCore/Modules/highlight/HighlightRegister.h:39
>>  class HighlightRegister : public RefCounted<HighlightRegister> {
> 
> Should this be a Registry, rather than a Register?

Not according to the spec.
https://www.w3.org/TR/2020/WD-css-highlight-api-1-20201022/

>> Source/WebCore/dom/Document.cpp:2777
>> +HighlightRegister& Document::appHighlightRegister()
> 
> registry?

Same as above.

>> Source/WebCore/rendering/InlineTextBox.cpp:1078
>> +    for (auto& highlight : renderer().document().appHighlightRegister().map()) {
> 
> Isn't this going to create the appHighlightRegister even when there are no app highlights?

True. Do we have a preferred pattern for checking if lazily made objects exist?

[Megan Gardner]

Created attachment 415152 [details]
Patch

[Tim Horton]

(In reply to Megan Gardner from comment #11)
> Comment on attachment 415085 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415085&action=review
> >> Source/WebCore/rendering/InlineTextBox.cpp:1078
> >> +    for (auto& highlight : renderer().document().appHighlightRegister().map()) {
> > 
> > Isn't this going to create the appHighlightRegister even when there are no app highlights?
> 
> True. Do we have a preferred pattern for checking if lazily made objects
> exist?

usually appHighlighterRegister() would return a nullable ptr, and ensureAppHighlighterRegister() (is it ensure we ended up with? I'm not sure, look around) constructs it if needed and returns a ref.

[Wenson Hsieh]

(In reply to Tim Horton from comment #13)
> (In reply to Megan Gardner from comment #11)
> > Comment on attachment 415085 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=415085&action=review
> > >> Source/WebCore/rendering/InlineTextBox.cpp:1078
> > >> +    for (auto& highlight : renderer().document().appHighlightRegister().map()) {
> > > 
> > > Isn't this going to create the appHighlightRegister even when there are no app highlights?
> > 
> > True. Do we have a preferred pattern for checking if lazily made objects
> > exist?
> 
> usually appHighlighterRegister() would return a nullable ptr, and
> ensureAppHighlighterRegister() (is it ensure we ended up with? I'm not sure,
> look around) constructs it if needed and returns a ref.

I vaguely recall some folks on webkit-dev decided that the preferred terminology is foo() and fooIfExists().

[Megan Gardner]

Created attachment 415180 [details]
Patch

[Megan Gardner]

Created attachment 415183 [details]
Patch

[Wenson Hsieh]

Comment on attachment 415183 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415183&action=review

> Source/WebCore/rendering/RenderReplaced.cpp:157
> +                return theme().appHighlightColor( styleColorOptions);

Nit - extra space before `styleColorOptions` here.

> Source/WebCore/rendering/RenderThemeIOS.mm:1300
> +            { CSSValueAppleSystemYellow, @selector(systemYellowColor) },

Nit - do we need this change?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:82
> +#import "WebPage.h"

We should probably refrain from importing `WebPage.h` in the UI process, since that will pull in things like DOM nodes, elements, etc. along with it.

If we just need CreateNewGroupForHighlight, I'd imagine we can just include "CreateNewGroupForHighlight.h".

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8619
> +    _page->createAppHighlightInSelectedRange(CreateNewGroupForHighlight::NO);

Does this actually compile when ENABLE(APP_HIGHLIGHTS) is on?

I imagine this should be WebKit::CreateNewGroupForHighlight::No, and you might need to #import "CreateNewGroupForHighlight.h"

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8624
> +    _page->createAppHighlightInSelectedRange(CreateNewGroupForHighlight::YES);

(Ditto)

[Wenson Hsieh]

View in context: https://bugs.webkit.org/attachment.cgi?id=415183&action=review

> Source/WebCore/rendering/RenderReplaced.cpp:157
> +                return theme().appHighlightColor( styleColorOptions);

Nit - extra space before `styleColorOptions` here.

> Source/WebCore/rendering/RenderThemeIOS.mm:1300
> +            { CSSValueAppleSystemYellow, @selector(systemYellowColor) },

Nit - do we need this change?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:82
> +#import "WebPage.h"

We should probably refrain from importing `WebPage.h` in the UI process, since that will pull in things like DOM nodes, elements, etc. along with it.

If we just need CreateNewGroupForHighlight, I'd imagine we can just include "CreateNewGroupForHighlight.h".

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8619
> +    _page->createAppHighlightInSelectedRange(CreateNewGroupForHighlight::NO);

Does this actually compile when ENABLE(APP_HIGHLIGHTS) is on?

I imagine this should be WebKit::CreateNewGroupForHighlight::No, and you might need to #import "CreateNewGroupForHighlight.h"

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8624
> +    _page->createAppHighlightInSelectedRange(CreateNewGroupForHighlight::YES);

(Ditto)

[Wenson Hsieh]

Comment on attachment 415183 [details]
Patch

LGTM with the above comments, though I do agree that "builtin highlights" would be more appropriate (we can tackle that in a followup, I suppose).

I think it would also be more ideal to make the builtin highlights registry available everywhere, since that provides a generic mechanism for rendering builtin (non-bindings-exposed) highlights, and then have the actual feature flag guard the specific user-facing feature that this is being added to support.

[Megan Gardner]

Created attachment 415241 [details]
Patch for landing

[EWS]

Committed r270362: <https://trac.webkit.org/changeset/270362>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415241 [details].

[Radar WebKit Bug Importer]

<rdar://problem/71905388>

[Wenson Hsieh]

Comment on attachment 415241 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=415241&action=review

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8619
> +    _page->createAppHighlightInSelectedRange(WebKit::CreateNewGroupForHighlight::NO);

It looks like this is still NO/YES.