[webkit-reviews] review denied: [Bug 12287] Patch: make most of Frame platform independent : [Attachment 12856] gets rid of FrameMac and FrameQt

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 12287: Patch: make most of Frame platform independent
http://bugs.webkit.org/show_bug.cgi?id=12287

Attachment 12856: gets rid of FrameMac and FrameQt
http://bugs.webkit.org/attachment.cgi?id=12856&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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.