[webkit-reviews] review granted: [Bug 226794] Upstream AppHighlight Observer code : [Attachment 430914] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 226794: Upstream AppHighlight Observer code
https://bugs.webkit.org/show_bug.cgi?id=226794

Attachment 430914: Patch

https://bugs.webkit.org/attachment.cgi?id=430914&action=review




--- Comment #2 from Tim Horton <thorton at apple.com> ---
Comment on attachment 430914
  --> https://bugs.webkit.org/attachment.cgi?id=430914
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430914&action=review

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:70
> +#import <Synapse/SYNotesActivationObserver.h>

This will eventually need an SPI header

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:73
> +SOFT_LINK_CLASS_OPTIONAL(Synapse, SYNotesActivationObserver)

Oddly we don't really seem to deal with the optional-ness elegantly? Maybe it's
OK because alloc returns nil and then -isVisible on nil returns NO and we
updateAppHighlightsVisibility with NO? But it would be nice if it were an early
return instead.

> Source/WebKit/UIProcess/WebPageProxy.h:3047
> +    void setUpHighlightsObserver();

There shouldn't be random methods in amongst the members, move this above.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9325
> +	   if ([menuItem action] ==
@selector(createHighlightInCurrentGroupWithRange:))
> +	       return;

Should this be checking for /either/ selector?