[webkit-reviews] review denied: [Bug 4726] Drop of multiple non-image file URLs only yields one item : [Attachment 4781] Proposed patch 3

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Andrew Wellington
<proton at wiretapped.net>'s request for review:
Bug 4726: Drop of multiple non-image file URLs only yields one item
http://bugzilla.opendarwin.org/show_bug.cgi?id=4726

Attachment 4781: Proposed patch 3
http://bugzilla.opendarwin.org/attachment.cgi?id=4781&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This looks great! Excellent work.

A few comments:

    1) Formatting nit:

+    while ((node = [nodeEnum nextObject]))
+    {

In our coding style, the brace is supposed to be on the same line as the while
statement.

    2) The node should be added to the paragraph with appendChild, not with
addChild. The addChild function is for the HTML parser only.

    3) Ideally, the body of this function should be inside markup.cpp -- to do
that the NSArray would have to be converted to some C++ data structure,
presumably a QPtrList<DOM::NodeImpl>. While this would make this patch more
complicated, it would reduce the amount of code that's in Mac OS X specific
Objective-C and make some other improvements practical as well (read on).

    4) This function is different from createFragmentFromText in one
significant way: createFragmentFromText uses a "magic BR" at the end. I
remember precisely why that's necessary (something related to editing of
course), but I think it may be important in this case as well as that one.
Ideally, we'd refactor createFragmentFromText into two pieces, and then code to
put the nodes into paragraphs can be shared by this new function and by
createFragmentFromText. Thus createFragmentFromText would create the text nodes
and put them in a QPtrList and then call the new createFragmentfromNodeList
function.

    5) The fragment returned from createDocumentFragment needs to be ref'd and
deref'd at the appropriate time; it's not legal to just call appendChild on
something newly created and floating; that could cause the fragment to be
deleted. Either use a SharedPtr<DocumentFragmentImpl> or put in explicit
ref/deref calls.