Bug 12287 - Patch: make most of Frame platform independent
Summary: Patch: make most of Frame platform independent
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 420+
Hardware: Other Other
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2007-01-15 11:02 PST by Lars Knoll
Modified: 2007-02-19 17:04 PST (History)
2 users (show)

See Also:

suggested patch (9.54 KB, patch)
2007-01-15 11:03 PST, Lars Knoll
mjs: review-
Details | Formatted Diff | Diff
new version (60.39 KB, patch)
2007-01-31 01:48 PST, Lars Knoll
no flags Details | Formatted Diff | Diff
gets rid of FrameMac and FrameQt (164.34 KB, patch)
2007-02-01 08:08 PST, Lars Knoll
darin: review-
Details | Formatted Diff | Diff
new version (165.56 KB, patch)
2007-02-02 10:44 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-15 11:02:39 PST
Patch to make a bit more of Frame platform independent (mainly moving implementations that where shared between FrameMac and FrameQt into Frame intself). Also removes an unused method declaration in FrameView.
Comment 1 Lars Knoll 2007-01-15 11:03:39 PST
Created attachment 12460 [details]
suggested patch
Comment 2 Adam Roben (:aroben) 2007-01-15 11:14:17 PST
Comment on attachment 12460 [details]
suggested patch

I think Geoff should review this.
Comment 3 Maciej Stachowiak 2007-01-19 16:54:28 PST
This patch looks good, I'll hold off on r+ because we'd rather have someone at Apple land to sync up with some other things. I'll try to get this in the tree this weekend.
Comment 4 Maciej Stachowiak 2007-01-23 01:17:30 PST
This appears to cause a crash on the Mac. This is in Safari, but DumpRenderTree also crashes. It happesn on the second page loaded.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000010
0x013040fd in -[WebScriptObject _isSafeScript] (self=0x16641350, _cmd=0x1462e14) at /Volumes/Data/mjs/Work/src/Safari/OpenSource/WebCore/bindings/objc/WebScriptObject.mm:109
(gdb) bt
#0  0x013040fd in -[WebScriptObject _isSafeScript] (self=0x16641350, _cmd=0x1462e14) at /Volumes/Data/mjs/Work/src/Safari/OpenSource/WebCore/bindings/objc/WebScriptObject.mm:109
#1  0x01305265 in -[WebScriptObject setValue:forKey:] (self=0x16641350, _cmd=0x90ac5298, value=0x16898560, key=0x19ad3c) at /Volumes/Data/mjs/Work/src/Safari/OpenSource/WebCore/bindings/objc/WebScriptObject.mm:254
#2  0x00099ccb in -[LocationChangeHandler webView:windowScriptObjectAvailable:] (self=0x216aaf0, _cmd=0x90ace020, webView=0x2140120, windowScriptObject=0x16641350) at /Volumes/Data/mjs/Work/src/Safari/Internal/WebBrowser/LocationChangeHandler.m:250
#3  0x90a56c56 in objc_msgSendv ()
#4  0x925fc43e in -[NSInvocation invoke] ()
#5  0x92622433 in -[NSInvocation invokeWithTarget:] ()
#6  0x00461042 in -[_WebSafeForwarder forwardInvocation:] (self=0x1621a300, _cmd=0x90aa5194, anInvocation=0x16eebf80) at /Volumes/Data/mjs/Work/src/Safari/OpenSource/WebKit/WebView/WebView.mm:1426
#7  0x925fb4f4 in -[NSObject(NSForwardInvocation) forward::] ()
#8  0x90a56ba1 in _objc_msgForward ()
#9  0x004299bb in -[WebFrameBridge windowObjectCleared] (self=0x2153cd0, _cmd=0x90ace814) at /Volumes/Data/mjs/Work/src/Safari/OpenSource/WebKit/WebCoreSupport/WebFrameBridge.mm:930
#10 0x0139a152 in WebCore::FrameLoader::partClearedInBegin (this=0x2880400) at /Volumes/Data/mjs/Work/src/Safari/OpenSource/WebCore/loader/mac/FrameLoaderMac.mm:198
#11 0x013bc1f4 in WebCore::FrameLoader::begin (this=0x2880400, url=@0x28805e8) at /Volumes/Data/mjs/Work/src/Safari/OpenSource/WebCore/loader/FrameLoader.cpp:796
#12 0x013bc6eb in WebCore::FrameLoader::receivedFirstData (this=0x2880400) at /Volumes/Data/mjs/Work/src/Safari/OpenSource/WebCore/loader/FrameLoader.cpp:753
#13 0x013bc8cb in WebCore::FrameLoader::setEncoding (this=0x2880400, name=@0xbfffe4e0, userChosen=false) at /Volumes/Data/mjs/Work/src/Safari/OpenSource/WebCore/loader/FrameLoader.cpp:1487
#14 0x010ff8a4 in -[WebCoreFrameBridge receivedData:textEncodingName:] (self=0x2153cd0, _cmd=0x90ab9160, data=0x162caca0, textEncodingName=0x0) at /Volumes/Data/mjs/Work/src/Safari/OpenSource/WebCore/page/mac/WebCoreFrameBridge.mm:1583
#15 0x004328ad in -[WebHTMLRepresentation receivedData:withDataSource:] (self=0x17023360, _cmd=0x90ab9180, data=0x162caca0, dataSource=0x162b8a80) at /Volumes/Data/mjs/Work/src/Safari/OpenSource/WebKit/WebView/WebHTMLRepresentation.mm:172
#16 0x0042e047 in -[WebDataSource(WebInternal) _receivedData:] (self=0x162b8a80, _cmd=0x90a820f8, data=0x162caca0) at /Volumes/Data/mjs/Work/src/Safari/OpenSource/WebKit/WebView/WebDataSource.mm:177
Comment 5 Maciej Stachowiak 2007-01-23 01:20:46 PST
Comment on attachment 12460 [details]
suggested patch

r- since this crashes on mac - Lars will revise.
Comment 6 Lars Knoll 2007-01-31 01:48:45 PST
Created attachment 12819 [details]
new version
Comment 7 Lars Knoll 2007-01-31 02:49:30 PST
Comment on attachment 12819 [details]
new version

Will go for a different approach after discussing with Maciej. Hope I'll have a new patch soon.
Comment 8 Lars Knoll 2007-02-01 08:08:24 PST
Created attachment 12856 [details]
gets rid of FrameMac and FrameQt

The patch get rid of FrameMac and FrameQt, and makes most of Frame platform independent. In addition it cleans up some things in the WebCoreFrameBridge and moves some methods away from Frame to the places they belong.

It also removes some now unused code paths in WebKit.

Compiled and tested for Mac and Qt.
Comment 9 Darin Adler 2007-02-02 09:31:06 PST
Comment on attachment 12856 [details]
gets rid of FrameMac and FrameQt

Great work! This looks really good to me. My only complaint is that it's a lot at once.

+    return m_frame->page()->chrome()->shouldInterruptJavaScript();

Page can be 0 -- we probably need a null check here.

+#include "bindings/NP_jsobject.h"

This doesn't look exactly right to me. Usually we treat things in the other framework as a "library" with a name like wtf or kjs and use <> include syntax to accomodate platforms where these are separate libraries rather than part of a JavaScriptCore framework. And we use ForwardingHeaders to make it work on the Mac where it's a framework, not libraries. It's strange for "bindings" to be a top level library name; can we make this work with "kjs/bindings" instead (may be trouble because of how things are laid out in JavaScriptCore)? The headers in the bindings directory also have awful names. I guess we can live with this, but it seems to be new ugliness. Long term I think we want the bindings to move out of JavaScriptCore since they are about the NPAPI, not the JavaScript language.

+    if (d->m_editor.clientIsEditable())
+	return true;

Looks like a tab was used here by accident.

Frame.cpp is now inconsistent about whether it does editor() or d->m_editor. Ideally I'd like us to be consistent one way or the other.

+    WebScriptObject* windowScriptObject();

We might want a name for this that makes it clear it's Objective-C. Maybe windowScriptNSObject or something. Doesn't have to be in this patch.

+    if (!bridge) {
+	[d->m_bridge clearFrame];
+	HardRelease(d->m_bridge);
+	d->m_bridge = nil;
+	return;
+    }

More tabs in the patch.

It's probably better to make m_bridge and m_windowScriptObject use RetainPtr to avoid those explicit HardRetain and HardRelease calls, but I suppose that can be a separate patch.

Sorry about our confusing rules about * -- the * is supposed to be after the space in the case of Objective-C classes. I think that's a very hard to maintain rule so I'm not going to insist on it here. We should probably revise our rules.

+    if([type length] == 0)

Missing space here after the if.

Looks great. review- for the missing Page null check, but otherwise looks good.
Comment 10 Lars Knoll 2007-02-02 10:44:27 PST
Created attachment 12881 [details]
new version

Sorry about the length of the patch. It's a bit because of timezone issues that make it hard for me to get an intermediate review of smaller pieces. 

Added the 0 pointer check for page and used d->m_editor throughout Frame.cpp. Also removed all tabs that sneaked in.

I agree that windowScriptObject should have a different name. I'm not changing it now, as I currently don't have access to a mac.

And: I would very much vote for having the same coding style in C++ and objc. anything else is highly confusing.
Comment 11 Lars Knoll 2007-02-03 01:26:28 PST
I'm gone for one week of vacation now. Would be nice if someone could land that patch for me if it get's an r+ :)

Comment 12 Maciej Stachowiak 2007-02-19 14:43:34 PST
Comment on attachment 12881 [details]
new version

Reviewed and landed (with some changes).
Comment 13 Sam Weinig 2007-02-19 17:04:25 PST
Landed in r19702.