Bug 227141 – AppHighlights disappear on page reload

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

AppHighlights disappear on page reload

[Megan Gardner]

Created attachment 431706 [details]
Patch

[Megan Gardner]

rdar://78190331

[Chris Dumez]

Comment on attachment 431706 [details]
Patch

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

> Source/WebCore/page/ChromeClient.h:246
> +    virtual WebCore::HighlightVisibility appHighlightsVisiblility() const { return WebCore::HighlightVisibility::Hidden; };

We're in WebCore, why the "WebCore::"s ?

[Wenson Hsieh]

Comment on attachment 431706 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:538
> +#if ENABLE(APP_BOUND_DOMAINS)

I think you meant to use ENABLE(APP_HIGHLIGHTS) here.

[Devin Rousso]

Comment on attachment 431706 [details]
Patch

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

r=me

> Source/WebCore/page/ChromeClient.h:65
> +#include "HighlightVisibility.h"

Is it really necessary to wrap this in an `#if`?  You could just always include it and remove the forward-declare below.

> Source/WebKit/Shared/WebPageCreationParameters.h:261
> +    WebCore::HighlightVisibility appHighlightsVisible { WebCore::HighlightVisibility::Hidden };

Does this need to be set in the UIProcess (e.g. `WebPageProxy::creationParameters`)?  If not, is it set somewhere else?

[Wenson Hsieh]

Comment on attachment 431706 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.h:2358
> +    WebCore::HighlightVisibility m_appHighlightsVisible { false };

Nit - this should be initialized to `WebCore::HighlightVisibility::Hidden` instead of `false` too.

[Megan Gardner]

Created attachment 431739 [details]
Patch

[Megan Gardner]

Created attachment 431896 [details]
Patch for landing

[Devin Rousso]

Comment on attachment 431896 [details]
Patch for landing

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

> Source/WebKit/Shared/WebPageCreationParameters.h:41
> +#include <WebCore/HighlightVisibility.h>

NIT: I'd also add this include in `WebProcess/WebCoreSupport/WebChromeClient.h` and `Source/WebKit/WebProcess/WebPage/WebPage.h`

> Source/WebKit/WebProcess/WebPage/WebPage.h:2358
> +    WebCore::HighlightVisibility m_appHighlightsVisible { false };

s/`false`/`WebCore::HighlightVisibility::Hidden`

[EWS]

Committed r279078 (238997@main): <https://commits.webkit.org/238997@main>

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