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

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> 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 201018: Patch v2
https://bugs.webkit.org/attachment.cgi?id=201018&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
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.