[Webkit-unassigned] [Bug 38711] DragData::asURL() shouldn't do file validity checks

[bugzilla-daemon at webkit.org]

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





--- Comment #20 from Daniel Cheng <dcheng at chromium.org>  2010-05-17 16:00:49 PST ---
(In reply to comment #18)
> (From update of attachment 56174 [details])
> > +        // FIXME: maybe it makes more sense to allow multiple files and only use the first one?
> 
> We capitalize sentences and fragments in comments, so this should be capitalized.
> 
Done.

> > +            // FIXME: this check should move to the loader.
> 
> And this.
> 
Done.

> > -            BOOL isDirectory;
> > +            BOOL isDirectory = false;
> 
> This should be "NO" rather than "false". But also, the change is not needed. That function will always set the BOOL we pass and the code was already fine in this respect.
> 
Reverted this line.

> > -            if([[NSFileManager defaultManager] fileExistsAtPath:file isDirectory:&isDirectory] && !isDirectory){
> > -                return [[NSURL fileURLWithPath:file] _webkit_canonicalize];
> > -            }
> > +            if ([[NSFileManager defaultManager] fileExistsAtPath:file isDirectory:&isDirectory] && isDirectory)
> > +                return nil;
> > +            // Purposely allow invalid paths to fall through so the loader can handle it.
> 
> So the loader can handle *them*, not *it*.
> 
> I think this comment is really confusing. It seems fine to just remove all the checking entirely without comment. What I think is needed is a comment about the strange code that tries to find out if a file is a directory. What you say in the change log is closer to what's needed here. The comment needs to explain that we are filtering out directories here because we always did that in the past and for now we end up opening directories in the Finder without the check. The check needs to go away as soon as possible.
> 
> I'm quite confused about the "loader" you are referring to. The term "loader" means something specific in WebKit, and somehow I don't think you mean the same thing I do by that word. Maybe you could be more specific about what you mean by "loader".

I updated the ChangeLog to be more specific about what I was referring to.

Thanks for the quick review.

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