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

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].