[webkit-reviews] review granted: [Bug 220633] [macOS] Titlebar separator doesn't show when WKWebView is scrolled : [Attachment 428907] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Aditya Keerthi <akeerthi at apple.com>'s
request for review:
Bug 220633: [macOS] Titlebar separator doesn't show when WKWebView is scrolled
https://bugs.webkit.org/show_bug.cgi?id=220633

Attachment 428907: Patch

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




--- Comment #22 from Darin Adler <darin at apple.com> ---
Comment on attachment 428907
  --> https://bugs.webkit.org/attachment.cgi?id=428907
Patch

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

> Source/WebKit/UIProcess/API/mac/WKView.mm:73
> + at interface WKView() <NSScrollViewSeparatorTrackingAdapter>

Normally we put a space before the category name, so "WKView ()".

Does declaring it this way result in checking that we remember to implement all
the methods below? If not, then maybe we should consider a named category
instead so that’s checked for us.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.h:152
> +#if HAVE(NSSCROLLVIEW_SEPARATOR_TRACKING_ADAPTER)
> +- (BOOL)_web_registerScrollViewSeparatorTrackingAdapter;
> +- (void)_web_unregisterScrollViewSeparatorTrackingAdapter;
> +#endif

I don’t think these methods are needed. I think the calling code can call
-[NSWindow registerScrollViewSeparatorTrackingAdapter:] and -[NSWindow
unregisterScrollViewSeparatorTrackingAdapter:]. No need to add this
specially-named method to have the object make the method call.

If we are worried about robustness and not registering a view that might not
implement the protocol, we can check conformsToProtocol: explicitly.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1825
> +    [m_view didChangeValueForKey:@"scrollViewFrame"];

This seems to always claim it’s changed even if no change occurred. Is it OK to
do it that way? Why not send the notification only if something changes,
instead?

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2329
> +	   if ([m_view
respondsToSelector:@selector(_web_unregisterScrollViewSeparatorTrackingAdapter)
]) {

I think we don’t need this check. If the view doesn’t respond to this selector,
then m_isRegisteredScrollViewSeparatorTrackingAdapter won’t be true.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2417
> +    updateTitlebarAdjacencyState();

Why isn’t didChangeValueForKey:@"scrollViewFrame" needed here?

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2426
> +    updateTitlebarAdjacencyState();

Ditto.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2439
> +    if ((scrollPosition.y() > 0 && m_pageIsScrolledToTop) ||
(scrollPosition.y() <= 0 && !m_pageIsScrolledToTop)) {

This should use != rather than the two boolean checks. Something like this
(although I have have gotten the logic backwards.

    if ((scrollPosition.y() <= 0) != m_pageIsScrolledToTop) {

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2461
> +    BOOL wantsScrollViewSeparatorTrackingAdapterRegistration = false;

In C++ files we should probably use bool instead of BOOL.

I would name this "need" instead of "wants" or "should register". In fact in
this short function I think I would literally name this "shouldRegister".
Context tells the reader what we should register: "this view as a scroll view
separator tracking adapter".

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2463
> +    NSWindow *window = [m_view window];

We don’t need this in a local variable. It’s only used once.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2465
> +    NSRect windowContentLayoutRectInSelf = [m_view convertRect:[window
contentLayoutRect] fromView:nil];

We don’t need this whole rectangle in a local variable. Instead we should just
put the NSMinY in the variable. Could give it an easier to understand name.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2466
> +    BOOL topOfWindowContentLayoutRectAdjacent = (NSMinY([m_view bounds]) <=
NSMinY(windowContentLayoutRectInSelf));

Not sure we need the outer parentheses here.

The idea of using the bounds and converting the content layout rectangle to
self doesn’t seem logical. Seems more logical to use frame and convert the
contentLayoutRect to the superview coordinate system. The frame is how we think
of the rectangle in the context of abutting other views. The bounds is more how
we think of the rectangle within our own drawing logic. On the other hand, this
logic is likely correct and changing it might just mess the logic up.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2473
> +	   if ([m_view
respondsToSelector:@selector(_web_registerScrollViewSeparatorTrackingAdapter)])
> +	       m_isRegisteredScrollViewSeparatorTrackingAdapter = [m_view
_web_registerScrollViewSeparatorTrackingAdapter];

I would suggest using && instead of an if statement here.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2475
> +	   if ([m_view
respondsToSelector:@selector(_web_unregisterScrollViewSeparatorTrackingAdapter)
]) {

Since we already checked m_isRegisteredScrollViewSeparatorTrackingAdapter, this
respondsToSelector: check isn’t needed or helpful. If the view responds to
_web_registerScrollViewSeparatorTrackingAdapter but not
_web_unregisterScrollViewSeparatorTrackingAdapter, then we have something truly
strange going on.