Summary: | [chromium] DragData::asURL should return file URL. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jian Li <jianli> | ||||||||||
Component: | WebCore Misc. | Assignee: | Jian Li <jianli> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | darin, levin | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Jian Li
2009-09-09 14:55:18 PDT
Created attachment 39305 [details]
Proposed Patch
Comment on attachment 39305 [details]
Proposed Patch
How do we know that the filename is an absolute path?
(In reply to comment #2) > (From update of attachment 39305 [details]) > How do we know that the filename is an absolute path? To make the filename an absolute path, I would like to add getAbsolutePath to FileSystem API and wrap it under PLATFORM(CHROMIUM) since no other platform is needing it now. How do you think? To address the FIXME in DragData::asURL, I plan to add add isDirectory to FileSystem and wrap it under PLATFORM(CHROMIUM). Created attachment 39494 [details]
Proposed Patch
Added the support to convert to absolute path. Also fixed the FIXME for directory check.
Created attachment 39899 [details]
Proposed Patch: add conversion from file path to file url
Created attachment 40114 [details]
Proposed Patch
Changed the function name and return type per other feedback.
Comment on attachment 40114 [details]
Proposed Patch
Why are we adding single-platform functions used only in a single platform’s code to FileSystem.h, a cross-platform header? That is inappropriate.
We should add functions to the cross-platform headers when they are used in multiple platforms. If the code is Chromium-only, then the calls should be directly to the Chromium bridge from the Chromium-specific code.
If the functions are intended for use to make future code cross-platform, they the declarations should *not* be in #if PLATFORM(CHROMIUM).
Also, it's incorrect to add #include "KURL.h" just so you can declare a function returning a KURL. All you need for that is a forward declaration:
class KURL;
I'm sorry I didn't notice this earlier. The patch was marked [Chromium] so I did not realize it was making changes to this cross-platform header.
Committed as http://trac.webkit.org/changeset/48821. I will fix the problems you point out in next patch. Thanks. (In reply to comment #7) > (From update of attachment 40114 [details]) > Why are we adding single-platform functions used only in a single platform’s > code to FileSystem.h, a cross-platform header? That is inappropriate. > > We should add functions to the cross-platform headers when they are used in > multiple platforms. If the code is Chromium-only, then the calls should be > directly to the Chromium bridge from the Chromium-specific code. > > If the functions are intended for use to make future code cross-platform, they > the declarations should *not* be in #if PLATFORM(CHROMIUM). > > Also, it's incorrect to add #include "KURL.h" just so you can declare a > function returning a KURL. All you need for that is a forward declaration: > > class KURL; > > I'm sorry I didn't notice this earlier. The patch was marked [Chromium] so I > did not realize it was making changes to this cross-platform header. |