[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


Timothy Hatcher <timothy at apple.com> changed:

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




--- Comment #16 from Timothy Hatcher <timothy at apple.com>  2013-05-07 20:28:27 PST ---
(From update of attachment 201018)
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.

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