|
Description
Jesus Sanchez-Palencia
2010-07-14 14:57:16 PDT
Created attachment 61571 [details]
patch
Simple patch that adds what is needed in WebCore. I'm just not sure if Frame.h would be the correct place to have the PlatformWebFrame typedef. I followed the idea behind PlatformPageClient which is defined in Widget.h as a typedef to a PlatformWidget.
Darin, Simon and Kenneth, I would really appreciate your comments on this. Thanks in advance! Comment on attachment 61571 [details]
patch
I don’t understand what this type is for. It seems this will enable layer violations instead of avoiding them.
We need to find another way to do this. Designs where things in the platform directory need a concept of “web frame” are no good. At the very least we need a different name for this. If the goal is to avoid “layer violations” then any types involved would need to be defined the platform directory, not in Frame.h. I think the concept to add here would be a NetworkingContext or NetworkingClient. (In reply to comment #4) Thanks for your comments, Darin. > We need to find another way to do this. Designs where things in the platform directory need a concept of “web frame” are no good. At the very least we need a different name for this. > > If the goal is to avoid “layer violations” then any types involved would need to be defined the platform directory, not in Frame.h. Something like a specifc header? (PlatformWebFrame.h) I agree that a different name would be better. > I think the concept to add here would be a NetworkingContext or NetworkingClient. What do you have in mind about a NetworkingContext (or Client)? Could you please give me example? IIUC, a NetworkingContext would be something similar to our QNetworkAccessManager but if this is the case I think that we are more in the need of a NetworkingClient. A network context would simply be an object that would be produced by the frame loader client that would be passed in to the ResourceHandle and CookieJar classes. You would not pass a Frame*, just a NetworkingContext*. On Qt, the NetworkingContext* could be a QWebFrame*. From the point of view of the platform layer it’s a “networking context”, but that just means “the thing you need to do loading and cookies” correctly. (In reply to comment #4) > We need to find another way to do this. Designs where things in the platform directory need a concept of “web frame” are no good. At the very least we need a different name for this. > > If the goal is to avoid “layer violations” then any types involved would need to be defined the platform directory, not in Frame.h. > > I think the concept to add here would be a NetworkingContext or NetworkingClient. That sounds like a sensible solution. We basically have two use cases for this. 1) We can sub class QNetworkAccessManager (what is used for all our net access) and set it on the QWebPage (::setNetworkAccessManager). Thus we need access to the sub classes instance of QNetworkAccessManager when creating the network connection. Basically this comes down to one virtual function: virtual QNetworkReply * createRequest ( Operation op, const QNetworkRequest & req, QIODevice * outgoingData = 0 ) 2) If I remember correctly our QNetworkReply needs to know which QWebFrame which resulted in the reply, in order for some of our API. I believe that was related to cookie handling and security origin, but I'm not a network expert. Do you have any suggestion how such a NetworkingClient should look like? (In reply to comment #6) > A network context would simply be an object that would be produced by the frame loader client that would be passed in to the ResourceHandle and CookieJar classes. You would not pass a Frame*, just a NetworkingContext*. On Qt, the NetworkingContext* could be a QWebFrame*. From the point of view of the platform layer it’s a “networking context”, but that just means “the thing you need to do loading and cookies” correctly. Wouldn't it be a layering violation to typedef NetworkingContext* to QWebFrame*, as QWebFrame is defined in WebKit/? our other Context's like GraphicsContexts wraps Qt classes, all defined in Qt and not in WebKit/ (In reply to comment #8) > Wouldn't it be a layering violation to typedef NetworkingContext* to QWebFrame*, as QWebFrame is defined in WebKit/? our other Context's like GraphicsContexts wraps Qt classes, all defined in Qt and not in WebKit/ The object can be an class that implements whatever the Qt networking code needs. It can be an abstract class, and the concrete instance can come from WebKit. The concrete instance can hold a QWebFrame* in it. (In reply to comment #9) > (In reply to comment #8) > > Wouldn't it be a layering violation to typedef NetworkingContext* to QWebFrame*, as QWebFrame is defined in WebKit/? our other Context's like GraphicsContexts wraps Qt classes, all defined in Qt and not in WebKit/ > > The object can be an class that implements whatever the Qt networking code needs. It can be an abstract class, and the concrete instance can come from WebKit. The concrete instance can hold a QWebFrame* in it. Yes, that sounds like our current "Clients" that we reimplement in WebKit/qt/WebCoreSupport. Wouldn't NetworkingClient be a better name then? Sure, NetworkingClient is a possible name, but the Client classes are mostly at the higher WebCore level, not in the WebCore/platform so we may want to use a different name. I still think NetworkingContext is a reasonable name. Maybe someone can think of an even better one. It contains the additional data needed to correctly do networking in the context of a particular web page. One interesting thing about this class is that there is no purpose to it cross-platform. It only has platform-specific things in it. The only platform-independent aspect to it is that it's received from the FrameLoaderClient and passed in to networking related code. But for most platforms that’s just a null pointer. Attachment 61571 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3453308 Changing bug's name to follow the most recent discussion going on here. Created attachment 61977 [details]
NetworkingContext v1
Addressing the previous comments/ideas.
What do you guys think about this NetworkingContext approach?
Thanks in advance!
Attachment 61977 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:57: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 61977 [details]
NetworkingContext v1
I don’t think we necessarily need these two layers, NetworkingContext and PlatformNextworkingContext. I’d want a pattern more like ResourceRequest, where the Qt version of NetworkingContext has a pointer to a QWebFrame, but there’s no ifdef in the main NetworkingContext header. Or maybe there’s another example we can match. Even if we do want it all in one header, I think the PlatformNetworkingContext thing is no good. Just have a QWebFrame* there for Qt only. Some platforms might have more data inside a NetworkingContext than a single pointer, and some might not need the pointer, so lets not force it on everyone.
The networking context on FrameLoaderClientQt should be an OwnPtr, not a raw pointer.
The initial patch should include replacing the existing Frame* argument in ResourceHandle with a NetworkingContext*.
What's the lifetime of the NetworkingContext object? I don't think this is expressed in a workable fashion in the current patch. It can be a reference counted object, or one that has some other explicit way of maintaining its lifetime. By adding the object but not uses of it, it allows us to have the lifetime issue not yet solved, which I think is a problem.
(In reply to comment #16) Thanks for your comments, Darin. > The initial patch should include replacing the existing Frame* argument in ResourceHandle with a NetworkingContext*. Just to be sure, do you mean the argument from ResourceHandle::create() and from ResourceHandle::loadResourceSynchronously()? Right now they have a #FIXME above them. > What's the lifetime of the NetworkingContext object? I don't think this is expressed in a workable fashion in the current patch. It can be a reference counted object, or one that has some other explicit way of maintaining its lifetime. By adding the object but not uses of it, it allows us to have the lifetime issue not yet solved, which I think is a problem. Yes, I agree. I'm sticking to the idea where FrameLoaderClient is the only one creating a NetworkingContext instance and everybody else access it when needed (i.e.g.: frame->loader()->client()->networkingContext()). What do you think? In this case should I still make it inherits RefCounted<NetworkingContext>? (In reply to comment #17) > Just to be sure, do you mean the argument from ResourceHandle::create() and from ResourceHandle::loadResourceSynchronously()? Right now they have a #FIXME above them. Yes. > > What's the lifetime of the NetworkingContext object? I don't think this is expressed in a workable fashion in the current patch. It can be a reference counted object, or one that has some other explicit way of maintaining its lifetime. By adding the object but not uses of it, it allows us to have the lifetime issue not yet solved, which I think is a problem. > > Yes, I agree. I'm sticking to the idea where FrameLoaderClient is the only one creating a NetworkingContext instance and everybody else access it when needed (i.e.g.: frame->loader()->client()->networkingContext()). What do you think? In this case should I still make it inherits RefCounted<NetworkingContext>? I’m not saying you need to reference count it. I am saying that you need to actually use it in ResourceHandle and see if it does need reference counting. It may turn out that people just get the data they need and do not need a way to keep a pointer past then. Created attachment 62106 [details]
NetworkingContext v2
Addressing the previous comments and ideas.
Comment on attachment 62106 [details] NetworkingContext v2 > - m_handle = ResourceHandle::create(r, this, m_frame.get(), false, true); > + m_handle = ResourceHandle::create(r, this, m_frame.get()->loader()->client()->networkingContext(), false, true); Should not need get() here any more. > - m_handle = ResourceHandle::create(clientRequest, this, m_frame.get(), m_defersLoading, m_shouldContentSniff); > + m_handle = ResourceHandle::create(clientRequest, this, m_frame.get()->loader()->client()->networkingContext(), m_defersLoading, m_shouldContentSniff); Or here. > diff --git a/WebCore/loader/ResourceLoader.h b/WebCore/loader/ResourceLoader.h > index cbdd8c2..1b097c6 100644 > --- a/WebCore/loader/ResourceLoader.h > +++ b/WebCore/loader/ResourceLoader.h > @@ -29,6 +29,7 @@ > #ifndef ResourceLoader_h > #define ResourceLoader_h > > +#include "FrameLoaderClient.h" Why does this need to be added to this header? Doesn't make sense to me. Perhaps should be added to some .cpp files, but not the header. > +class NetworkingContext : public Noncopyable { > +public: > + static PassOwnPtr<NetworkingContext> create(Frame *frame); The networking context class cannot include a pointer to the Frame*. That’s the same layering violation we are trying to fix. Instead, it needs to take whatever arguments make sense on each platform that needs it. But never a Frame*. This means you have to plumb the NetworkingContext in deeper, because you can't just go from NetworkingContext* to Frame*. But maybe you could temporarily have it work this way as a staging step. > + , m_networkingContext(0) No need for this. An OwnPtr automatically starts at 0. > +#include "OwnPtr.h" Should be <wtf/OwnPtr.h>. review- because of the "includes a pointer to Frame*" issue. Attachment 62106 [details] did not build on mac: Build output: http://queues.webkit.org/results/3395659 (In reply to comment #20) > > +class NetworkingContext : public Noncopyable { > > +public: > > + static PassOwnPtr<NetworkingContext> create(Frame *frame); > > The networking context class cannot include a pointer to the Frame*. That’s the same layering violation we are trying to fix. Instead, it needs to take whatever arguments make sense on each platform that needs it. But never a Frame*. This means you have to plumb the NetworkingContext in deeper, because you can't just go from NetworkingContext* to Frame*. But maybe you could temporarily have it work this way as a staging step. I included this Frame* because after I removed the Frame* argument from ResourceHandle::create() it was still needed in ResourceHandle::start(Frame*). create() calls start(). I've checked Qt's ResourceHandle::start() implementation and it would be ok to modify it to ResourceHandle::start(NetworkingContext*) because we don't really need a Frame* but I'm not sure about other ports at this moment. I might be missing something if there is another way to pass a Frame* to ResourceHandle::start(). What do you think? Thanks for the review, by the way! :) (In reply to comment #22) > I included this Frame* because after I removed the Frame* argument from ResourceHandle::create() it was still needed in ResourceHandle::start(Frame*). create() calls start(). > I've checked Qt's ResourceHandle::start() implementation and it would be ok to modify it to ResourceHandle::start(NetworkingContext*) because we don't really need a Frame* but I'm not sure about other ports at this moment. I might be missing something if there is another way to pass a Frame* to ResourceHandle::start(). What do you think? ResourceHandle::start() needs to take a NetworkingContext*. Then we need to keep digging down until we find the place in each port where it needs some port-specific data of some sort. That will tell us what needs to be in the NetworkingContext for each port. (In reply to comment #23) > ResourceHandle::start() needs to take a NetworkingContext*. Then we need to keep digging down until we find the place in each port where it needs some port-specific data of some sort. That will tell us what needs to be in the NetworkingContext for each port. Ok, I see. I had a quick look at all implementations of ResourceHandle::start and what I noticed was that all ports do something similar to: if (!frame) return false; Page *page = frame->page(); // If we are no longer attached to a Page, this must be an attempted load from an // onUnload handler, so let's just block it. if (!page) return false; Besides that they have: =Qt= Has getInternal()->m_frame = static_cast<FrameLoaderClientQt*>(frame->loader()->client())->webFrame(); but will be changed to getInternal()->m_frame = networkingContext->webFrame(); so it's not a problem. =Mac= Has d->m_needsSiteSpecificQuirks = frame->settings() && frame->settings()->needsSiteSpecificQuirks(); and createNSURLConnection( d->m_proxy.get(), shouldUseCredentialStorage, d->m_shouldContentSniff || frame->settings()->localFileContentSniffingEnabled()); So it basically needs the frame->settings(). =Android= DocumentLoader* docLoader = frame->loader()->activeDocumentLoader(); MainResourceLoader* mainLoader = docLoader->mainResourceLoader(); bool isMainResource = mainLoader && (mainLoader->handle() == this); PassRefPtr<ResourceLoaderAndroid> loader = ResourceLoaderAndroid::start(this, d->m_request, frame->loader()->client(), isMainResource, false); if (loader) { d->m_loader = loader; return true; } So it basically needs the frame->loader() in order to get the FrameLoaderClient and the DocumentLoader. Need to check what ResourceLoaderAndroid::start does with a FrameLoaderClient. =CF= Nothing more than the general !frame->page() check. =Soup= // Used to set the authentication dialog toplevel; may be NULL d->m_frame = frame; Keeps a Frame*. Need to check what for. =Curl= Nothing more than the general !frame->page() check. =Win= Does not check if !frame or !frame->page() but uses the Frame* for: String userAgentStr = frame->loader()->userAgent(request().url()) + String("", 1); and String referrer = frame->loader()->referrer(); I'll dig down a little bit more. (In reply to comment #24) > (In reply to comment #23) > > ResourceHandle::start() needs to take a NetworkingContext*. Then we need to keep digging down until we find the place in each port where it needs some port-specific data of some sort. That will tell us what needs to be in the NetworkingContext for each port. So, in summary, the port-specific bits are: - For Mac, NetworkingContext needs a Settings*. Is this a layer violation as having a Frame*? - For Android, a bool isMainResource and a FrameLoaderClient* are needed in order to start their ResourceLoader. - For Win, two Strings are needed: user agent and referrer. - When using Soup, the ResourceHandle keeps a Frame* and I don't know how to avoid this yet. Each FrameLoaderClient will create a NetworkingContext with port-specific data. I'll prepare a more complete patch but I'm afraid I can only test with Qt and Mac. I might your guidance in order to have this working properly with all ports, so please let me know what you guys think about the above ideas. Thanks in advance (In reply to comment #25) > - For Mac, NetworkingContext needs a Settings*. Is this a layer violation as having a Frame*? Having a Settings* in a class defined in WebKit is not a layer violation. But using Settings* in any code inside WebCore/platform is. The way to do this is to have NetworkingContext have virtual functions returning the specific settings, and then the Mac version can choose whether to put in a Settings* or just store the values of those settings when making the NetworkingContext object. > - For Android, a bool isMainResource and a FrameLoaderClient* are needed in order to start their ResourceLoader. The isMainResource item is a tricky issue. It doesn't seem to be a true part of NetworkingContext. This should be passed explicitly as an argument separately from the NetworkingContext*. In fact, I’m not sure how that flag could be part of a Frame* at all! The NetworkingContext* for a particular WebKit could definitely include the FrameLoaderClient* if it was calling into WebKit code. But if the networking code is inside platform then using FrameLoaderClient* is a layering violation, and the real question is what it does with the FrameLoaderClient*. It’s the same problem all over again, another level deep. > - For Win, two Strings are needed: user agent and referrer. Perfect! Those can either be stored in the NetworkingClient at creation time or we can have virtual functions in NetworkingClient that call through to things in WebKit to get the answer. > - When using Soup, the ResourceHandle keeps a Frame* and I don't know how to avoid this yet. Not enough information here. Need to figure out what the real need is. (In reply to comment #26) OK, so for Qt, Mac and Win things are clear. The last issue is the generally used: // If we are no longer attached to a Page, this must be an attempted load from an // onUnload handler, so let's just block it. Page* page = frame->page(); if (!page) return false; > The isMainResource item is a tricky issue. It doesn't seem to be a true part of NetworkingContext. This should be passed explicitly as an argument separately from the NetworkingContext*. In fact, I’m not sure how that flag could be part of a Frame* at all! Actually it's not part of a Frame*. They use the Frame* to: DocumentLoader* docLoader = frame->loader()->activeDocumentLoader(); MainResourceLoader* mainLoader = docLoader->mainResourceLoader(); bool isMainResource = mainLoader && (mainLoader->handle() == this); And just then this isMainResource and FrameLoaderClient* are used to start their ResourceLoader. PassRefPtr<ResourceLoaderAndroid> loader = ResourceLoaderAndroid::start(this, d->m_request, frame->loader()->client(), isMainResource, false); I went after the implementation of ResourceLoaderAndroid::start() but I couldn't find it. Actually, I couldn't find any implementation of ResourceLoaderAndroid besides its header file. I'm not sure if it's ok to just modify their header. I'm trying to contact someone from the Android port. > > - When using Soup, the ResourceHandle keeps a Frame* and I don't know how to avoid this yet. > > Not enough information here. Need to figure out what the real need is. GTK port uses it in static GtkWidget* currentToplevelCallback, from WebKit/gtk/webkit/webkitprivate.cpp, to: GtkWidget* toplevel = gtk_widget_get_toplevel(GTK_WIDGET(frame->page()->chrome()->platformWindow())); They use it to set the authentication dialog toplevel. I'll have to contact the GTK guys to see how we can remove the Frame* usage. It does not feel right to have a GTK toplevel widget in NetworkingContext, so maybe they'll need another way in WebKit to get this. While I believe that solving this layering violation is a valuable work, I am wondering about how will the NetworkContext will solve the problems in the qt webkit2 port. Currently we have no QWebFrame in webkit2 so a NetworkContext* can not be a QWebFrame*. Are we going to implement a new API that has smg like a QWebFrame, or just pass 0 as NetworkContext* and creating a default QNetworkAccessManager, a default QNetworkCookieJar, etc? Maybe there is smg better possibility what I did not realized? (In reply to comment #27) > (In reply to comment #26) > OK, so for Qt, Mac and Win things are clear. The last issue is the generally used: > // If we are no longer attached to a Page, this must be an attempted load from an > // onUnload handler, so let's just block it. > Page* page = frame->page(); > if (!page) > return false; Another Mac specific issue that I've just found out. In ResourceHandle::loadResourceSynchronously the Mac port uses Frame* to get the FrameLoader and call FrameLoader::blockedError . I have found ResuorceLoaderAndroid::start with the help of www.google.com/codesearch
PassRefPtr<ResourceLoaderAndroid> ResourceLoaderAndroid::start(
ResourceHandle* handle, const ResourceRequest& request, FrameLoaderClient* client, bool isMainResource, bool isSync)
{
FrameLoaderClientAndroid* clientAndroid = static_cast<FrameLoaderClientAndroid*> (client);
return clientAndroid->webFrame()->startLoadingResource(handle, request, isMainResource, isSync);
}
so it does not need more then a pointer to a WebFrame what seems to be an API class of chromioum
Created attachment 62760 [details]
NetworkingContext v3 (not ready)
Although this patch is not ready I'm uploading it with r? so I can get it built on the ews bots. Unfortunately this is the only way I have to test Win and GTK build.
However, comments are very welcome. The remaining open bits are:
- on GTK: They need a frame* from Soup as I explained.
- on Mac: I've added a // FIXME and commented a the code snippet below in ResourceHandle::loadResourceSynchronously:
// if (handle->d->m_scheduledFailureType != NoFailure) {
// error = frame->loader()->blockedError(request);
// return;
// }
Created attachment 62772 [details]
NetworkingContext v3 (not ready)
Same as before, but this one was rebased on trunk.
I'm aware that the Changelogs are outdated, but this will be fixed on the final patch. I'd appreciate any comments on the mentioned open bits in my previous comment.
Xan, can you help Jesus with the GTK bits? or indicate someone to help him? Darin, who would be the right mac guy?
> - on Mac: I've added a // FIXME and commented a the code snippet below in ResourceHandle::loadResourceSynchronously:
>
> // if (handle->d->m_scheduledFailureType != NoFailure) {
> // error = frame->loader()->blockedError(request);
> // return;
> // }
So if I understand you correctly, the problem is that mac is accessing the FrameLoader here. Is that blockedError uses elsewhere or just here?
Attachment 62772 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3589584 Attachment 62772 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3570557 (In reply to comment #34) > > - on Mac: I've added a // FIXME and commented a the code snippet below in ResourceHandle::loadResourceSynchronously: > > > > // if (handle->d->m_scheduledFailureType != NoFailure) { > > // error = frame->loader()->blockedError(request); > > // return; > > // } > > So if I understand you correctly, the problem is that mac is accessing the FrameLoader here. Is that blockedError uses elsewhere or just here? Actually the loader gets the blockedError from the FrameLoaderClient so we do not need to touch the FrameLoader just the client. However this is in contrast to Darin's concept which is to avoid touching the FrameLoaderClient in platform code. WebCore/platform/network/qt/NetworkingContextQt.cpp:27
+ #if PLATFORM(QT)
I think this is somewhat redundant to guard with platform check here since the build system will pick this file only if we are building Qt. Actually most of the C++ IDE-s I know cannot resolve the parametric macros we use in WebKit and do not index the files that have this kind of guards. In my opinion such guards should be used only if we have a good reason to do this.
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1517
+NetworkingContext* FrameLoaderClientQt::networkingContext()
+{
+ return m_networkingContext.get();
+}
Wouldn't it be better if we creating the NetworkingContext on demand as you did in the mac and win implementation? This way we could assure that it's state is valid. In regard of the on demand logic a better name for the method would be createNetworkingContext.
However, I am not a reviewer :)
Hi, Sorry for the late reply and thanks for CC'ing me on this bug. (In reply to comment #27) > (In reply to comment #26) > > > > The isMainResource item is a tricky issue. It doesn't seem to be a true part of NetworkingContext. This should be passed explicitly as an argument separately from the NetworkingContext*. In fact, I’m not sure how that flag could be part of a Frame* at all! > > Actually it's not part of a Frame*. They use the Frame* to: > DocumentLoader* docLoader = frame->loader()->activeDocumentLoader(); > MainResourceLoader* mainLoader = docLoader->mainResourceLoader(); > bool isMainResource = mainLoader && (mainLoader->handle() == this); > > And just then this isMainResource and FrameLoaderClient* are used to start their ResourceLoader. > PassRefPtr<ResourceLoaderAndroid> loader = ResourceLoaderAndroid::start(this, d->m_request, frame->loader()->client(), isMainResource, false); > > I went after the implementation of ResourceLoaderAndroid::start() but I couldn't find it. Actually, I couldn't find any implementation of ResourceLoaderAndroid besides its header file. I'm not sure if it's ok to just modify their header. I'm trying to contact someone from the Android port. > As far as Andoid is concerned, I think having the NetworkContext class would work fine. The only problem is the isMainResource parameter. This flag is passed all the way to our Java HTTP machinery and is used when the response headers arrive: http://android.git.kernel.org/?p=platform/frameworks/base.git;a=blob;f=core/java/android/webkit/LoadListener.java;h=e6fa405b11ceb5dbcc30eddfe2b06b9bd5a86f2e;hb=refs/heads/froyo#l394 Depending on the response mime type and whether this is a main resource load, we may query the platform to see if there is some other application that can handle the request. To me, it seems that this information is about the resource request and it's similar to the ResourceRequest::m_userGesture flag. So how about adding a new field like m_mainResource to ResourceRequest? As for modifying the ResourceLoaderAndroid header, it would be useful to us if you could do that in this patch and we can take care of fixing the rest ourselves. Thanks, Andrei (In reply to comment #38) > WebCore/platform/network/qt/NetworkingContextQt.cpp:27 > + #if PLATFORM(QT) > I think this is somewhat redundant to guard with platform check here since the build system will pick this file only if we are building Qt. Actually most of the C++ IDE-s I know cannot resolve the parametric macros we use in WebKit and do not index the files that have this kind of guards. In my opinion such guards should be used only if we have a good reason to do this. Totally agree, I just forgot it there for some reason. Will be removed. > WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1517 > +NetworkingContext* FrameLoaderClientQt::networkingContext() > +{ > + return m_networkingContext.get(); > +} > Wouldn't it be better if we creating the NetworkingContext on demand as you did in the mac and win implementation? This way we could assure that it's state is valid. In regard of the on demand logic a better name for the method would be createNetworkingContext. Agreed again. I had this in mind, but I haven't had the time to update the Qt code since I was working on the other ports. Anyway, it will be like win and mac in the final patch. > > However, I am not a reviewer :) Yet. :) Comments are always welcome, thanks! (In reply to comment #39) Thanks for your reply! > To me, it seems that this information is about the resource request and it's similar to the ResourceRequest::m_userGesture flag. So how about adding a new field like m_mainResource to ResourceRequest? Agreed. > > As for modifying the ResourceLoaderAndroid header, it would be useful to us if you could do that in this patch and we can take care of fixing the rest ourselves. I'll work on that. Thanks again! (In reply to comment #37) > (In reply to comment #34) > > > - on Mac: I've added a // FIXME and commented a the code snippet below in ResourceHandle::loadResourceSynchronously: > > > > > > // if (handle->d->m_scheduledFailureType != NoFailure) { > > > // error = frame->loader()->blockedError(request); > > > // return; > > > // } > > > > So if I understand you correctly, the problem is that mac is accessing the FrameLoader here. Is that blockedError uses elsewhere or just here? > > Actually the loader gets the blockedError from the FrameLoaderClient so we do not need to touch the FrameLoader just the client. However this is in contrast to Darin's concept which is to avoid touching the FrameLoaderClient in platform code. The most obvious solution here is to have the NetworkingClient offer a way to create the blocked error. That can be a Mac-specific function of the NetworkingClient or a cross-platform one if there is some similar need on other platforms. (In reply to comment #42) > The most obvious solution here is to have the NetworkingClient offer a way to create the blocked error. That can be a Mac-specific function of the NetworkingClient or a cross-platform one if there is some similar need on other platforms. I mean NetworkingContext. Created attachment 62882 [details]
NetworkingContext v3 (not ready)
Same as previous but now covering GTK and Chromium as well. Let's see if it builds on EWS.
I've updated the Changelogs and the Qt code as well. Now we only have left the blockedError() call and the Frame* need for GTK issues to solve.
Attachment 62882 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/CMakeListsEfl.txt:94: Line contains tab character. [whitespace/tab] [5]
Total errors found: 1 in 39 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 62882 [details] did not build on qt: Build output: http://queues.webkit.org/results/3563635 Created attachment 62886 [details]
NetworkingContext v3 (not ready)
Attachment 62886 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/CMakeListsEfl.txt:94: Line contains tab character. [whitespace/tab] [5]
Total errors found: 1 in 44 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 62882 [details] did not build on mac: Build output: http://queues.webkit.org/results/3565658 Attachment 62886 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3573578 Created attachment 62941 [details]
NetworkingContext v3 (not ready)
Comment on attachment 62941 [details]
NetworkingContext v3 (not ready)
Removing the r? since all ews' bots have runned.
Does anyone know why the patch never applies on Windows? It's something related to the WebCore.vcproj file.
The isValid() aspect of this patch is wrong. The issue isn't whether frame->page() was 0 at the time the NetworkingContext object was created. Instead the issue is whether frame->page() is 0 now!
I think the NetworkingContext class itself should have only pure virtual functions, no data members, and a protected constructor.
Then there could be a class in WebCore that adds the isValid implementation, perhaps even by putting a Frame* into the object.
class FrameNetworkingContext : public NetworkingContext
Then the frame loader client function would create a new networking context for each frame, and we would store it in the frame loader. The frame loader client would create an object of a class that's derived from FrameNetworkingContext. FrameNetworkingContext itself could either be a concrete class or an abstract base class depending on whether FrameLoader has everything needed or if WebKit needs to add some things not known to WebCore.
Jesus, I think you didn't analyze the problem fully. What Qt needs is to know who initiated a request. As said by the docs: ---- void QNetworkRequest::setOriginatingObject ( QObject * object ) Allows setting a reference to the object initiating the request. For example QtWebKit sets the originating object to the QWebFrame that initiated the request. ---- ResourceRequest is used by Qt to create our QNetworkRequest (using our ResourceRequest::toNetworkRequest). This is where the QWebFrame is used when calling setOriginatingObject. I am not familiar with how these ResourceRequests are created, but it seems that we will need to call the setOriginatingObject from the WebKit side. The current suggested architecture doesn't seem to easily allow this. Darin, do you have any suggestions with this new info? (In reply to comment #54) > Darin, do you have any suggestions with this new info? Sure, to deal with this I would suggest: 1) The Qt version of the NetworkingContext abstract base class will have a virtual function that has the responsibilities of filling in additional information in the request, which would include at least the originating object. This can either be a platform-independent function with a platform-specific implementation, in which case it could possibly take a ResourceRequest argument, or a Qt-specific function, in which case it could take a QNetworkRequest argument. 2) Each frame will have its own NetworkingContext object. Already taken care of in my recent proposal. I suggested storing it in the FrameLoader. 3) The concrete implementation of NetworkingContext in the Qt WebKit would have a way to get the QWebFrame* and call setOriginatingObject, passing it, in the implementation of the function from (1) above. It can hold the QWebFrame* pointer directly, or get it indirectly in whatever way makes the most sense; we just have to get the object lifetimes right. Since the platform-independent FrameNetworkingContext will probably have a Frame* in it, one way of doing this is to call the functions that take us from a Frame* to a QWebFrame*. Hope that helps. Created attachment 63752 [details]
NetworkingContext v4 (not ready)
This patch covers the latest ideas raised here and is ready for Qt and Mac (just missing the blockedError call, but now this is solvable with the Frame*).
Darin, could you please take a look at in order to check if I misunderstood something?
Thanks in advance!
Comment on attachment 63752 [details] NetworkingContext v4 (not ready) Yes, this is looking good. The right general direction. The weakest spot is the object lifetime. What guarantees that the NetworkingContext object is still around as long as the networking code holds a pointer to it? If it’s owned by the frame and gets deleted when the frame does, then somehow everyone holding a pointer to it in a local variable or in a data member (all the ResourceHandle objects) will need it zeroed out. Alternatively, if it's going to be shared it needs to be reference counted, and then the frame loader needs to null out and invalidate it when it is deleted, or perhaps earlier when the page is destroyed. > +FrameNetworkingContext* FrameLoader::frameNetworkingContext() Just name the function networkingContext. > + return m_frameNetworkingContext.get(); Just name the data member m_networkingContext. > + m_frameNetworkingContext.clear(); > + m_frameNetworkingContext = frameNetworkingContext; No need to clear an OwnPtr before assigning a PassOwnPtr to it. You can just leave out that clear line. > + FrameNetworkingContext* frameNetworkingContext(); > + void setFrameNetworkingContext(PassOwnPtr<FrameNetworkingContext>); I'm not sure it makes sense to have a public setter function. The frame loader should create the networking context itself, not have it set by another object. In fact, the getter function should probably call createNetworkingContext. > +#include "Frame.h" It should not be necessary to include "Frame.h". A forward declaration should do. > + virtual ~FrameNetworkingContext() { } This declaration should not be needed. > +#if PLATFORM(QT) > + static PassOwnPtr<FrameNetworkingContext> create(Frame* frame, QObject* originatingObject, QNetworkAccessManager* networkManager); > +#else > + static PassOwnPtr<FrameNetworkingContext> create(Frame* frame); > +#endif FrameNetworkingContext should not have a create function. It should be an abstract base class and only concrete subclasses of it should be created. Those concrete subclasses can be in WebKit and that is where the creation functions will live. > +#if PLATFORM(QT) > + virtual QObject* originatingObject() const; > + virtual QNetworkAccessManager* networkAccessManager() const; > +#elif PLATFORM(MAC) > + virtual bool needsSiteSpecificQuirks() const; > + virtual bool localFileContentSniffingEnabled() const; > + virtual SchedulePairHashSet* scheduledRunLoopPairs() const; > +#elif PLATFORM(WIN) > + virtual String userAgent() const; > + virtual String referrer() const; > +#endif These functions should not be implemented in FrameNetworkingContext. Instead they can all be defined in the specific classes derived from this. > + virtual Frame* frame() const { return m_frame; } No reason to have this be a virtual function. And I also think there's no reason to have this function at all. It could be protected or not exist at all. > +#if PLATFORM(QT) > + QObject* m_originatingObject; > + QNetworkAccessManager* m_networkAccessManager; > +#endif These data members should be in the Qt-specific concrete NetworkingContext class, defined in WebKit, rather than in FrameNetworkingContext. > - m_handle = ResourceHandle::create(r, this, m_frame.get(), false, true); > + m_handle = ResourceHandle::create(r, this, m_frame->loader()->client()->networkingContext(), false, true); This is wrong. It should be m_frame->loader()->networkingContext(), and must not call through to the client. > +#include "Page.h" > +#include "Settings.h" > + > + > +namespace WebCore { We don't do those double blank lines. I see them in many places. > +FrameNetworkingContext::FrameNetworkingContext(Frame* frame) > + : m_frame(frame) > +{ > + > +} I think this should go in the FrameNetworkingContext header as an inline. > +#include <wtf/Noncopyable.h> > +#if PLATFORM(MAC) > +#include "SchedulePair.h" > +#endif The Mac-specific include should go in a separate paragraph (add a blank line). > +class NetworkingContext : public Noncopyable { > +public: > +#if PLATFORM(QT) > + virtual QObject* originatingObject() const = 0; > + virtual QNetworkAccessManager* networkAccessManager() const = 0; > +#elif PLATFORM(MAC) > + virtual bool needsSiteSpecificQuirks() const = 0; > + virtual bool localFileContentSniffingEnabled() const = 0; > + virtual SchedulePairHashSet* scheduledRunLoopPairs() const = 0; > +#elif PLATFORM(WIN) > + virtual String userAgent() const = 0; > + virtual String referrer() const = 0; > +#endif > + virtual bool isValid() const = 0; The platform-independent "isValid" function should go first, and then the platform specific functions could each go in their own paragraph. Maybe eventually we will find a way to cut down on those platform-specific functions too. > + static PassRefPtr<ResourceHandle> create(const ResourceRequest&, ResourceHandleClient*, NetworkingContext*, bool defersLoading, bool shouldContentSniff); > + static void loadResourceSynchronously(const ResourceRequest&, StoredCredentials, ResourceError&, ResourceResponse&, Vector<char>& data, NetworkingContext* context); The argument name "context" should be omitted here. Also, I suggest the context be the first argument, before even the request. The reason the Frame was a later argument was that it was added later. > - getInternal()->m_frame = static_cast<FrameLoaderClientQt*>(frame->loader()->client())->webFrame(); > + getInternal()->m_networkingContext = context; I think the name of this data member can be m_context. Since this is a ResourceHandle, "networking" is already assumed. > + virtual void createNetworkingContext(); > + virtual WebCore::NetworkingContext* networkingContext(); I think these should not be two separate functions. There should only be a single function, createNetworkingContext, which should return a PassOwnPtr or PassRefPtr. This would be called in only one place, from the FrameLoader::networkingContext function. Nice work! Lets keep going. Almost there! *** Bug 43710 has been marked as a duplicate of this bug. *** > (From update of attachment 63752 [details])
> FrameNetworkingContext should not have a create function. It should be an abstract base class and only concrete subclasses of it should be created. Those concrete subclasses can be in WebKit and that is where the creation functions will live.
Would the platform specific FrameNetworkingContext subclasses be different for WebKit2? Doesn't putting them to WebKit result in duplication and unnecessary platform specific code in WebKit2?
(In reply to comment #59) > > (From update of attachment 63752 [details] [details]) > > FrameNetworkingContext should not have a create function. It should be an abstract base class and only concrete subclasses of it should be created. Those concrete subclasses can be in WebKit and that is where the creation functions will live. > > Would the platform specific FrameNetworkingContext subclasses be different for WebKit2? Doesn't putting them to WebKit result in duplication and unnecessary platform specific code in WebKit2? Good point... The patch I'm going to upload in a few minutes addresses everything that Darin has reviewed but moving specific implementations to WebKit. As usual it works with Qt and Mac and as soon as I get a "good to go" from Darin I'll prepare the full patch, for all platforms. Created attachment 64037 [details]
NetworkingContext v5 (Qt and Mac)
Attachment 64037 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:57: Alphabetical sorting problem. [build/include_order] [4]
WebCore/loader/mac/FrameNetworkingContextMac.cpp:22: You should add a blank line after implementation file's own header. [build/include_order] [4]
WebCore/loader/qt/FrameNetworkingContextQt.cpp:22: You should add a blank line after implementation file's own header. [build/include_order] [4]
Total errors found: 3 in 27 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 64037 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3743043 Created attachment 64051 [details]
NetworkingContext v5 (all)
A first attempt to see how it goes for all ports.
Attachment 64051 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:50: Alphabetical sorting problem. [build/include_order] [4]
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:57: Alphabetical sorting problem. [build/include_order] [4]
WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:47: Alphabetical sorting problem. [build/include_order] [4]
WebCore/loader/mac/FrameNetworkingContextMac.cpp:22: You should add a blank line after implementation file's own header. [build/include_order] [4]
WebCore/loader/qt/FrameNetworkingContextQt.cpp:22: You should add a blank line after implementation file's own header. [build/include_order] [4]
WebKit/win/WebFrame.cpp:83: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 6 in 46 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 64037 [details] did not build on win: Build output: http://queues.webkit.org/results/3737039 Attachment 64037 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3766030 Attachment 64051 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3761037 If there is no need to have any WebKit-specific code, then we could have just the platform-specific bits inside WebCore. I was under the impression that some platforms wanted to supply information from the WebKit level to be passed back to the networking layer. For example, I thought that QWebFrame was something known only at the WebKit level and not WebCore. I guess I was wrong about that. If we don't need to involve WebKit, then there's no need to involve the FrameLoaderClient either. FrameLoader can just create the object. The whole point of involving the client is to allow the WebKit layer to supply a derived class with details that are known only to WebCore and not WebKit. Since QWebFrame is part of WebKit and not WebCore, I think the networking code needs to be written in terms of virtual functions in NetworkingContext and not direct use of QWebFrame. And I think we do need to allow WebKit to allocate a subclass. The issue determining whether to put the class in WebKit or WebCore is layering, not platform independence. Comment on attachment 64051 [details] NetworkingContext v5 (all) > +NetworkingContext* FrameLoader::networkingContext() > +{ > + return m_networkingContext.get(); > +} This can be inlined in the header, instead of being put in the .cpp file. > #include "FrameLoaderTypes.h" > +#include "FrameNetworkingContext.h" I don't understand why this header, which uses only the NetworkingContext class, includes FrameNetworkingContext.h instead of NetworkingContext.h. > + virtual PassRefPtr<NetworkingContext> createNetworkingContext() { return PassRefPtr<NetworkingContext>(); } I don't think this default implementation is useful. This will just immediately crash if you try to do any I/O. This should be "= 0" instead. > +#if PLATFORM(QT) > + static PassRefPtr<FrameNetworkingContext> create(Frame* frame, QObject* originatingObject, QNetworkAccessManager* networkManager); > +#elif PLATFORM(WIN) > + static PassRefPtr<FrameNetworkingContext> create(Frame* frame, String userAgent); > +#else > + static PassRefPtr<FrameNetworkingContext> create(Frame* frame) { return adoptRef(new FrameNetworkingContext(frame)); } > +#endif If we're actually going to create instances of FrameNetworkingContext, then I'm not entirely clear why the FrameLoaderClient is asked to do this. I guess in the Qt case it involves things you can only get inside WebKit? If the function declarations here do not include bodies, then there is not need to have the argument names "frame" and "networkManager" and it is our style to leave them out. The userAgent argument should be "const String&". > +#if PLATFORM(QT) > + virtual QObject* originatingObject() const; > + virtual QNetworkAccessManager* networkAccessManager() const; > + > +#elif PLATFORM(MAC) > + virtual bool needsSiteSpecificQuirks() const; > + virtual bool localFileContentSniffingEnabled() const; > + virtual SchedulePairHashSet* scheduledRunLoopPairs() const; > + virtual ResourceError blockedError(const ResourceRequest&) const; > + > +#elif PLATFORM(WIN) > + virtual String userAgent() const; > + virtual String referrer() const; > +#endif > + > + // If we are no longer attached to a Page, this must be an attempted load from an > + // onUnload handler, so the FrameNetworkingContext must be invalid. > + virtual bool isValid() const { return m_frame && m_frame->page(); } I suggest making all these virtual function implementations private. Callers should only call them if they have a NetworkingContext*. If they have a FrameNetworkingContext* it's probably a programming mistake for them to use these abstractions so it's good to catch that error. > + virtual void invalidate() { m_frame = 0; } This function should be non-virtual and public and present only in FrameNetworkingContext and not in NetworkingContext. To make it possible to call this, FrameLoader should store a RefPtr<FrameNetworkingContext> instead of a RefPtr<NetworkingContext>. > +FrameNetworkingContext::FrameNetworkingContext(Frame* frame, QObject* originatingObject, QNetworkAccessManager* networkAccessManager) > + : NetworkingContext() > + , m_frame(frame) > + , m_originatingObject(originatingObject) > + , m_networkAccessManager(networkAccessManager) > +{ > + > +} No need for that "NetworkingContext()". No need for the blank line in the function body. > +FrameNetworkingContext::FrameNetworkingContext(Frame* frame, String userAgent) > + : m_frame(frame) > + , m_userAgent(userAgent) > +{ > + > +} No need for the blank line in the function body. > + virtual void invalidate() = 0; > + > + friend class FrameLoader; Please don't make FrameLoader a friend. That introduces a layering violation, since FrameLoader is not part of the platform layer. As I suggested above, please leave the invalidate function out of NetworkingContext entirely. > + Frame* frame = core(m_webFrame.get()); > + return FrameNetworkingContext::create(frame); The local variable doesn't seem to be needed or helpful here. Just nest the function calls. Created attachment 64165 [details]
NetworkingContext v6 (all)
Another attempt to address Darin's comments and to have all ports working. Let's see how it goes...
I still need to update Changelogs, I leave this for a final step.
The implementations of FrameNetworkingContext were moved to WebKit and the invalidate() is now removed from NetworkingContext, being left only in FrameNetworkingContext.
Some changes were needed in order to avoid cyclic dependency between header's include and another ones to avoid the usage of static_cast.
Hopefully now we are almost there. :)
(In reply to comment #72) > Created an attachment (id=64165) [details] Maybe I should protect the default constructor of FrameNetworkingContext.h in order to prevent people from trying to use it directly? Attachment 64165 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3742066 Created attachment 64221 [details]
NetworkingContext v6
Added Changelogs and fix GTK build.
I just don't know why Win EWS never applies this.
(In reply to comment #75) > I just don't know why Win EWS never applies this. I ran into the same problem with applying a patch which has parts with Windows file endings (\r\n). I think this is an svn-apply bug. I suspect it's interaction between the line endings of your patch, and svn:eol-style=native. It is possible this could be worked around with changes to svn-apply. (In reply to comment #76) > (In reply to comment #75) > > I just don't know why Win EWS never applies this. > > I ran into the same problem with applying a patch which has parts with Windows file endings (\r\n). I think this is an svn-apply bug. I looked into this briefly a while ago and from what I recall the issue is the patch command-line tool, which is called by svn-apply to apply a patch. The patch tool included in the WebKit Cygwin distribution has issues applying diffs that contain Windows line endings. I'll look into this further. (In reply to comment #78) > (In reply to comment #76) > > (In reply to comment #75) > > > I just don't know why Win EWS never applies this. > > > > I ran into the same problem with applying a patch which has parts with Windows file endings (\r\n). I think this is an svn-apply bug. > > I looked into this briefly a while ago and from what I recall the issue is the patch command-line tool, which is called by svn-apply to apply a patch. The patch tool included in the WebKit Cygwin distribution has issues applying diffs that contain Windows line endings. I'll look into this further. I found this discussion: http://kerneltrap.org/mailarchive/git/2008/4/16/1450834 Maybe it can help... This is really bad, since I don't have access to a Windows machine I can never know if this patch introduces windows build problems or not. =/ Created attachment 64227 [details]
NetworkingContext v6 - rebased
Just rebased on trunk so EWSs can apply it.
Attachment 64227 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3717084 Created attachment 64237 [details]
NetworkingContext v6
Added another GTK build fix.
Created attachment 64243 [details] NetworkingContext v6 Fixed GTK's Http authentication dialog issues after talking to Gustavo Noronha. More information about this in comment #27 of this bug. Comment on attachment 64243 [details] NetworkingContext v6 > + virtual PassRefPtr<FrameNetworkingContext> createNetworkingContext() { return PassRefPtr<FrameNetworkingContext>(); } This either needs ASSERT_NOT_REACHED, or we need to change things so FrameLoader can deal with getting a null pointer. > + m_networkingContext->invalidate(); This is an example of a line of code that will crash if a FrameLoader is used with the empty client. It's possible this is the only one, but I think there are other places using FrameLoader::networkingContext() without null-checking it. > +class FrameNetworkingContext : public NetworkingContext { > +public: > + // If we are no longer attached to a Page, this must be an attempted load from an > + // onUnload handler, so the FrameNetworkingContext must be invalid. > + virtual bool isValid() const { return m_frame && m_frame->page(); } I suggest making this virtual function override private. If someone is using it on a FrameNetworkingContext, it's wasteful for them to make a virtual function call. Making the override private will catch that sort of mistake. > +protected: > + FrameNetworkingContext() { } I suggest having a Frame* argument to the constructor. > + Frame* m_frame; I suggest making m_frame private rather than protected. We can initialize m_frame by passing it to the constructor and if we need to get m_frame we can use a protected getter function. It's best practice to have all data members be private. > +#if PLATFORM(QT) > + virtual QObject* originatingObject() const = 0; > + virtual QNetworkAccessManager* networkAccessManager() const = 0; > + > +#elif PLATFORM(MAC) > + virtual bool needsSiteSpecificQuirks() const = 0; > + virtual bool localFileContentSniffingEnabled() const = 0; > + virtual SchedulePairHashSet* scheduledRunLoopPairs() const = 0; > + virtual ResourceError blockedError(const ResourceRequest&) const = 0; > + > +#elif PLATFORM(WIN) > + virtual String userAgent() const = 0; > + virtual String referrer() const = 0; > +#endif I think these would be better as separate #if/#endif rather than using #elif. Later some of these might overlap. > --- a/WebCore/platform/network/ResourceHandle.h > +++ b/WebCore/platform/network/ResourceHandle.h > @@ -29,6 +29,7 @@ > #include "AuthenticationChallenge.h" > #include "AuthenticationClient.h" > #include "HTTPHeaderMap.h" > +#include "NetworkingContext.h" I would like to see this header file use a forward declaration of NetworkingContext rather than including the header. > --- a/WebCore/platform/network/ResourceHandleInternal.h > +++ b/WebCore/platform/network/ResourceHandleInternal.h > @@ -51,9 +51,9 @@ class Frame; > #endif > > #if PLATFORM(QT) > -class QWebFrame; > class QWebNetworkJob; > namespace WebCore { > +class NetworkingContext; > class QNetworkReplyHandler; > } I'm surprised this forward declaration is needed. I'd think that files including this would also include ResourceHandle.h which already takes care of declaring this. > + // If NetworkingContext is invalid then we are no longer attached to a Page, > + // this must be an attempted load from an onUnload handler, so let's just block it. I think we should say "unload event handler" instead of "onUnload handler". It's too bad this code has to be repeated in all the different ResourceHandle::start implementations. It would be better if we refactored this so the code is shared. I'm a little worried about ResourceHandle functions that take a NetworkingContext* argument and then use the context repeatedly without taking a reference to it. If at any point the frame could go away, it's important we ref the NetworkingContext and perhaps even check isValid again. This, however, should not be different from the Frame* lifetime before, so maybe I should stop worrying. > --- a/WebKit/win/ChangeLog > +++ b/WebKit/win/ChangeLog > @@ -1,3 +1,38 @@ > +2010-08-12 Jesus Sanchez-Palencia <jesus.palencia@openbossa.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Add NetworkingContext to avoid layer violations > + https://bugs.webkit.org/show_bug.cgi?id=42292 > + > + Add Win's specific implementation of NetworkingContext. > + > + * WebCoreSupport/WebFrameNetworkingContext.cpp: Added. > + (create): > + (WebFrameNetworkingContext::userAgent): > + (WebFrameNetworkingContext::referrer): > + * WebCoreSupport/WebFrameNetworkingContext.h: Added. > + (WebFrameNetworkingContext::WebFrameNetworkingContext): > + * WebFrame.cpp: > + (WebFrame::createNetworkingContext): > + * WebFrame.h: > + * WebKit.vcproj/WebKit.vcproj: > + > +2010-07-28 Jesus Sanchez-Palencia <jesus.palencia@openbossa.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Add NetworkingContext to avoid layer violations > + https://bugs.webkit.org/show_bug.cgi?id=42292 > + > + Implemented FrameLoaderClient::networkingContext() > + which creates and returns a pointer to the platform > + specific NetworkingContext. > + > + * WebFrame.cpp: > + (WebFrameLoaderClient::networkingContext): > + * WebFrame.h: Doubled change log entry here. Looking good. (In reply to comment #84) > > --- a/WebCore/platform/network/ResourceHandle.h > > +++ b/WebCore/platform/network/ResourceHandle.h > > +#include "NetworkingContext.h" > > I would like to see this header file use a forward declaration of NetworkingContext rather than including the header. For building Qt we need this because of the RefPtr usage. Don't know if this is related to Qt only or if it's general issue. I've just tried to remove it and add a forward declaration instead and it breaks the build. > I think we should say "unload event handler" instead of "onUnload handler". > > It's too bad this code has to be repeated in all the different ResourceHandle::start implementations. It would be better if we refactored this so the code is shared. Yes, true, but some ports actually do accept a context even though it is null. IIRC, Gtk is a good example of it. > I'm a little worried about ResourceHandle functions that take a NetworkingContext* argument and then use the context repeatedly without taking a reference to it. If at any point the frame could go away, it's important we ref the NetworkingContext and perhaps even check isValid again. This, however, should not be different from the Frame* lifetime before, so maybe I should stop worrying. Most places we are now using a NetworkingContext were places where a Frame* was used without any checks. For safety I've added checks where it seems to be needed. I'm doing a local build for Qt and Mac (the ones I can) and then I'll upload an updated version of the patch. Thanks for the review! Created attachment 64271 [details]
NetworkingContext v7
Updated and rebased.
Attachment 64271 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3754090 (In reply to comment #85) > (In reply to comment #84) > > > --- a/WebCore/platform/network/ResourceHandle.h > > > +++ b/WebCore/platform/network/ResourceHandle.h > > > +#include "NetworkingContext.h" > > > > I would like to see this header file use a forward declaration of NetworkingContext rather than including the header. > > For building Qt we need this because of the RefPtr usage. Don't know if this is related to Qt only or if it's general issue. I've just tried to remove it and add a forward declaration instead and it breaks the build. The RefPtr usage should only affect the constructor, destructor, and any function trying to set the value of the RefPtr. As long as none of those are defined in the header file we should be OK. Could you show me the specific build failure at some point? (In reply to comment #88) > The RefPtr usage should only affect the constructor, destructor, and any function trying to set the value of the RefPtr. As long as none of those are defined in the header file we should be OK. > > Could you show me the specific build failure at some point? Yes, it's exactly related to that. We are defining (and protecting) the default ctor and dtor of NetworkingContext in its header. Created attachment 64341 [details]
NetworkingContext v7
Fix the last issues with GTK and Mac.
Attachment 64341 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3739139 Created attachment 64352 [details]
NetworkingContext v7
I'm re-assigning this bug to Diego Gonzalez, a co-worker of mine. I'm moving to another country tomorrow and I won't be able to do any further work on this for the next 3 or 4 weeks. Since the patch is almost there, I've asked Diego to keep this work going on until the patch is reviewed, landed and we don't have any regressions. Sorry for any inconvenience and thank you all the help. Thank you Daring, for the guidance. Also Luiz Agostini (luiz@webkit.org) is willing to help. *** Bug 43351 has been marked as a duplicate of this bug. *** I think I’m going to land the parts of this that affect the build system first, then we can land a version that makes a real substantive change separately. Checked in a first step on this as <http://trac.webkit.org/changeset/65579>. This should get the build system part out of the way so we can concentrate on the contents of the files. Comment on attachment 64352 [details]
NetworkingContext v7
Clearing the flag from this version, since now we need to rebase to TOT.
Created attachment 64998 [details]
Provide NetworkingContext access
Continuing to land Jesus's patch. This part consist in:
Create and provide access to NetworkingContext in FrameLoader and implement of FrameNetworkingContext for each port.
It is still a preparation to NetworkingContext to be activated and work for all ports.
Darin could you look?
Attachment 64998 [details] did not build on win: Build output: http://queues.webkit.org/results/3766420 Diego, ain't you missing an implementation for WebKit2/Qt? (In reply to comment #100) > Diego, ain't you missing an implementation for WebKit2/Qt? I was intending to land WebKit2/Qt impl in another part. As the Qt bot now are build WebKit2 I will update this patch with WebKit2/Qt. Now I'm taking care in Windows impl to do not break the bots Comment on attachment 64998 [details] Provide NetworkingContext access > +NetworkingContext* FrameLoader::networkingContext() > +{ > + return m_networkingContext.get(); > +} Seems to me this should be inline in the header. No reason to pay function call overhead just to fetch the value of a field. It's a little strange to add all of this as dead code. Not quite the same thing as adding only the header files, but I guess still a good idea. Diego, can you list what needs to be done after this patch has been landed? (In reply to comment #103) > Diego, can you list what needs to be done after this patch has been landed? After that I intending to land the last part (mainly ResourceHandle related) which really activate the NetworkingContext. I'm doing some tests now to try do reduce the possibilities of break any port. Created attachment 65835 [details]
Just to see the bots output (not to review and/or commit)
Attachment 65835 [details] did not build on win: Build output: http://queues.webkit.org/results/3875078 (In reply to comment #106) > Attachment 65835 [details] did not build on win: > Build output: http://queues.webkit.org/results/3875078 This build output does not show me anything. My win build env is not building well, I'm trying to fix it. If anyone could help with win build will very great :) Are we making progress here? It would be nice to have functional Qt WebKit2 in ToT. Should I make a patch and finish this? (In reply to comment #108) > Are we making progress here? It would be nice to have functional Qt WebKit2 in ToT. Should I make a patch and finish this? I agree and think that it would be totally fine if you did so! (In reply to comment #109) > (In reply to comment #108) > > Are we making progress here? It would be nice to have functional Qt WebKit2 in ToT. Should I make a patch and finish this? > > I agree and think that it would be totally fine if you did so! The problem now is the Windows build. The rest is ok Created attachment 66417 [details]
Provide NetworkingContext access
This patch should now build on window. Let's assure it watching the bots
Created attachment 66426 [details]
Provide NetworkingContext access (land preparation)
Lets assure this patch will not break any bot
Comment on attachment 66426 [details] Provide NetworkingContext access (land preparation) Patch landed at r66794. Next step will be Networking context activation Sorry for commenting on the patch to late. The think that I do not like in the patch is the copied files from WebKit to WebKit2. I am convinced about we should creating a new directory for code that could be shared between WebKit and WebKit2. I would call this as WebKitShared. Do you feel it reasonable? (In reply to comment #114) > Sorry for commenting on the patch to late. The think that I do not like in the patch is the copied files from WebKit to WebKit2. I am convinced about we should creating a new directory for code that could be shared between WebKit and WebKit2. I would call this as WebKitShared. Do you feel it reasonable? Seems reasonable, I think we can start this discussion. My first choice location for code that needs to be shared between WebKit and WebKit2 would be to find a way to put the code in WebCore. I don’t think WebKitShared is a good idea. We can talk about specific examples. Created attachment 66587 [details]
Activate NetworkingContext
This is the last part of Jesus work. It will really activate the Networking Context for all ports
Comment on attachment 66587 [details] Activate NetworkingContext > diff --git a/WebCore/platform/network/ResourceHandle.h b/WebCore/platform/network/ResourceHandle.h > index d52076c..17d1472 100644 > --- a/WebCore/platform/network/ResourceHandle.h > +++ b/WebCore/platform/network/ResourceHandle.h > @@ -29,6 +29,7 @@ > #include "AuthenticationChallenge.h" > #include "AuthenticationClient.h" > #include "HTTPHeaderMap.h" > +#include "NetworkingContext.h" This file only needs a forward declaration of NetworkingContext. It doesn't need to include the entire header. Also, I would expect to see at least one or two cases where we remove includes of Frame.h or other headers outside the platform directory from the files in the platform directory. Attachment 66587 [details] did not build on win: Build output: http://queues.webkit.org/results/3959136 (In reply to comment #116) > My first choice location for code that needs to be shared between WebKit and WebKit2 would be to find a way to put the code in WebCore. I don’t think WebKitShared is a good idea. We can talk about specific examples. I am totally ok with WebCore but I am obscure about the name. Could you suggest something? (In reply to comment #120) > (In reply to comment #116) > > My first choice location for code that needs to be shared between WebKit and WebKit2 would be to find a way to put the code in WebCore. I don’t think WebKitShared is a good idea. We can talk about specific examples. > > I am totally ok with WebCore but I am obscure about the name. Could you suggest something? I am not suggesting a entire directory with the concept "code to be shared between WebKit and WebKit2". I am suggesting that when we find specific code we want to share we find a way to put it in WebCore. We need a specific example to talk about, so I can show you what I mean. (In reply to comment #121) > (In reply to comment #120) > > (In reply to comment #116) > > > My first choice location for code that needs to be shared between WebKit and WebKit2 would be to find a way to put the code in WebCore. I don’t think WebKitShared is a good idea. We can talk about specific examples. > > > > I am totally ok with WebCore but I am obscure about the name. Could you suggest something? > > I am not suggesting a entire directory with the concept "code to be shared between WebKit and WebKit2". I am suggesting that when we find specific code we want to share we find a way to put it in WebCore. We need a specific example to talk about, so I can show you what I mean. My candidates would be the FrameNetworkContext files for qt and win but I have just realized that the WebKit and WebKit2 versions are not exactly the same. (I thought that the "copied from" message in the changeset page means that the file was created with svn copy.) Currently there is no copy-pasted file so we can hold over this decision. Sorry for the confusion. Comment on attachment 64998 [details] Provide NetworkingContext access Cleared Darin Adler's review+ from obsolete attachment 64998 [details] so that this bug does not appear in http://webkit.org/pending-commit. Created attachment 67244 [details]
Activate NetworkingContext (preparing to land)
Just to see the bots output
Created attachment 67247 [details]
Activate NetworkingContext (preparing to land) v2
Just to see the bots output
Comment on attachment 66587 [details] Activate NetworkingContext Cleared Darin Adler's review+ from obsolete attachment 66587 [details] so that this bug does not appear in http://webkit.org/pending-commit. I haven't read this whole bug, but we need to remove Frame from the WebCore/platform/network abstractions. I plan to look at doing that next week. All the code that knows about Frame in WebCore/platform/network is wrong and needs to go. (In reply to comment #127) > I haven't read this whole bug, but we need to remove Frame from the WebCore/platform/network abstractions. I plan to look at doing that next week. All the code that knows about Frame in WebCore/platform/network is wrong and needs to go. Ok Adam, the patch is almost ready land, but we can wait your comments. So, please point your opinions. Maybe we can correct this issues in another patch? Oh, go ahead with this patch. I'll need to study how all this stuff works first anyway. (In reply to comment #129) > Oh, go ahead with this patch. I'll need to study how all this stuff works first anyway. Thanks Adam :) Comment on attachment 67247 [details] Activate NetworkingContext (preparing to land) v2 Patch landed at r67291 All patches landed. Closing bug |