Bug 12311 - Implement frame loading for Qt. Clean up FrameLoader a bit
Summary: Implement frame loading for Qt. Clean up FrameLoader a bit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 420+
Hardware: Other Linux
: P1 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-18 07:25 PST by Lars Knoll
Modified: 2007-01-23 07:25 PST (History)
1 user (show)

See Also:


Attachments
see bug description (25.19 KB, patch)
2007-01-18 07:26 PST, Lars Knoll
no flags Details | Formatted Diff | Diff
new version (26.82 KB, patch)
2007-01-19 00:26 PST, Lars Knoll
no flags Details | Formatted Diff | Diff
Fixes Lars' patch to compile on Mac (27.85 KB, patch)
2007-01-19 00:40 PST, Adam Roben (:aroben)
no flags Details | Formatted Diff | Diff
new patch refactoring also the Mac code (65.40 KB, patch)
2007-01-22 11:11 PST, Lars Knoll
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lars Knoll 2007-01-18 07:25:36 PST
The patch implements support for creation and loading of subframes in the Qt build. At the same time it cleans up some of the remaining platform dependent code in FrameLoader. It moves some code that is platform independent into loadSubFrame and adds stubs for most of the platform dependent FrameLoader methods to the FrameLoaderClient (so that FrameLoader can become 100% platform independent one day). On the Mac side it only cleans up a little, but makes it possible to move quite a bit of code from FrameMac and the WebFrameBridge to WebFrameLoaderClient.

The code is untested on th Mac, so someone needs to give it a try to see if it compiles and works.
Comment 1 Lars Knoll 2007-01-18 07:26:17 PST
Created attachment 12533 [details]
see bug description
Comment 2 Adam Roben (:aroben) 2007-01-18 23:34:30 PST
Comment on attachment 12533 [details]
see bug description

 #include "HTMLFormElement.h"
 #include "HTMLNames.h"
 #include "HTMLObjectElement.h"
+#include "HTMLFrameElement.h"

   #includes should be alphabetical.

+    if (ownerElement->hasTagName(frameTag) || ownerElement->hasTagName(iframeTag)) {
+        HTMLFrameElement* o = static_cast<HTMLFrameElement*>(ownerElement);

   I know this isn't new code, but it seems like this cast should really be to HTMLFrameElementBase, since that's what both HTMLFrameElement and HTMLIFrameElement derive from. Also, HTMLFrameElement and HTMLIFrameElement are the only things that can be HTMLFrameOwnerElements, so I'm not sure what value the if has.

     class ResourceRequest;
     class ResourceResponse;
     class String;
+    class HTMLFrameOwnerElement;
+    class Widget;
+    class IntSize;

   Please alphabetize these forward declarations.

+        virtual Frame *createFrame(const KURL& url, const String& name, HTMLFrameOwnerElement* ownerElement,
+                                   const String& referrer, bool allowsScrolling, int marginWidth, int marginHeight) = 0;

   The * should be next to Frame, and the 'ownerElement' parameter name is unnecessary. (And ditto in WebFrameLoaderClient.h)

+    // stubbed out until the code is moved here from the FrameBridge, and FrameLoader looses the createFrame() method

    Typo: 'looses' => 'loses'

   I think it would be good for Darin to have a look at this before checking in.
Comment 3 Adam Roben (:aroben) 2007-01-18 23:55:58 PST
Comment on attachment 12533 [details]
see bug description

This patch also needs to update SVGEmptyFrameLoaderClient in SVGImageEmptyClients.h
Comment 4 Lars Knoll 2007-01-19 00:26:14 PST
Created attachment 12551 [details]
new version
Comment 5 Adam Roben (:aroben) 2007-01-19 00:40:18 PST
Created attachment 12552 [details]
Fixes Lars' patch to compile on Mac

This patch fixes Lars' latest patch to compile on Mac (changes to WebFrameLoaderClient.(h|mm) were made).
Comment 6 Adam Roben (:aroben) 2007-01-19 00:48:53 PST
Comment on attachment 12552 [details]
Fixes Lars' patch to compile on Mac

I've confirmed that this patch causes no layout test regressions on Mac.
Comment 7 Darin Adler 2007-01-20 08:17:23 PST
Comment on attachment 12552 [details]
Fixes Lars' patch to compile on Mac

I don't think this is a good way to handle the Mac side. If the functions still exist on the bridge that's fine. But they should be called through the frame loader client. The client can call to the bridge.
Comment 8 Lars Knoll 2007-01-21 03:02:53 PST
I agree, and that was the basic idea behind the change. But it was hard to change this without having a Mac at hand. Fortunately I got an iMac at work on Friday. 

So I can now move the Mac side over to the client as well. I'll prepare a new patch on Monday.

Lars
Comment 9 Lars Knoll 2007-01-22 11:11:13 PST
Created attachment 12610 [details]
new patch refactoring also the Mac code
Comment 10 Lars Knoll 2007-01-22 11:27:12 PST
My last patch compiles on the Mac and passes the regression tests. It makes Frames work for the Qt build, although it still has some smaller issues there (failing some frame related dom tests).
Comment 11 Maciej Stachowiak 2007-01-23 01:44:45 PST
Comment on attachment 12610 [details]
new patch refactoring also the Mac code

r=me but please fix the following:

1) "pieced" in the ChangeLog should be "pieces"
2) The below should use HTMLFrameElementBase.

+        HTMLFrameElement* o = static_cast<HTMLFrameElement*>(ownerElement);

Also please test Java since the LayoutTests don't cover it.
Comment 12 Maciej Stachowiak 2007-01-23 01:46:31 PST
Comment on attachment 12552 [details]
Fixes Lars' patch to compile on Mac

removing review flag and obsoleting
Comment 13 David Kilzer (:ddkilzer) 2007-01-23 07:25:50 PST
Please include Bugzilla bug numbers in ChangeLog entries in the future.  Thanks!

Fixed in r19042.