[Webkit-unassigned] [Bug 12287] Patch: make most of Frame platform independent

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=12287


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #12856|review?                     |review-
               Flag|                            |




------- Comment #9 from darin at apple.com  2007-02-02 09:31 PDT -------
(From update of attachment 12856)
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.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.