Bug 115564 - Web Inspector: Implement WK2 version of WebInspectorFrontendClient::save
Summary: Web Inspector: Implement WK2 version of WebInspectorFrontendClient::save
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.8
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-03 11:44 PDT by Brian Burg
Modified: 2013-05-10 22:02 PDT (History)
18 users (show)

See Also:


Attachments
initial patch - missing Changelog (20.91 KB, patch)
2013-05-07 16:56 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (20.80 KB, patch)
2013-05-07 17:17 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (28.33 KB, patch)
2013-05-07 17:29 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (28.40 KB, patch)
2013-05-07 17:45 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch v2 (29.53 KB, patch)
2013-05-07 19:41 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch v3 (34.39 KB, patch)
2013-05-08 10:02 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch v4 (23.40 KB, patch)
2013-05-09 10:32 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch v5 (24.21 KB, patch)
2013-05-09 13:07 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch v5b (24.27 KB, patch)
2013-05-10 16:38 PDT, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2013-05-03 11:44:12 PDT
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.
Comment 1 Brian Burg 2013-05-07 16:07:10 PDT
patch forthcoming.
Comment 2 Brian Burg 2013-05-07 16:56:44 PDT
Created attachment 200997 [details]
initial patch - missing Changelog
Comment 3 WebKit Commit Bot 2013-05-07 16:58:06 PDT
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.
Comment 4 Early Warning System Bot 2013-05-07 17:03:45 PDT
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
Comment 5 EFL EWS Bot 2013-05-07 17:06:09 PDT
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
Comment 6 Brian Burg 2013-05-07 17:17:34 PDT
Created attachment 201000 [details]
Patch
Comment 7 EFL EWS Bot 2013-05-07 17:23:22 PDT
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
Comment 8 Early Warning System Bot 2013-05-07 17:23:38 PDT
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
Comment 9 Brian Burg 2013-05-07 17:29:54 PDT
Created attachment 201001 [details]
Patch
Comment 10 EFL EWS Bot 2013-05-07 17:35:02 PDT
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
Comment 11 Early Warning System Bot 2013-05-07 17:37:01 PDT
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
Comment 12 Brian Burg 2013-05-07 17:45:43 PDT
Created attachment 201005 [details]
Patch
Comment 13 Timothy Hatcher 2013-05-07 18:04:53 PDT
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.
Comment 14 Build Bot 2013-05-07 18:13:14 PDT
Comment on attachment 201005 [details]
Patch

Attachment 201005 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/318276
Comment 15 Brian Burg 2013-05-07 19:41:11 PDT
Created attachment 201018 [details]
Patch v2
Comment 16 Timothy Hatcher 2013-05-07 20:28:27 PDT
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.
Comment 17 Timothy Hatcher 2013-05-07 20:29:44 PDT
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()".
Comment 18 Brian Burg 2013-05-07 20:58:17 PDT
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!
Comment 19 Brian Burg 2013-05-08 10:02:28 PDT
Created attachment 201081 [details]
Patch v3
Comment 20 Timothy Hatcher 2013-05-08 10:48:10 PDT
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.
Comment 21 Brian Burg 2013-05-09 10:32:06 PDT
Created attachment 201249 [details]
Patch v4
Comment 22 Timothy Hatcher 2013-05-09 10:35:53 PDT
Comment on attachment 201249 [details]
Patch v4

Looks good! I'll see about getting a WK2 owner to r+ it.
Comment 23 Sam Weinig 2013-05-09 11:53:19 PDT
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.
Comment 24 Timothy Hatcher 2013-05-09 11:56:19 PDT
(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.
Comment 25 Brian Burg 2013-05-09 13:07:01 PDT
Created attachment 201268 [details]
Patch v5
Comment 26 Timothy Hatcher 2013-05-09 13:17:12 PDT
Comment on attachment 201268 [details]
Patch v5

Nice!
Comment 27 Benjamin Poulain 2013-05-10 12:34:07 PDT
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.
Comment 28 Timothy Hatcher 2013-05-10 13:04:40 PDT
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.
Comment 29 Brian Burg 2013-05-10 13:25:44 PDT
(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.
Comment 30 Brian Burg 2013-05-10 16:38:12 PDT
Created attachment 201440 [details]
Patch v5b
Comment 31 Benjamin Poulain 2013-05-10 19:22:29 PDT
Comment on attachment 201440 [details]
Patch v5b

LGTM.
Comment 32 Benjamin Poulain 2013-05-10 19:25:53 PDT
(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.
Comment 33 WebKit Commit Bot 2013-05-10 22:02:28 PDT
Comment on attachment 201440 [details]
Patch v5b

Clearing flags on attachment: 201440

Committed r149923: <http://trac.webkit.org/changeset/149923>
Comment 34 WebKit Commit Bot 2013-05-10 22:02:35 PDT
All reviewed patches have been landed.  Closing bug.