[webkit-reviews] review denied: [Bug 62112] [chromium] Add plumbing to support pasting context in HTML paste : [Attachment 109219] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 62112: [chromium] Add plumbing to support pasting context in HTML paste
https://bugs.webkit.org/show_bug.cgi?id=62112

Attachment 109219: Patch
https://bugs.webkit.org/attachment.cgi?id=109219&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109219&action=review


I'm sad that there's no way for us to test this.

> Source/WebCore/editing/markup.cpp:50
> +#include "FileChooser.h"
> +#include "FormState.h"

Why are these headers required here?

> Source/WebCore/editing/markup.cpp:54
> +#include "HTMLDocument.h"

Ditto.

> Source/WebCore/editing/markup.cpp:56
> +#include "HTMLFormElement.h"

Ditto.

> Source/WebCore/editing/markup.cpp:77
> +namespace {

Why do we need an anonymous namespace? We don't normally do this in WebCore.

> Source/WebCore/editing/markup.cpp:80
> +Page* pageForParsing = 0;
> +
> +bool ensurePageForParsingInitialized()

I'd prefer this function returned a page so rename it to something like "Page*
pageForPaste()".

> Source/WebCore/editing/markup.cpp:85
> +    static FrameLoaderClient* dummyFrameLoaderClient = new
EmptyFrameLoaderClient;

You need to use DEFINE_STATIC_LOCAL. And please use OwnPtr.

> Source/WebCore/editing/markup.cpp:763
> +    // We use comment tags to help us extract the appropriate
DocumentFragment.
> +    const char beginTag[] = "3f81c6fb-e92e-417d-a05e-fd6f7a6c883c";
> +    const char endTag[] = "3879efc4-7f0a-4352-ba73-61158bbecd70";

I'd prefer using webkit prefix e.g. <!--webkit-fragment-marker-->
or<?-webkit-fragment-marker?>  Also we should make sure that this string
doesn't exist inside the markup and if it does, we need to append appropriate
text to distinguish ours from them.

> Source/WebCore/editing/markup.cpp:783
> +    // TODO: We need to do some cleanup before loading the new doc... figure
out what.

We don't use TODO in WebKit. It should be FIXME.

> Source/WebCore/editing/markup.cpp:795
> +	       if (static_cast<CharacterData*>(n)->data() == endTag)
> +		   fragmentEndNode = n;
> +	       if (fragmentStartNode && fragmentEndNode)
> +		   break;

So there's a case where fragmentStartNode appears after fragmentStartNode?

> Source/WebCore/editing/markup.cpp:801
> +					   Position(fragmentStartNode,
Position::PositionIsAfterAnchor),
> +					   Position(fragmentEndNode,
Position::PositionIsBeforeAnchor));

Nit: indentation. Position should be indented by exactly 8 spaces and should
NOT be assigned with pageForParsing or RefPtr<Range>.

> Source/WebKit/chromium/public/WebClipboard.h:69
> +    virtual WebString readHTML(
> +	   Buffer buffer, WebURL* pageURL, int* fragmentStart,
> +	   int* fragmentEnd) { return WebString(); }

It seems like we should put all of the them in one line. As I understand it,
Source/WebKit/chromium follows WebKit coding guideline so there's no 80-column
restriction.