Bug 75550 – Pass WebFrame instead of WebCore::Frame to the WebKit2 specific FrameNetworkingContext

• Component: WebKit2
• Status: RESOLVED FIXED

[Alexander Færøy]

For the Qt port, we need access to the WebFrame::page()->pageID() to be able to implement HTTP authentication and SSL handling.

[Alexander Færøy]

Created attachment 121114 [details]
Patch

WIP patch. Uploaded to see what the EWS says :-)

[Alexander Færøy]

Adding Simon for review.

Patch worked for me locally on my Mac and it seems to pass EWS :-)

[Kenneth Rohde Christiansen]

Comment on attachment 121114 [details]
Patch

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

Please fix the changelog and bug title

> Source/WebKit2/ChangeLog:4
> +        [Qt] Pass WebKit::WebFrame instead of WebCore::Frame to WebFrameNetworkingContext
> +        https://bugs.webkit.org/show_bug.cgi?id=75550

This is not Qt only, so please remove the Qt part.

Pass WebFrame instead of WebCore::Frame to the WebKit2 specific FrameNetworkingContext

> Source/WebKit2/ChangeLog:9
> +        This patch changes the constructor of the various, port-specific,
> +        WebFrameNetworkingContext-classes to take a pointer to a
> +        WebKit::WebFrame instead of a WebCore::Frame. This is needed for the
> +        Qt port to implement HTTP authentication and SSL support.

You say it is port specific, so why do you need to change that for all ports... could you make this more clear?

Make the WebKit2 FrameNetworkingContext implementation store the WebFrame
instead of the WebCore::Frame as this is needed for the Qt port to implement a.o.
HTTP autentication and SSL Support.

As the WebFrameNetworkingContext is partly shared across all WebKit2 ports, all
port specific files have been modified.

[Kenneth Rohde Christiansen]

Comment on attachment 121114 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/qt/WebFrameNetworkingContext.h:37
> +    ~WebFrameNetworkingContext();

Shouldn't this be virtual?

[Alexander Færøy]

Thanks for the review Kenneth.

I nailed down my related issue to the following bug in Qt: https://gist.github.com/1562660

Will talk with the Qt earth team tomorrow before proceeding with this.

[Alexander Færøy]

Created attachment 121254 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 121254 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/qt/WebFrameNetworkingContext.cpp:37
> +    // The page ID is needed later for HTTP authentication and SSL errors.
> +    m_originatingObject->setProperty("pageID", qulonglong(frame->page()->pageID()));

you do more in your patch than what you advertise

[Alexander Færøy]

Created attachment 121256 [details]
Patch

[Simon Hausmann]

Comment on attachment 121256 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/qt/WebFrameNetworkingContext.cpp:42
> +    m_originatingObject->deleteLater();

Any particular reason for using deleteLater() here?

What about using an OwnPtr for m_originatingObject?

[Alexander Færøy]

Created attachment 121264 [details]
Patch

[Alexander Færøy]

Comment on attachment 121264 [details]
Patch

Removing CQ...

[WebKit Review Bot]

Comment on attachment 121264 [details]
Patch

Clearing flags on attachment: 121264

Committed r104168: <http://trac.webkit.org/changeset/104168>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.