Bug 85736 - Cleanup: FileSystem API's Entry.toURL() impl should return KURL instead of String
Summary: Cleanup: FileSystem API's Entry.toURL() impl should return KURL instead of St...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kinuko Yasuda
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-06 09:25 PDT by Kinuko Yasuda
Modified: 2012-05-07 23:57 PDT (History)
1 user (show)

See Also:


Attachments
Patch (6.63 KB, patch)
2012-05-06 09:37 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (10.50 KB, patch)
2012-05-06 11:17 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2012-05-06 09:25:30 PDT
Entry.toURL() implementation should return KURL instead of String.  This would not have any visible impact in the app-facing javascript layer but must make the code clearer and more readable.
Comment 1 Kinuko Yasuda 2012-05-06 09:37:58 PDT
Created attachment 140423 [details]
Patch
Comment 2 Kinuko Yasuda 2012-05-06 10:08:10 PDT
This is one of the patches separated from the bigger one for http://webkit.org/b/84135 (for cross-filesystem support).
Comment 3 Kinuko Yasuda 2012-05-06 11:17:03 PDT
Created attachment 140428 [details]
Patch
Comment 4 David Levin 2012-05-06 11:30:53 PDT
Comment on attachment 140428 [details]
Patch

I don't understand why I don't see any changes to whoever calls toURL (maybe there was an implicit conversion?). Anyway, as long as this compiles it seems fine.
Comment 5 Kinuko Yasuda 2012-05-06 21:16:21 PDT
(In reply to comment #4)
> (From update of attachment 140428 [details])
> I don't understand why I don't see any changes to whoever calls toURL (maybe there was an implicit conversion?). Anyway, as long as this compiles it seems fine.

Yes it is converted back to String by implicit conversion in the binding layer. I found that some other IDL implementation is doing this and thought it'd be nice to make it clearer what type of object we are dealing with in C++ world. I could add an explicit conversion code before at the boundary between the binding code if it makes things even clearer.
Comment 6 Kinuko Yasuda 2012-05-07 02:02:32 PDT
Comment on attachment 140428 [details]
Patch

Committed r116273 <http://trac.webkit.org/changeset/116273>.
Comment 7 Darin Adler 2012-05-07 10:30:45 PDT
Comment on attachment 140428 [details]
Patch

This seems like a bad change. Why parse the URL and then convert it back to a string each time? We should not use KURL for anything just because it happens to be a URL. This is adding extra work for no benefit.
Comment 8 Kinuko Yasuda 2012-05-07 12:02:40 PDT
(In reply to comment #7)
> (From update of attachment 140428 [details])
> This seems like a bad change. Why parse the URL and then convert it back to a string each time? We should not use KURL for anything just because it happens to be a URL. This is adding extra work for no benefit.

Ok, on the second thought I wondered the same.  I'm going to make another change to revert it.  Thanks,
Comment 9 Kinuko Yasuda 2012-05-07 23:57:21 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 140428 [details] [details])
> > This seems like a bad change. Why parse the URL and then convert it back to a string each time? We should not use KURL for anything just because it happens to be a URL. This is adding extra work for no benefit.
> 
> Ok, on the second thought I wondered the same.  I'm going to make another change to revert it.  Thanks,

The patch is: https://bugs.webkit.org/show_bug.cgi?id=85858