Bug 227141

Summary: AppHighlights disappear on page reload
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, esprehn+autocc, ews-watchlist, hi, kangil.han, thorton, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Megan Gardner 2021-06-17 12:36:51 PDT 
AppHighlights disappear on page reload
Comment 1 Megan Gardner 2021-06-17 12:43:03 PDT 
Created attachment 431706 [details]
Patch
Comment 2 Megan Gardner 2021-06-17 12:43:43 PDT 
rdar://78190331
Comment 3 Chris Dumez 2021-06-17 12:46:53 PDT 
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 ?
Comment 4 Wenson Hsieh 2021-06-17 12:49:05 PDT 
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.
Comment 5 Devin Rousso 2021-06-17 12:53:12 PDT 
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?
Comment 6 Wenson Hsieh 2021-06-17 15:30:25 PDT 
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.
Comment 7 Megan Gardner 2021-06-17 16:34:58 PDT 
Created attachment 431739 [details]
Patch
Comment 8 Megan Gardner 2021-06-21 12:49:05 PDT 
Created attachment 431896 [details]
Patch for landing
Comment 9 Devin Rousso 2021-06-21 12:53:02 PDT 
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`
Comment 10 EWS 2021-06-21 13:44:57 PDT 
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].