Bug 42293 - [Qt] Remove FrameLoaderClientQt::webFrame() to use NetworkingContext to get the WebFrame to avoid layering violations
Summary: [Qt] Remove FrameLoaderClientQt::webFrame() to use NetworkingContext to get t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Diego Gonzalez
URL:
Keywords: Qt
Depends on: 42292
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-14 15:10 PDT by Jesus Sanchez-Palencia
Modified: 2010-09-16 05:39 PDT (History)
5 users (show)

See Also:


Attachments
patch (10.57 KB, patch)
2010-07-14 15:29 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (8.39 KB, patch)
2010-09-14 14:27 PDT, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Patch v2 (10.17 KB, patch)
2010-09-15 06:38 PDT, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Patch v3 (10.32 KB, patch)
2010-09-15 07:06 PDT, Diego Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesus Sanchez-Palencia 2010-07-14 15:10:09 PDT
After we have something like a PlatformWebFrame and a FrameLoaderClient::platformWebFrame we should remove all layering violations related to this.
Comment 1 Jesus Sanchez-Palencia 2010-07-14 15:29:09 PDT
Created attachment 61574 [details]
patch
Comment 2 Antonio Gomes 2010-07-15 15:22:04 PDT
Good you are looking into it!

An yet-not-perfect-alternate-approach would be replacing the use of FrameLoaderClientQt in WebCore by ...

QWebFrame* QWebFramePrivate::core(Frame*)

... method. Given a Frame, it returns the associated QWebFrame.

Of course, you'd be eliminating all direct accesses to FrameLoaderClientQt from WebCore *but* including qwebframe_p.h.

Again, it is not perfect, but works better anyways to me.
Comment 3 Antonio Gomes 2010-07-15 15:58:18 PDT
(In reply to comment #2)
> QWebFrame* QWebFramePrivate::core(Frame*)

s/core/kit
Comment 4 Jesus Sanchez-Palencia 2010-08-12 12:06:11 PDT
After NetworkingContext (bug 42292) is reviewed and landed, we should probably review the need of having FrameLoaderClientQt::webFrame().
Comment 5 Diego Gonzalez 2010-09-14 14:27:16 PDT
Created attachment 67606 [details]
Patch
Comment 6 Early Warning System Bot 2010-09-14 14:36:52 PDT
Attachment 67606 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4019008
Comment 7 Kenneth Rohde Christiansen 2010-09-15 00:51:44 PDT
Comment on attachment 67606 [details]
Patch

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

> WebKit/qt/Api/qwebframe.cpp:898
> +            QWebFrame* webFrame = static_cast<QWebFrame*>(loader->networkingContext()->originatingObject());
qobject_cast?

> WebKit/qt/Api/qwebframe.cpp:1457
> +    return static_cast<QWebFrame*>(coreFrame->loader()->networkingContext()->originatingObject());
qobject_cast?

> WebKit/qt/Api/qwebpage.cpp:1921
> +    return static_cast<QWebFrame*>(frame->loader()->networkingContext()->originatingObject());
qobject_cast?

> WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:298
> +    QWebFrame* webFrame = static_cast<QWebFrame*>(f->loader()->networkingContext()->originatingObject());
here

> WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:305
> +    QWebFrame* webFrame = static_cast<QWebFrame*>(f->loader()->networkingContext()->originatingObject());
here as well

> WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:312
> +    QWebFrame* webFrame = static_cast<QWebFrame*>(f->loader()->networkingContext()->originatingObject());
and here :-)
Comment 8 Diego Gonzalez 2010-09-15 06:38:51 PDT
Created attachment 67670 [details]
Patch v2
Comment 9 Early Warning System Bot 2010-09-15 06:45:59 PDT
Attachment 67670 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3997020
Comment 10 Diego Gonzalez 2010-09-15 06:48:50 PDT
Comment on attachment 67670 [details]
Patch v2

Wrong patch. :(
Comment 11 Antonio Gomes 2010-09-15 06:57:50 PDT
since static_cast is faster , in this case it wont't fail, and he is not null-checking anyways, I thought it was ok as is.

no strong preference.

(In reply to comment #7)
> (From update of attachment 67606 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67606&action=prettypatch
> 
> > WebKit/qt/Api/qwebframe.cpp:898
> > +            QWebFrame* webFrame = static_cast<QWebFrame*>(loader->networkingContext()->originatingObject());
> qobject_cast?
> 
> > WebKit/qt/Api/qwebframe.cpp:1457
> > +    return static_cast<QWebFrame*>(coreFrame->loader()->networkingContext()->originatingObject());
> qobject_cast?
> 
> > WebKit/qt/Api/qwebpage.cpp:1921
> > +    return static_cast<QWebFrame*>(frame->loader()->networkingContext()->originatingObject());
> qobject_cast?
> 
> > WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:298
> > +    QWebFrame* webFrame = static_cast<QWebFrame*>(f->loader()->networkingContext()->originatingObject());
> here
> 
> > WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:305
> > +    QWebFrame* webFrame = static_cast<QWebFrame*>(f->loader()->networkingContext()->originatingObject());
> here as well
> 
> > WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:312
> > +    QWebFrame* webFrame = static_cast<QWebFrame*>(f->loader()->networkingContext()->originatingObject());
> and here :-)
Comment 12 Diego Gonzalez 2010-09-15 07:06:24 PDT
Created attachment 67672 [details]
Patch v3
Comment 13 WebKit Commit Bot 2010-09-16 05:39:05 PDT
Comment on attachment 67672 [details]
Patch v3

Clearing flags on attachment: 67672

Committed r67608: <http://trac.webkit.org/changeset/67608>
Comment 14 WebKit Commit Bot 2010-09-16 05:39:11 PDT
All reviewed patches have been landed.  Closing bug.