Bug 115564 – Web Inspector: Implement WK2 version of WebInspectorFrontendClient::save

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Brian Burg]

There's a WK1 version of this function, which is/was used to save Timeline data to the filesystem. That implementation uses a modal dialog and requires user interaction to save files, for security reasons.

[Brian Burg]

patch forthcoming.

[Brian Burg]

Created attachment 200997 [details]
initial patch - missing Changelog

[WebKit Commit Bot]

Attachment 200997 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/inspector/InspectorFrontendClientLocal.h', u'Source/WebCore/testing/Internals.cpp', u'Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp', u'Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.h', u'Source/WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp', u'Source/WebKit/gtk/WebCoreSupport/InspectorClientGtk.h', u'Source/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp', u'Source/WebKit/qt/WebCoreSupport/InspectorClientQt.h', u'Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp', u'Source/WebKit/win/WebCoreSupport/WebInspectorClient.h', u'Source/WebKit2/UIProcess/WebInspectorProxy.cpp', u'Source/WebKit2/UIProcess/WebInspectorProxy.h', u'Source/WebKit2/UIProcess/WebInspectorProxy.messages.in', u'Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp', u'Source/WebKit2/UIProcess/gtk/WebInspectorProxyGtk.cpp', u'Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm', u'Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorFrontendClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorFrontendClient.h', u'Source/WebKit2/WebProcess/WebPage/WebInspector.cpp', u'Source/WebKit2/WebProcess/WebPage/WebInspector.h', u'Source/WebKit2/WebProcess/WebPage/WebInspector.messages.in']" exit_code: 1
Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 200997 [details]
initial patch - missing Changelog

Attachment 200997 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/353203

[EFL EWS Bot]

Comment on attachment 200997 [details]
initial patch - missing Changelog

Attachment 200997 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/353207

[Brian Burg]

Created attachment 201000 [details]
Patch

[EFL EWS Bot]

Comment on attachment 201000 [details]
Patch

Attachment 201000 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/275042

[Early Warning System Bot]

Comment on attachment 201000 [details]
Patch

Attachment 201000 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/445018

[Brian Burg]

Created attachment 201001 [details]
Patch

[EFL EWS Bot]

Comment on attachment 201001 [details]
Patch

Attachment 201001 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/275046

[Early Warning System Bot]

Comment on attachment 201001 [details]
Patch

Attachment 201001 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/425051

[Brian Burg]

Created attachment 201005 [details]
Patch

[Timothy Hatcher]

Comment on attachment 201005 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=201005&action=review

Needs reviewed by a WebKit2 owner.

> Source/WebKit2/UIProcess/WebInspectorProxy.h:229
> +    HashMap<String, RetainPtr<NSURL>> m_saveURLs;  

m_savedURLs? m_savedURLToFileURLMap?

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:508
> +    String url = refURL;

Having both url and URL is confusing (and violates style guidelines). Are there more descriptive names you could use?

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:515
> +        m_page->process()->send(Messages::WebInspector::DidSave([URL absoluteString]),
> +                                m_page->pageID());

I wouldn't wrap this across two lines.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:521
> +        URL = [NSURL URLWithString:url];

We should ASSERT and return early if the NSURL is nil after this.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:543
> +        URL = [NSURL URLWithString:url];

Ditto.

> Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:128
> +    bool result;
> +    WebProcess::shared().connection()->sendSync(Messages::WebInspectorProxy::CanSave(), result, m_page->pageID());
> +    return result;

sendSync == bad*. We should avoid it or cache it after the first time, since the UIProcess returns a static true or false.

* Frowned upon.

[Build Bot]

Comment on attachment 201005 [details]
Patch

Attachment 201005 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/318276

[Brian Burg]

Created attachment 201018 [details]
Patch v2

[Timothy Hatcher]

Comment on attachment 201018 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=201018&action=review

I realize some of this is copied from WK1 and wasn't your original code. Your clean up helps! We should consider making WK1 match the final version of this change.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:508
> +    String suggestedURLCopy = suggestedURL;

I don't think it makes a difference to "copy" here, String isn't mutable. Or was it needed for the block?

Should we ASSERT and return early if !suggestedURL.isEmpty()? It would be weird to map empty string to a platform URL.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:512
> +        m_suggestedToActualURLMap.set(suggestedURLCopy, actualURL);

Would be nice to ASSERT(actualURL);

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:521
> +    if (platformURL == nil)
> +        platformURL = [NSURL URLWithString:suggestedURL];
> +    if (platformURL == nil)

WebKit's style is to use ! instead of == NULL/nil/0.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:524
> +    if (!forceSaveAs) {

I think the logic is wrong here now. This use to be an "else if", which would have skipped this if platformURL (then URL) was false. Now the first save does not bring up the save dialog and just uses the suggestedURL, which might not even be a local file URL. You could just set forceSaveAs to true if platformURL came from the suggestedURL.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:546
> +    if (platformURL == nil)
> +        platformURL = [NSURL URLWithString:suggestedURL];
> +    if (platformURL == nil)

Ditto.

[Timothy Hatcher]

Comment on attachment 201018 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=201018&action=review

>> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:508
>> +    String suggestedURLCopy = suggestedURL;
> 
> I don't think it makes a difference to "copy" here, String isn't mutable. Or was it needed for the block?
> 
> Should we ASSERT and return early if !suggestedURL.isEmpty()? It would be weird to map empty string to a platform URL.

Ooops, I meant "ASSERT and return early if suggestedURL.isEmpty()".

[Brian Burg]

Roger, roger. (In reply to comment #16)
> (From update of attachment 201018 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201018&action=review
> 
> I realize some of this is copied from WK1 and wasn't your original code. Your clean up helps! We should consider making WK1 match the final version of this change.

Roger, roger. Should that be a separate bug or rolled into this one?

> 
> > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:508
> > +    String suggestedURLCopy = suggestedURL;
> 
> I don't think it makes a difference to "copy" here, String isn't mutable. Or was it needed for the block?

It's necessary for the block. (I asked the same question and removed it, only to have WebKit crash after the file dialog.) Would a comment be appropriate?

> Should we ASSERT and return early if !suggestedURL.isEmpty()? It would be weird to map empty string to a platform URL.

Noted.
 
> > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:512
> > +        m_suggestedToActualURLMap.set(suggestedURLCopy, actualURL);
> 
> Would be nice to ASSERT(actualURL);
> 
> > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:521
> > +    if (platformURL == nil)
> > +        platformURL = [NSURL URLWithString:suggestedURL];
> > +    if (platformURL == nil)
> 
> WebKit's style is to use ! instead of == NULL/nil/0.

Ah, okay- I don't frequently use ObjC, so wasn't sure if the C++ idiom would translate.
 
> > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:524
> > +    if (!forceSaveAs) {
> 
> I think the logic is wrong here now. This use to be an "else if", which would have skipped this if platformURL (then URL) was false. Now the first save does not bring up the save dialog and just uses the suggestedURL, which might not even be a local file URL. You could just set forceSaveAs to true if platformURL came from the suggestedURL.

Yeah, boolean logic is annoying :-P Good catch!
 
> > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:546
> > +    if (platformURL == nil)
> > +        platformURL = [NSURL URLWithString:suggestedURL];
> > +    if (platformURL == nil)
> 
> Ditto.

I'll address these comments and post a new version tomorrow morning. Thanks for the quick turnaround!

[Brian Burg]

Created attachment 201081 [details]
Patch v3

[Timothy Hatcher]

Comment on attachment 201081 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=201081&action=review

> Source/WebCore/inspector/InspectorFrontendClientLocal.h:-72
> -    virtual bool canSave() { return false; }
> -    virtual void save(const String&, const String&, bool) { }
> -    virtual void append(const String&, const String&) { }

Correct me if I am wrong, but can't you keep this and eliminate the need to stub out all the other platforms on WK1?

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

Any reason not to make this bool platformCanSave()?

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:510
> +    // necessary for the block below.

Comments should be have a capital letter starting them.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:525
> +        // the user must confirm new filenames before we can save to them.

Ditto for the first letter of the comment.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:529
> +    }
> +    if (!platformURL)
> +        return;

This should ASSERT(platformURL) before returning early. Also an empty line between the previous and this block would be nice.

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

Ditto.

These all apply to the WK1 version.

[Brian Burg]

Created attachment 201249 [details]
Patch v4

[Timothy Hatcher]

Comment on attachment 201249 [details]
Patch v4

Looks good! I'll see about getting a WK2 owner to r+ it.

[Sam Weinig]

Comment on attachment 201249 [details]
Patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=201249&action=review

> Source/WebKit2/UIProcess/WebInspectorProxy.messages.in:31
> +    CanSave() -> (bool result)

Why is this sync message required?  Lets try to do this without adding any more sync messages.

[Timothy Hatcher]

(In reply to comment #23)
> (From update of attachment 201249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201249&action=review
> 
> > Source/WebKit2/UIProcess/WebInspectorProxy.messages.in:31
> > +    CanSave() -> (bool result)
> 
> Why is this sync message required?  Lets try to do this without adding any more sync messages.

Just make all of WK2 return true for canSave in the WebProcess and just leave the save and append methods empty (notImplemented()) for the other ports.

[Brian Burg]

Created attachment 201268 [details]
Patch v5

[Timothy Hatcher]

Comment on attachment 201268 [details]
Patch v5

Nice!

[Benjamin Poulain]

Comment on attachment 201268 [details]
Patch v5

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.

[Timothy Hatcher]

Comment on attachment 201268 [details]
Patch v5

View in context: https://bugs.webkit.org/attachment.cgi?id=201268&action=review

>> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:315
>>      };
> 
> What is the point of this block exactly?

It reduces some code duplication. It is called twice in this function.

>> Source/WebKit2/UIProcess/WebInspectorProxy.h:169
>> +    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.

We use String because the content comes from JavaScript (via the JSON Inspector protocol), where string is the only type we have to work with for this sort of thing.

[Brian Burg]

(In reply to comment #27)
> (From update of attachment 201268 [details])
> 
> > 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.

The use case is that the Inspector serializes collected runtime data (ie., Timeline profiles) into a JSON string and it's written to disk by InspectorFrontendHost. The API is not used for saving any other type of data currently.

I guess it could be encoded earlier and passed around as a blob behind the scenes, at least on WK2. Is there a preferred way to do that? CoreIPC::DataReference? Or going further, should we push blob construction down into the inspector frontend?

> > Source/WebKit2/UIProcess/WebInspectorProxy.h:179
> > +    void canSave(bool&);
> 
> Where is this defined?

Oops- left over from previous design. Will remove.

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

OK

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

OK

> > Source/WebKit2/WebProcess/WebPage/WebInspector.h:100
> > +    // Implemented in platform WebInspector file
> 
> Missing period.
> 
> That comment is useless really.

I agree. Will remove.

[Brian Burg]

Created attachment 201440 [details]
Patch v5b

[Benjamin Poulain]

Comment on attachment 201440 [details]
Patch v5b

LGTM.

[Benjamin Poulain]

(In reply to comment #29)
> > > 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.
> 
> The use case is that the Inspector serializes collected runtime data (ie., Timeline profiles) into a JSON string and it's written to disk by InspectorFrontendHost. The API is not used for saving any other type of data currently.
> 
> I guess it could be encoded earlier and passed around as a blob behind the scenes, at least on WK2. Is there a preferred way to do that? CoreIPC::DataReference? Or going further, should we push blob construction down into the inspector frontend?

A Vector of uchar8_t is commonly used to pass raw bytes.

It is probably okay for now as you say it is only use with JSON.
If you ever need to convert data to string to pass it through the API, please change the API instead.

[WebKit Commit Bot]

Comment on attachment 201440 [details]
Patch v5b

Clearing flags on attachment: 201440

Committed r149923: <http://trac.webkit.org/changeset/149923>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.