Bug 29109 – [chromium] DragData::asURL should return file URL.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jian Li]

In Chromium implementation, DragData::asURL should return file URL, instead of file system path. This causes drag-to-navigate.html to crash in Mac and Linux.

[Jian Li]

Created attachment 39305 [details]
Proposed Patch

[Eric Seidel (no email)]

Comment on attachment 39305 [details]
Proposed Patch

How do we know that the filename is an absolute path?

[Jian Li]

(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).

[Jian Li]

Created attachment 39494 [details]
Proposed Patch

Added the support to convert to absolute path. Also fixed the FIXME for directory check.

[Jian Li]

Created attachment 39899 [details]
Proposed Patch: add conversion from file path to file url

[Jian Li]

Created attachment 40114 [details]
Proposed Patch

Changed the function name and return type per other feedback.

[Darin Adler]

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.

[Jian Li]

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.