Bug 50898 – Change the WebKit2 public API so there is no explicit WKPageNamespace object

• Component: WebKit2
• Status: RESOLVED FIXED

[Sam Weinig]

Lets get rid of WKPageNamespace and replace it with the ability to create a view that is forced into the same namespace as another page.

[Sam Weinig]

Created attachment 76339 [details]
WIP (Do not review)

[Sam Weinig]

<rdar://problem/8253496>

[Early Warning System Bot]

Attachment 76339 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6984081

[Sam Weinig]

Created attachment 76341 [details]
Patch

[Early Warning System Bot]

Attachment 76341 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7005066

Comment on attachment 76341 [details]
Patch

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

> WebKit2/ChangeLog:24
> +        all callers of it will have their views end up in the same shared process, where
> +        as with WKViewCreate, a new internal page namespace will be created and could

whereas?

> WebKit2/ChangeLog:49
> +        Add new initializers for the objective-c WKView. It follows the same

Objective-C

> WebKit2/UIProcess/WebContext.cpp:238
> +    if (m_sharedNamespace)
> +        return m_sharedNamespace.get();
> +
> +    m_sharedNamespace = createPageNamespace();
> +    return m_sharedNamespace.get();

A shorter way to write this is:
if (!m_sharedNamespace)
    m_sharedNamespace = createPageNamespace();

return m_sharedNamespace.get();

> WebKit2/UIProcess/API/C/win/WKView.h:40
> +WK_EXPORT WKViewRef WKViewCreateForAssociatedPage(RECT rect, WKPageRef page, WKPageGroupRef pageGroup, HWND parentWindow);

I think a WKPageRef-based create API is an unnecessary convenience. This one is also awkwardly named. Is calling this different from calling WKViewCreate() passing the context of the page? If so, and three different WKViewCreate…() functions are needed, I suggest merging them into one and adding an options parameter.

> WebKit2/UIProcess/API/mac/WKView.h:44
> -- (id)initWithFrame:(NSRect)frame pageNamespaceRef:(WKPageNamespaceRef)pageNamespaceRef;
> -- (id)initWithFrame:(NSRect)frame pageNamespaceRef:(WKPageNamespaceRef)pageNamespaceRef pageGroupRef:(WKPageGroupRef)pageGroupRef;
> +- (id)initWithFrame:(NSRect)frame contextRef:(WKContextRef)contextRef;
> +- (id)initWithFrame:(NSRect)frame contextRef:(WKContextRef)contextRef pageGroupRef:(WKPageGroupRef)pageGroupRef;
>  
> -- (WKPageRef)pageRef;
> +- (id)initWithFrame:(NSRect)frame contextRef:(WKContextRef)contextRef usingSharedProcess:(BOOL)usingSharedProcess;
> +- (id)initWithFrame:(NSRect)frame contextRef:(WKContextRef)contextRef pageGroupRef:(WKPageGroupRef)pageGroupRef usingSharedProcess:(BOOL)usingSharedProcess;;
>  
> -- (void)setDrawsBackground:(BOOL)flag;
> -- (BOOL)drawsBackground;
> +- (id)initWithFrame:(NSRect)frame forAssociatedPageRef:(WKPageRef)pageRef;
> +- (id)initWithFrame:(NSRect)frame forAssociatedPageRef:(WKPageRef)pageRef pageGroupRef:(WKPageGroupRef)pageGroupRef;
>  

Similar comments about the Objective-C API.

[Sam Weinig]

Created attachment 76342 [details]
Updated Patch

[Sam Weinig]

> > WebKit2/UIProcess/API/C/win/WKView.h:40
> > +WK_EXPORT WKViewRef WKViewCreateForAssociatedPage(RECT rect, WKPageRef page, WKPageGroupRef pageGroup, HWND parentWindow);
> 
> I think a WKPageRef-based create API is an unnecessary convenience. This one is also awkwardly named. Is calling this different from calling WKViewCreate() passing the context of the page? If so, and three different WKViewCreate…() functions are needed, I suggest merging them into one and adding an options parameter.

The associate page create function is not a convenience, it is the only way ensure that two views have their WebProcess counterparts in the same WebProcess.  It should be used in cases like one page calling window.open().  I am not sure how we can merge these into one function, can you explain what its signature would be?

[Early Warning System Bot]

Attachment 76342 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6895068

(In reply to comment #8)
> > > WebKit2/UIProcess/API/C/win/WKView.h:40
> > > +WK_EXPORT WKViewRef WKViewCreateForAssociatedPage(RECT rect, WKPageRef page, WKPageGroupRef pageGroup, HWND parentWindow);
> > 
> > I think a WKPageRef-based create API is an unnecessary convenience. This one is also awkwardly named. Is calling this different from calling WKViewCreate() passing the context of the page? If so, and three different WKViewCreate…() functions are needed, I suggest merging them into one and adding an options parameter.
> 
> The associate page create function is not a convenience, it is the only way ensure that two views have their WebProcess counterparts in the same WebProcess.  It should be used in cases like one page calling window.open().  I am not sure how we can merge these into one function, can you explain what its signature would be?

I see now why a separate WKPageRef-based API is needed.

[Csaba Osztrogonác]

Comment on attachment 76342 [details]
Updated Patch

r-, because Qt build is still broken. I'm going to check what the problem is, and will help you to fix it.

[Sam Weinig]

Created attachment 76436 [details]
Update Patch

Does the Qt bot stop after the first error?

[Csaba Osztrogonác]

(In reply to comment #12)
> Does the Qt bot stop after the first error?
Unfortunately yes.

But I started a clean build locally with your patch.
It will be finished ~6-8 min, and then I'll ping you.

[Sam Weinig]

(In reply to comment #13)
> (In reply to comment #12)
> > Does the Qt bot stop after the first error?
> Unfortunately yes.
> 
> But I started a clean build locally with your patch.
> It will be finished ~6-8 min, and then I'll ping you.

Thanks.

[Csaba Osztrogonác]

Comment on attachment 76436 [details]
Update Patch

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

> WebKitTools/WebKitTestRunner/qt/PlatformWebViewQt.cpp:65
> +PlatformWebView(WKContextRef, WKPageGroupRef);
> +PlatformWebView(WKPageRef, WKPageGroupRef);
> +

Please remove these useless lines, and Qt build will happy. ;)
rs+=me to land the previous patch with this Qt file change. (Of course r=Anders)

[Sam Weinig]

(In reply to comment #15)
> (From update of attachment 76436 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76436&action=review
> 
> > WebKitTools/WebKitTestRunner/qt/PlatformWebViewQt.cpp:65
> > +PlatformWebView(WKContextRef, WKPageGroupRef);
> > +PlatformWebView(WKPageRef, WKPageGroupRef);
> > +
> 
> Please remove these useless lines, and Qt build will happy. ;)
> rs+=me to land the previous patch with this Qt file change. (Of course r=Anders)

Thanks, landed in r73965.