[webkit-reviews] review denied: [Bug 211868] [PlayStation] Add minimal WKView API to enable TestWebKitAPI : [Attachment 399831] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Yoshiaki Jitsukawa
<yoshiaki.jitsukawa at sony.com>'s request for review:
Bug 211868: [PlayStation] Add minimal WKView API to enable TestWebKitAPI
https://bugs.webkit.org/show_bug.cgi?id=211868

Attachment 399831: Patch

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




--- Comment #10 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 399831
  --> https://bugs.webkit.org/attachment.cgi?id=399831
Patch

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

better

>
Source/WebKit/UIProcess/API/C/playstation/WKContextConfigurationPlayStation.cpp
:58
> +    using namespace WebKit;

This seems excessive just to avoid WebKit:: on one line.

>
Source/WebKit/UIProcess/API/C/playstation/WKContextConfigurationPlayStation.h:4
1
> +WK_EXPORT void WKContextConfigurationSetUserId(WKContextConfigurationRef
configuration, int32_t userId);

I'm not convinced this should be on the context.  Why not the page?  What
exactly is the user id?  why is it signed?

> Source/WebKit/UIProcess/API/C/playstation/WKPagePrivatePlayStation.h:35
> +WK_EXPORT void WKPageSetSize(WKPageRef page, WKSize size);

Let's not add this if it's not implemented.

> Source/WebKit/UIProcess/API/C/playstation/WKView.cpp:33
> +WKViewRef WKViewCreate(WKPageConfigurationRef configuration)

I'm not crazy about having "WKView" be the base of this name, but it is
WKViewRef, so that might not be the end of the world.

> Source/WebKit/UIProcess/playstation/WebView.cpp:41
> +RefPtr<WebView> WebView::create(const API::PageConfiguration& configuration)

Could we name this something other than "WebView"?  PlayStationWebView maybe? 
That way it'll get less confused with our other classes named WebView.

> Source/WebKit/UIProcess/playstation/WebView.h:41
> +class WebView : public API::ObjectImpl<API::Object::Type::View> {

Having a API::Object::Type::View and API::Object::Type::Page being separate
objects is something we want to merge into one object once we can, so this may
substantially change shape in the future.  I'm ok with adding this for now, but
be aware that this will need to change in the next few years.