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

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

Summary: [chromium] DragData::asURL should return file URL.

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	WebCore Misc. (show other bugs)
Version:	528+ (Nightly build)
Hardware:	All All

Importance:	P2 Normal
Assignee:	Jian Li

URL:
Keywords:

Depends on:
Blocks:

Reported:	2009-09-09 14:55 PDT by Jian Li
Modified:	2009-09-28 12:46 PDT (History)
CC List:	2 users (show)

See Also:

Attachments
Proposed Patch (1.18 KB, patch) 2009-09-09 14:59 PDT, Jian Li	jianli: commit-queue-	Details \| Formatted Diff \| Diff
Proposed Patch (3.23 KB, patch) 2009-09-11 17:12 PDT, Jian Li	jianli: commit-queue-	Details \| Formatted Diff \| Diff
Proposed Patch: add conversion from file path to file url (3.48 KB, patch) 2009-09-21 17:59 PDT, Jian Li	levin: review+ jianli: commit-queue-	Details \| Formatted Diff \| Diff
Proposed Patch (3.64 KB, patch) 2009-09-25 09:49 PDT, Jian Li	levin: review+ jianli: commit-queue-	Details \| Formatted Diff \| Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Jian Li 2009-09-09 14:55:18 PDT

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.

Comment 1 Jian Li 2009-09-09 14:59:23 PDT

Created attachment 39305 [details]
Proposed Patch

Comment 2 Eric Seidel (no email) 2009-09-09 15:04:41 PDT

Comment on attachment 39305 [details]
Proposed Patch

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

Comment 3 Jian Li 2009-09-10 10:29:03 PDT

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

Comment 4 Jian Li 2009-09-11 17:12:52 PDT

Created attachment 39494 [details]
Proposed Patch

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

Comment 5 Jian Li 2009-09-21 17:59:20 PDT

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

Comment 6 Jian Li 2009-09-25 09:49:03 PDT

Created attachment 40114 [details]
Proposed Patch

Changed the function name and return type per other feedback.

Comment 7 Darin Adler 2009-09-28 12:00:56 PDT

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.

Comment 8 Jian Li 2009-09-28 12:46:15 PDT

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.