Allow AppHighlight visibility to be toggled
Created attachment 427519 [details] Patch
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 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()`.
Created attachment 427521 [details] Patch
Created attachment 427602 [details] Patch
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 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)`
Created attachment 427611 [details] Patch
rdar://77410861
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()) ```
Created attachment 427637 [details] Patch for landing
rdar://77351075
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].