| Summary: | Web Inspector: Implement WK2 version of WebInspectorFrontendClient::save | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brian Burg <burg> | ||||||||||||||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | abecsi, benjamin, cdumez, cmarcelo, commit-queue, ddkilzer, eflews.bot, graouts, gyuyoung.kim, gyuyoung.kim, joepeck, menard, philn, rakuco, rego+ews, timothy, webkit-ews, xan.lopez | ||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||
| Hardware: | Mac | ||||||||||||||||||||||
| OS: | OS X 10.8 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Brian Burg
2013-05-03 11:44:12 PDT
patch forthcoming. Created attachment 200997 [details]
initial patch - missing Changelog
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 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 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 Created attachment 201000 [details]
Patch
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 on attachment 201000 [details] Patch Attachment 201000 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/445018 Created attachment 201001 [details]
Patch
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 on attachment 201001 [details] Patch Attachment 201001 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/425051 Created attachment 201005 [details]
Patch
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 on attachment 201005 [details] Patch Attachment 201005 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/318276 Created attachment 201018 [details]
Patch v2
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 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()". 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! Created attachment 201081 [details]
Patch v3
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. Created attachment 201249 [details]
Patch v4
Comment on attachment 201249 [details]
Patch v4
Looks good! I'll see about getting a WK2 owner to r+ it.
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. (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. Created attachment 201268 [details]
Patch v5
Comment on attachment 201268 [details]
Patch v5
Nice!
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 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. (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. Created attachment 201440 [details]
Patch v5b
Comment on attachment 201440 [details]
Patch v5b
LGTM.
(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 on attachment 201440 [details] Patch v5b Clearing flags on attachment: 201440 Committed r149923: <http://trac.webkit.org/changeset/149923> All reviewed patches have been landed. Closing bug. |