[webkit-reviews] review denied: [Bug 115564] Web Inspector: Implement WK2 version of WebInspectorFrontendClient::save : [Attachment 201268] Patch v5

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Brian Burg
<burg at cs.washington.edu>'s request for review:
Bug 115564: Web Inspector: Implement WK2 version of
WebInspectorFrontendClient::save
https://bugs.webkit.org/show_bug.cgi?id=115564

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=201268&action=review


Some small issues bellow:

> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:315
> +    auto saveToURL = ^(NSURL *actualURL) {
> +	   ASSERT(actualURL);
> +	   
> +	   m_suggestedToActualURLMap.set(suggestedURLCopy, actualURL);
> +	   [contentCopy writeToURL:actualURL atomically:YES
encoding:NSUTF8StringEncoding error:NULL];
> +	   core([m_windowController
webView])->mainFrame()->script()->executeScript([NSString
stringWithFormat:@"InspectorFrontendAPI.savedURL(\"%@\")",
actualURL.absoluteString]);
>      };

What is the point of this block exactly?

> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:350
> +    // do not append unless the user has already confirmed this filename in
save().

do -> Do

> Source/WebKit2/UIProcess/WebInspectorProxy.h:169
> +    void platformSave(const String& filename, const String& content, bool
forceSaveAs);
> +    void platformAppend(const String& filename, const String& content);

I find it intrinsically weird to pass any file content as String. I am not
aware of any API that works like that.

> Source/WebKit2/UIProcess/WebInspectorProxy.h:179
> +    void canSave(bool&);

Where is this defined?

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:515
> +    // Necessary for the block below.
> +    String suggestedURLCopy = suggestedURL;
> +    String contentCopy = content;
> +
> +    auto saveToURL = ^(NSURL *actualURL) {
> +	   ASSERT(actualURL);
> +	   
> +	   m_suggestedToActualURLMap.set(suggestedURLCopy, actualURL);
> +	   [contentCopy writeToURL:actualURL atomically:YES
encoding:NSUTF8StringEncoding error:NULL];
> +	   m_page->process()->send(Messages::WebInspector::DidSave([actualURL
absoluteString]), m_page->pageID());
> +    };

Please reduce the use span for this, move that code closer to where it is used:
after if (!platformURL).

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:550
> +    // do not append unless the user has already confirmed this filename in
save().

do -> Do

> Source/WebKit2/WebProcess/WebPage/WebInspector.h:100
> +    // Implemented in platform WebInspector file

Missing period.

That comment is useless really.