[webkit-reviews] review denied: [Bug 5727] We want to evaluate scripts in viewless documents : [Attachment 21870] 2nd version - updated with Darin's comments

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Julien Chaffraix
<julien.chaffraix at gmail.com>'s request for review:
Bug 5727: We want to evaluate scripts in viewless documents
https://bugs.webkit.org/show_bug.cgi?id=5727

Attachment 21870: 2nd version - updated with Darin's comments
https://bugs.webkit.org/attachment.cgi?id=21870&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Here's better wording for this comment:

+    // Changing the Document frame should not occurs under normal condition.
+    // Currently only the dummyFrame issue requires it.

    // Changing the Document frame should not occur under normal conditions.
    // Currently only createDummyFrame requires it.

And since this function will be rarely used, it should be moved farther down in
the source file. The most used functions should be up at the top if possible.

This seems to be indented incorrectly:

+    if (!doc->frame())
+	     Frame::setDummyFrameFor(doc, true);

In the createDummyFrame function:

+    Page* page = new Page(dummyChromeClient, dummyContextMenuClient,
dummyEditorClient, dummyDragClient, dummyInspectorClient);

What owns this page? It seems to me that this Page object leaks permanently.
This is the most serious problem with the patch.

+    Frame* frame = new Frame(page, 0, dummyFrameLoaderClient);
+    [...]
+    return adoptRef(frame);

The correct style is to do adoptRef right on the same line as the "new" and
keep it in a RefPtr. So it should be:

    RefPtr<Frame> frame = adoptRef(new Frame(page, 0, dummyFrameLoaderClient));

    [...]
    return frame.release();

We want the new and the adoptRef to be as close to each other as possible.

How about skipping the local variable here:

+    FrameView* frameView = new FrameView(frame);
+    frame->setView(frameView);

Do this instead:

    frame->setView(new FrameView(frame));

This reads strangely to me:

    frame->d->m_loader.init(false);

Please consider the more usual:

    frame->loader()->init(false);

I think the right thing to do is to put the Document::setFrame function call
into createDummyFrame. Everyone will call that function directly. Simply have
it attach the dummy frame to the document automatically if you passed a
document into createDummyFrame. Then we can get rid of the awkwardly name
setDummyFrameFor.

+    static void setDummyFrameFor(Document*, bool shouldEnableJavaScript);

I still think that createDummyFrame belongs in FrameLoader, but I guess you
weren't convinced last time I suggested it.

For this:

+	 Document* doc = document();
+	 if (!doc->frame())

the style I now prefer is:

    Document* document = this->document();
    if (!document->frame())

I don't like using various mixed abbreviations just to dodge member names.

I still don't see any callers passing false for shouldEnableJavaScript to
setDummyFrameFor, despite your earlier comments. Is that coming in a future
patch?

review- because of the Page ownership issue.

I'd also like you to refine the arguments to createDummyFrame and its behavior
further and consider my other comments. But the only essential issue is the
ownership one.