Bug 12287 – Patch: make most of Frame platform independent

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Lars Knoll]

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.

[Lars Knoll]

Created attachment 12460 [details]
suggested patch

[Adam Roben (:aroben)]

Comment on attachment 12460 [details]
suggested patch

I think Geoff should review this.

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

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

[Maciej Stachowiak]

Comment on attachment 12460 [details]
suggested patch

r- since this crashes on mac - Lars will revise.

[Lars Knoll]

Created attachment 12819 [details]
new version

[Lars Knoll]

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.

[Lars Knoll]

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.

[Darin Adler]

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.

[Lars Knoll]

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.

[Lars Knoll]

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+ :)

Cheers,
Lars

[Maciej Stachowiak]

Comment on attachment 12881 [details]
new version

Reviewed and landed (with some changes).

[Sam Weinig]

Landed in r19702.