Bug 227141 - AppHighlights disappear on page reload
Summary: AppHighlights disappear on page reload
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-06-17 12:36 PDT by Megan Gardner
Modified: 2021-06-21 13:44 PDT (History)
7 users (show)

See Also:

Attachments
Patch (10.03 KB, patch)
2021-06-17 12:43 PDT, Megan Gardner 		no flags Details | Formatted Diff | Diff
Patch (10.90 KB, patch)
2021-06-17 16:34 PDT, Megan Gardner 		no flags Details | Formatted Diff | Diff
Patch for landing (10.89 KB, patch)
2021-06-21 12:49 PDT, Megan Gardner 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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].