Summary: | Create and draw app highlights | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | Megan Gardner <megan_gardner> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | annulen, cdumez, changseok, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, kangil.han, kondapallykalyan, macpherson, menard, mmaxfield, pdr, peng.liu6, ryuan.choi, sergio, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Megan Gardner
2020-11-30 11:52:08 PST
Created attachment 415058 [details]
Patch
Comment on attachment 415058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415058&action=review > Source/WebCore/dom/Document.cpp:2784 > +void Document::collectRageDataFromRegister(Vector<WeakPtr<HighlightRangeData>>& rangesData, RefPtr<HighlightRegister> highlightRegister) Nit - `collectRageDataFromRegister` Comment on attachment 415058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415058&action=review > Source/WebCore/rendering/InlineTextBox.cpp:849 > + case MarkedText::AppHighlight: { Nit. Do we need the "{" here? Seems inconsistent with other cases. > Source/WebCore/rendering/RenderThemeMac.mm:481 > + LocalDefaultSystemAppearance localAppearance(options.contains(StyleColor::Options::UseDarkAppearance)); What's the purpose of this local variable? > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:553 > + Nit. This empty line can be removed. > Source/WebKit/WebProcess/WebPage/WebPage.cpp:7056 > + Nit. This empty line can be removed. Comment on attachment 415058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415058&action=review >> Source/WebCore/rendering/InlineTextBox.cpp:849 >> + case MarkedText::AppHighlight: { > > Nit. Do we need the "{" here? Seems inconsistent with other cases. Yes, the cases that this is like are lower down, like MarkedText::TextMatch and MarkedText::Selection. >> Source/WebCore/rendering/RenderThemeMac.mm:481 >> + LocalDefaultSystemAppearance localAppearance(options.contains(StyleColor::Options::UseDarkAppearance)); > > What's the purpose of this local variable? It switches the state to dark appearance if it is set. This is very similar to the other color calculation functions above and below. Created attachment 415075 [details]
Patch
Comment on attachment 415058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415058&action=review >>> Source/WebCore/rendering/InlineTextBox.cpp:849 >>> + case MarkedText::AppHighlight: { >> >> Nit. Do we need the "{" here? Seems inconsistent with other cases. > > Yes, the cases that this is like are lower down, like MarkedText::TextMatch and MarkedText::Selection. (It's needed because of the local variable, and indeed this is the "normal" place to put it) Created attachment 415081 [details]
Patch
Created attachment 415084 [details]
Patch
Created attachment 415085 [details]
Patch
Comment on attachment 415085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415085&action=review > Source/WebCore/Modules/highlight/HighlightRegister.cpp:56 > +ASCIILiteral HighlightRegister::appHighlightKey() "app" is a bit vague here. It's more like "built-in highlights" or "client highlights" where "client" is the client of WebKit. > Source/WebCore/Modules/highlight/HighlightRegister.cpp:66 > + m_map.get(appHighlightKey())->addToSetLike(value); > + else > + setFromMapLike(appHighlightKey(), Highlight::create(value)); WTFMove(value)? > Source/WebCore/Modules/highlight/HighlightRegister.h:39 > class HighlightRegister : public RefCounted<HighlightRegister> { Should this be a Registry, rather than a Register? > Source/WebCore/css/CSSValueKeywords.in:262 > +-apple-system-app-highlight-background built-in-highlight? Not sure. > Source/WebCore/dom/Document.cpp:2777 > +HighlightRegister& Document::appHighlightRegister() registry? > Source/WebCore/dom/Document.cpp:2784 > +void Document::collectRangeDataFromRegister(Vector<WeakPtr<HighlightRangeData>>& rangesData, RefPtr<HighlightRegister> highlightRegister) Pass the register as a const HighlightRegister& ? > Source/WebCore/rendering/InlineTextBox.cpp:1078 > + for (auto& highlight : renderer().document().appHighlightRegister().map()) { Isn't this going to create the appHighlightRegister even when there are no app highlights? > Source/WebKit/UIProcess/WebPageProxy.h:1827 > + void createAppHighlightInSelectedRange(bool); Use an enum instead of the bool. Can't tell what the bool means here. Comment on attachment 415085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415085&action=review >> Source/WebCore/Modules/highlight/HighlightRegister.cpp:56 >> +ASCIILiteral HighlightRegister::appHighlightKey() > > "app" is a bit vague here. It's more like "built-in highlights" or "client highlights" where "client" is the client of WebKit. App Highlight is what I have been using for everything already, so changing the name now I fear would be more confusing. If Everything needs to switch, I'll do it in a followup patch. >> Source/WebCore/Modules/highlight/HighlightRegister.h:39 >> class HighlightRegister : public RefCounted<HighlightRegister> { > > Should this be a Registry, rather than a Register? Not according to the spec. https://www.w3.org/TR/2020/WD-css-highlight-api-1-20201022/ >> Source/WebCore/dom/Document.cpp:2777 >> +HighlightRegister& Document::appHighlightRegister() > > registry? Same as above. >> Source/WebCore/rendering/InlineTextBox.cpp:1078 >> + for (auto& highlight : renderer().document().appHighlightRegister().map()) { > > Isn't this going to create the appHighlightRegister even when there are no app highlights? True. Do we have a preferred pattern for checking if lazily made objects exist? Created attachment 415152 [details]
Patch
(In reply to Megan Gardner from comment #11) > Comment on attachment 415085 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=415085&action=review > >> Source/WebCore/rendering/InlineTextBox.cpp:1078 > >> + for (auto& highlight : renderer().document().appHighlightRegister().map()) { > > > > Isn't this going to create the appHighlightRegister even when there are no app highlights? > > True. Do we have a preferred pattern for checking if lazily made objects > exist? usually appHighlighterRegister() would return a nullable ptr, and ensureAppHighlighterRegister() (is it ensure we ended up with? I'm not sure, look around) constructs it if needed and returns a ref. (In reply to Tim Horton from comment #13) > (In reply to Megan Gardner from comment #11) > > Comment on attachment 415085 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=415085&action=review > > >> Source/WebCore/rendering/InlineTextBox.cpp:1078 > > >> + for (auto& highlight : renderer().document().appHighlightRegister().map()) { > > > > > > Isn't this going to create the appHighlightRegister even when there are no app highlights? > > > > True. Do we have a preferred pattern for checking if lazily made objects > > exist? > > usually appHighlighterRegister() would return a nullable ptr, and > ensureAppHighlighterRegister() (is it ensure we ended up with? I'm not sure, > look around) constructs it if needed and returns a ref. I vaguely recall some folks on webkit-dev decided that the preferred terminology is foo() and fooIfExists(). Created attachment 415180 [details]
Patch
Created attachment 415183 [details]
Patch
Comment on attachment 415183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415183&action=review > Source/WebCore/rendering/RenderReplaced.cpp:157 > + return theme().appHighlightColor( styleColorOptions); Nit - extra space before `styleColorOptions` here. > Source/WebCore/rendering/RenderThemeIOS.mm:1300 > + { CSSValueAppleSystemYellow, @selector(systemYellowColor) }, Nit - do we need this change? > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:82 > +#import "WebPage.h" We should probably refrain from importing `WebPage.h` in the UI process, since that will pull in things like DOM nodes, elements, etc. along with it. If we just need CreateNewGroupForHighlight, I'd imagine we can just include "CreateNewGroupForHighlight.h". > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8619 > + _page->createAppHighlightInSelectedRange(CreateNewGroupForHighlight::NO); Does this actually compile when ENABLE(APP_HIGHLIGHTS) is on? I imagine this should be WebKit::CreateNewGroupForHighlight::No, and you might need to #import "CreateNewGroupForHighlight.h" > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8624 > + _page->createAppHighlightInSelectedRange(CreateNewGroupForHighlight::YES); (Ditto) View in context: https://bugs.webkit.org/attachment.cgi?id=415183&action=review > Source/WebCore/rendering/RenderReplaced.cpp:157 > + return theme().appHighlightColor( styleColorOptions); Nit - extra space before `styleColorOptions` here. > Source/WebCore/rendering/RenderThemeIOS.mm:1300 > + { CSSValueAppleSystemYellow, @selector(systemYellowColor) }, Nit - do we need this change? > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:82 > +#import "WebPage.h" We should probably refrain from importing `WebPage.h` in the UI process, since that will pull in things like DOM nodes, elements, etc. along with it. If we just need CreateNewGroupForHighlight, I'd imagine we can just include "CreateNewGroupForHighlight.h". > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8619 > + _page->createAppHighlightInSelectedRange(CreateNewGroupForHighlight::NO); Does this actually compile when ENABLE(APP_HIGHLIGHTS) is on? I imagine this should be WebKit::CreateNewGroupForHighlight::No, and you might need to #import "CreateNewGroupForHighlight.h" > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8624 > + _page->createAppHighlightInSelectedRange(CreateNewGroupForHighlight::YES); (Ditto) Comment on attachment 415183 [details]
Patch
LGTM with the above comments, though I do agree that "builtin highlights" would be more appropriate (we can tackle that in a followup, I suppose).
I think it would also be more ideal to make the builtin highlights registry available everywhere, since that provides a generic mechanism for rendering builtin (non-bindings-exposed) highlights, and then have the actual feature flag guard the specific user-facing feature that this is being added to support.
Created attachment 415241 [details]
Patch for landing
Committed r270362: <https://trac.webkit.org/changeset/270362> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415241 [details]. Comment on attachment 415241 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=415241&action=review > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8619 > + _page->createAppHighlightInSelectedRange(WebKit::CreateNewGroupForHighlight::NO); It looks like this is still NO/YES. |