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

Megan Gardner
Reported 2021-06-17 12:36:51 PDT
AppHighlights disappear on page reload
Attachments
Patch (10.03 KB, patch)
2021-06-17 12:43 PDT, Megan Gardner 		no flags
DetailsFormatted DiffDiff
Patch (10.90 KB, patch)
2021-06-17 16:34 PDT, Megan Gardner 		no flags
DetailsFormatted DiffDiff
Patch for landing (10.89 KB, patch)
2021-06-21 12:49 PDT, Megan Gardner 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Megan Gardner
Comment 1 2021-06-17 12:43:03 PDT
Created attachment 431706 [details] Patch
Megan Gardner
Comment 2 2021-06-17 12:43:43 PDT
rdar://78190331
Chris Dumez
Comment 3 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 ?
Wenson Hsieh
Comment 4 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.
Devin Rousso
Comment 5 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?
Wenson Hsieh
Comment 6 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.
Megan Gardner
Comment 7 2021-06-17 16:34:58 PDT
Created attachment 431739 [details] Patch
Megan Gardner
Comment 8 2021-06-21 12:49:05 PDT
Created attachment 431896 [details] Patch for landing
Devin Rousso
Comment 9 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`
EWS
Comment 10 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].
Note You need to log in before you can comment on or make changes to this bug.