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

[bugzilla-daemon at webkit.org]

Karl Koscher <supersat at chromium.org> has canceled  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 Karl Koscher <supersat at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113578&action=review


We decided to implement swapping out this way because it lets us get rid of the
renderer-side IPC message filtering. If we were to just send back an blank
cross-site response, we'd still need to filter out all of the navigation
messages the renderer would send.

>> Source/WebKit/chromium/public/WebDocument.h:75
>> +	WEBKIT_EXPORT static WebDocument create(const WebString& MIMEType,
> 
> nit: mimeType <-- webkit style

Fixed.

>> 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.

Fixed.

>> 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.

Where does the WebFrame keep the current URL? I couldn't find anywhere that it
does...

>> 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)?

Fixed.

>> 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.

No, I don't. Fixed.