Bug 225276 - Allow AppHighlight visibility to be toggled
Summary: Allow AppHighlight visibility to be toggled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-01 21:54 PDT by Megan Gardner
Modified: 2021-05-03 22:54 PDT (History)
10 users (show)

See Also:


Attachments
Patch (16.06 KB, patch)
2021-05-01 21:59 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (16.48 KB, patch)
2021-05-02 00:48 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (16.87 KB, patch)
2021-05-03 14:22 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (16.78 KB, patch)
2021-05-03 16:02 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (22.17 KB, patch)
2021-05-03 22:15 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2021-05-01 21:54:25 PDT
Allow AppHighlight visibility to be toggled
Comment 1 Megan Gardner 2021-05-01 21:59:56 PDT
Created attachment 427519 [details]
Patch
Comment 2 Wenson Hsieh 2021-05-01 22:36:06 PDT
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?
Comment 3 Wenson Hsieh 2021-05-01 22:44:27 PDT
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()`.
Comment 4 Megan Gardner 2021-05-02 00:48:19 PDT
Created attachment 427521 [details]
Patch
Comment 5 Megan Gardner 2021-05-03 14:22:43 PDT
Created attachment 427602 [details]
Patch
Comment 6 Tim Horton 2021-05-03 14:38:01 PDT
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?
Comment 7 Wenson Hsieh 2021-05-03 15:04:49 PDT
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)`
Comment 8 Megan Gardner 2021-05-03 16:02:42 PDT
Created attachment 427611 [details]
Patch
Comment 9 Wenson Hsieh 2021-05-03 17:21:21 PDT
rdar://77410861
Comment 10 Wenson Hsieh 2021-05-03 17:22:30 PDT
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())
```
Comment 11 Megan Gardner 2021-05-03 22:15:44 PDT
Created attachment 427637 [details]
Patch for landing
Comment 12 Megan Gardner 2021-05-03 22:18:47 PDT
rdar://77351075
Comment 13 EWS 2021-05-03 22:54:23 PDT
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].