[Webkit-unassigned] [Bug 115564] Web Inspector: Implement WK2 version of WebInspectorFrontendClient::save

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=115564


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #201268|review?                     |review-
               Flag|                            |




--- Comment #27 from Benjamin Poulain <benjamin at webkit.org>  2013-05-10 12:34:07 PST ---
(From update of attachment 201268)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.