Summary: | Allow AppHighlight visibility to be toggled | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||||||
Component: | New Bugs | Assignee: | Megan Gardner <megan_gardner> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | changseok, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, mmaxfield, pdr, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Megan Gardner
2021-05-01 21:54:25 PDT
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
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
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]. |