[webkit-reviews] review denied: [Bug 70836] [chromium] add API methods to support renderer swaps : [Attachment 113578] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Karl Koscher
<supersat at chromium.org>'s request for review:
Bug 70836: [chromium] add API methods to support renderer swaps
https://bugs.webkit.org/show_bug.cgi?id=70836

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113578&action=review


I'm curious why you require these APIs.  In our meeting, we talked about simply
truncating the data stream for a cross-origin document load.  Why did you
decide not to go that route?

> Source/WebKit/chromium/public/WebDocument.h:75
> +    WEBKIT_EXPORT static WebDocument create(const WebString& MIMEType,

nit: mimeType <-- webkit style

> Source/WebKit/chromium/public/WebDocument.h:76
> +					       const WebFrame*,

nit: I'd probably list the WebFrame parameter first since it is the container
for the WebDocument.

> Source/WebKit/chromium/public/WebDocument.h:77
> +					       const WebURL&,

By passing a WebURL here, it seems like it is possible for the WebFrame and the
WebDocument to have different ideas about what URL is loaded in the frame.  Is
that really helpful?  It seems like it could cause subtle bugs.  Another idea
would be to infer the URL of the document from the frame.

> Source/WebKit/chromium/public/WebDocument.h:78
> +					       bool inViewSourceMode);

nit: is it strictly necessary to pass the inViewSourceMode parameter here? 
can't you just use WebFrame::enableViewSourceMode(true)?

> Source/WebKit/chromium/src/WebDocument.cpp:70
> +	   MIMEType, frame ? static_cast<const WebFrameImpl*>(frame)->frame() :
0,

Do you have a use case for frame being null here?  Maybe we should require that
it is non-null.