[Webkit-unassigned] [Bug 115564] Web Inspector: Implement WK2 version of WebInspectorFrontendClient::save

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue May 7 20:59:54 PDT 2013


https://bugs.webkit.org/show_bug.cgi?id=115564





--- Comment #18 from Brian Burg <burg at cs.washington.edu>  2013-05-07 20:58:17 PST ---
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!

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


More information about the webkit-unassigned mailing list