Bug 66886 – [Qt][WK2] Remove QWKContext class and make sure the WebProcessProxy life is correctly managed.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alexis Menard (darktears)]

[Qt][WK2] Remove QWKContext class and make sure the WebContext life is correctly managed.

[Alexis Menard (darktears)]

Created attachment 105057 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 105057 [details]
Patch

jocelyn did the QWKContext class, he might have comments

[Andreas Kling]

Comment on attachment 105057 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        Remove QWKContext class from Qt APIs as it doesn't bring that much value. QtWebPageProxy now holds

I don't agree that QWKContext brings no value, it was just way too low-level for the current Qt5 API vision. :)

> Source/WebKit2/UIProcess/qt/ClientImpl.cpp:153
> -    emit toQWKContext(clientInfo)->iconChangedForPageURL(qUrl);
> +    // FIXME : emit toQWKContext(clientInfo)->iconChangedForPageURL(qUrl);

I'd rather you remove this function altogether.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:63
> +RefPtr<WebContext> QtWebPageProxy::defaultContext;

I'm not 100% sure about this. This means that defaultWKContext() is unsafe to call during static object initialization.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:212
> +    if (m_context == defaultContext && m_context->refCount() == 2)
> +        defaultContext = 0;

This looks very fragile. If someone adds another ref somewhere in the future, we'll never reset the defaultContext ptr. Can we solve this in another way?

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:198
> +    static RefPtr<WebContext> defaultContext;

I like the s_ prefix for static variables. NABD.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:200
> +    RefPtr<WebContext>  m_context;

Two spaces.

[Alexis Menard (darktears)]

(In reply to comment #3)
> (From update of attachment 105057 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105057&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        Remove QWKContext class from Qt APIs as it doesn't bring that much value. QtWebPageProxy now holds
> 
> I don't agree that QWKContext brings no value, it was just way too low-level for the current Qt5 API vision. :)

Ok ok Andreas don't be too personal about that class, I don't want you to cry maouahahahhaa.

> 
> > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:153
> > -    emit toQWKContext(clientInfo)->iconChangedForPageURL(qUrl);
> > +    // FIXME : emit toQWKContext(clientInfo)->iconChangedForPageURL(qUrl);
> 
> I'd rather you remove this function altogether.

Will do.

> 
> > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:63
> > +RefPtr<WebContext> QtWebPageProxy::defaultContext;
> 
> I'm not 100% sure about this. This means that defaultWKContext() is unsafe to call during static object initialization.

But how this could happen? Like someone nasty use another static which get a defaultWKContext() as a value. static relaying on other static are usually not recommended, or do you have an other use case in mind?

> 
> > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:212
> > +    if (m_context == defaultContext && m_context->refCount() == 2)
> > +        defaultContext = 0;
> 
> This looks very fragile. If someone adds another ref somewhere in the future, we'll never reset the defaultContext ptr. Can we solve this in another way?

Wait let me see what I can improve.

> 
> > Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:198
> > +    static RefPtr<WebContext> defaultContext;
> 
> I like the s_ prefix for static variables. NABD.

Kenneth'd.

> 
> > Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:200
> > +    RefPtr<WebContext>  m_context;
> 
> Two spaces.

Will change.

[Alexis Menard (darktears)]

Created attachment 105167 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 105174 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 105184 [details]
Patch

[Benjamin Poulain]

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

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:219
> +            s_defaultContext = 0;

s_defaultContext.clear() or = nullptr for clarity?

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:72
> +    QtWebPageProxy(WebKit::ViewInterface*, WebKit::PolicyInterface*, WKContextRef, WKPageGroupRef = 0);

WKContextRef = 0 so you can simplify the constructors of the views?

[Benjamin Poulain]

Comment on attachment 105184 [details]
Patch

Check my previous comments before landing.

[Alexis Menard (darktears)]

(In reply to comment #8)
> View in context: https://bugs.webkit.org/attachment.cgi?id=105174&action=review
> 
> > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:219
> > +            s_defaultContext = 0;
> 
> s_defaultContext.clear() or = nullptr for clarity?

ok

> 
> > Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:72
> > +    QtWebPageProxy(WebKit::ViewInterface*, WebKit::PolicyInterface*, WKContextRef, WKPageGroupRef = 0);
> 
> WKContextRef = 0 so you can simplify the constructors of the views?


Fine will do before landing

[Alexis Menard (darktears)]

Committed r93784: <http://trac.webkit.org/changeset/93784>