Bug 225276 – Allow AppHighlight visibility to be toggled

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Allow AppHighlight visibility to be toggled

[Megan Gardner]

Created attachment 427519 [details]
Patch

[Wenson Hsieh]

Comment on attachment 427519 [details]
Patch

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

> Source/WebKit/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

These ChangeLog entries should have at least a brief sentence or two.

> Source/WebCore/Modules/highlight/HighlightRegister.cpp:68
> +void HighlightRegister::setActive(HighlightsActive active)

Nit - maybe a newline before this function definition?

> Source/WebCore/Modules/highlight/HighlightRegister.h:39
> +enum class HighlightsActive : bool { No, Yes };

Maybe `AppHighlightVisibility : bool { Hidden, Visible };` would be more consistent with the rest of the code?

> Source/WebCore/Modules/highlight/HighlightRegister.h:52
> +    HighlightsActive isActive() { return m_active; }

Nit - `AppHighlightVisibility appHighlightVisibility() const { return m_appHighlightVisibility; }`

> Source/WebCore/Modules/highlight/HighlightRegister.h:58
> +    
> +    

Some extra newlines here.

> Source/WebCore/Modules/highlight/HighlightRegister.h:63
> +    HighlightsActive m_active;

Maybe initialize this to a consistent value? (e.g. `AppHighlightVisibility m_appHighlightVisibility { AppHighlightVisibility::Hidden };`)

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:581
> +void WebPageProxy::setAppHighlightsActive(WebCore::HighlightsActive active)

Nit - `setAppHighlightVisibility(AppHighlightVisibility visibility)`

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7528
> +    auto document = makeRefPtr(m_page->focusController().focusedOrMainFrame().document());

Do we just want the focused (or main) frame here? Or should we set visibility in all frames?

[Wenson Hsieh]

Comment on attachment 427519 [details]
Patch

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

>> Source/WebCore/Modules/highlight/HighlightRegister.cpp:68
>> +void HighlightRegister::setActive(HighlightsActive active)
> 
> Nit - maybe a newline before this function definition?

Actually, maybe once the enum reaches `HighlightRegister` it should just be a `bool`, since `HighlightRegister` is used for both CSS highlights and app highlights.

Or alternately, you could make it so that `AppHighlightVisibility` state only affects the entry corresponding to `appHighlightKey()`.

[Megan Gardner]

Created attachment 427521 [details]
Patch

[Megan Gardner]

Created attachment 427602 [details]
Patch

[Tim Horton]

Comment on attachment 427602 [details]
Patch

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

> Source/WebCore/Modules/highlight/Highlight.h:63
> +    void forceRepaint();

There's nothing forceful about this, it should just be called `repaint` like elsewhere

> Source/WebCore/Modules/highlight/HighlightRegister.cpp:69
> +void HighlightRegister::setHighlightsVisibility(HighlightVisibility highlightsVisibility)

plural? singular? I'm not sure.

> Source/WebCore/rendering/InlineTextBox.cpp:915
> +            for (auto& highlight : appHighlightRegister->map()) {

Can this code and the code above be merged?

> Source/WebCore/rendering/RenderReplaced.cpp:152
> +                        continue;

Can this code and the code below be merged?

[Wenson Hsieh]

Comment on attachment 427602 [details]
Patch

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

> Source/WebCore/Modules/highlight/HighlightRegister.h:39
> +enum class HighlightVisibility : bool { Visible, Hidden };

Nit - I think thorton indicated that 0 should probably be the Hidden value.

> Source/WebCore/Modules/highlight/HighlightRegister.h:50
> +    HighlightVisibility highlightsVisibility() { return m_highlightsVisibility; }

Nit - this is still non-const

> Source/WebCore/Modules/highlight/HighlightRegister.h:63
> +    HighlightVisibility m_highlightsVisibility {HighlightVisibility::Hidden};

Nit - spaces around `HighlightVisibility::Hidden`

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7532
> +    auto document = makeRefPtr(m_page->focusController().focusedOrMainFrame().document());

I think you meant to remove this.

> Source/WebKit/WebProcess/WebPage/WebPage.messages.in:632
> +    SetAppHighlightsVisibility(enum:bool WebCore::HighlightVisibility active)

Nit - `SetAppHighlightsVisibility(enum:bool WebCore::HighlightVisibility visibility)`

[Megan Gardner]

Created attachment 427611 [details]
Patch

[Wenson Hsieh]

rdar://77410861

[Wenson Hsieh]

Comment on attachment 427611 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7529
> +        if (auto* document = frame->document())

This should be protected with a RefPtr, like so:

```
auto document = makeRefPtr(frame->document())
```

[Megan Gardner]

Created attachment 427637 [details]
Patch for landing

[Megan Gardner]

rdar://77351075

[EWS]

Committed r276947 (237280@main): <https://commits.webkit.org/237280@main>

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