[webkit-reviews] review granted: [Bug 128130] NavigationState should be a PageLoadState::Observer : [Attachment 223027] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 128130: NavigationState should be a PageLoadState::Observer
https://bugs.webkit.org/show_bug.cgi?id=128130

Attachment 223027: Patch
https://bugs.webkit.org/attachment.cgi?id=223027&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223027&action=review


> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewInternal.h:38
> +#if PLATFORM(IOS)
> +#import "WKContentViewInternal.h"
> +#import <UIKit/UIScrollView_Private.h>
> +#import <UIKit/_UIWebViewportHandler.h>
> +
> +#define WK_WEB_VIEW_PROTOCOLS <UIScrollViewDelegate, WKContentViewDelegate,
_UIWebViewportHandlerDelegate>
> +#endif

The paragraphing here is hard to read. I’d almost prefer two separate if
statements.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewInternal.h:42
> +#if PLATFORM(MAC) && !PLATFORM(IOS)
> +#define WK_WEB_VIEW_PROTOCOLS
> +#endif

Maybe just:

#if !defined(WK_WEB_VIEW_PROTOCOLS)
#define WK_WEB_VIEW_PROTOCOLS
#endif

> Source/WebKit2/UIProcess/Cocoa/NavigationState.h:92
> +    // PageLoadState::Observer
> +    virtual void willChangeIsLoading() override;
> +    virtual void didChangeIsLoading() override;

Without a blank line, this comment seems to be grouped with only these first
two overrides, so I suggest different formatting.

> Source/WebKit2/UIProcess/Cocoa/NavigationState.mm:340
> +    printf("%s!\n", __PRETTY_FUNCTION__);

Do we really want to land this?

> Source/WebKit2/UIProcess/Cocoa/NavigationState.mm:345
> +    printf("%s!\n", __PRETTY_FUNCTION__);

Do we really want to land this?