Bug 35361 - File.type support
Summary: File.type support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jian Li
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-24 14:38 PST by Jian Li
Modified: 2010-02-25 14:31 PST (History)
1 user (show)

See Also:


Attachments
Proposed Patch (6.21 KB, patch)
2010-02-24 14:42 PST, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (6.32 KB, patch)
2010-02-24 16:34 PST, Jian Li
dimich: review-
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (6.42 KB, patch)
2010-02-25 12:17 PST, Jian Li
dimich: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2010-02-24 14:38:07 PST
Need to implement File.type as defined in File API spec.
Comment 1 Jian Li 2010-02-24 14:42:11 PST
Created attachment 49443 [details]
Proposed Patch
Comment 2 Dmitry Titov 2010-02-24 14:58:43 PST
Is the non-existing file detected as "application/octet-stream" on all platforms? The test update seems to assume this.
Comment 3 Jian Li 2010-02-24 16:34:07 PST
Created attachment 49450 [details]
Proposed Patch

Changed to call the function that returns empty string on failure, in order to stick to the spec.

For non-existent file, we do not perform the check since the check will involve synchronous file operation and the file might become valid right before accessing it.
Comment 4 Dmitry Titov 2010-02-25 11:44:08 PST
Comment on attachment 49450 [details]
Proposed Patch

> diff --git a/WebCore/html/File.cpp b/WebCore/html/File.cpp
>  File::File(const String& path)
>      : Blob(path)
>      , m_name(pathGetFileName(path))
> +    // We don't use MIMETypeRegistry::getMIMETypeForPath() because it returns "application/octet-stream" upon failure
> +    , m_type(MIMETypeRegistry::getMIMETypeForExtension(path.substring(path.reverseFind('.') + 1)))

It looks too complex code to have it in initializer. Also, for the path "gif" it will return a valid mime type while it should return "".
Could it be moved into ctor body with more checks?

Also, getMIMETypeForExtension on some platforms also returns non-empty string when it can't match the extension - see for example MIMETypeRegistryQt.cpp
It might be a good idea to ask port developers if they want to fix those.

r- because of initializer.
Comment 5 Jian Li 2010-02-25 12:17:59 PST
Created attachment 49514 [details]
Proposed Patch
Comment 6 Jian Li 2010-02-25 12:20:40 PST
(In reply to comment #4)
> (From update of attachment 49450 [details])
> > diff --git a/WebCore/html/File.cpp b/WebCore/html/File.cpp
> >  File::File(const String& path)
> >      : Blob(path)
> >      , m_name(pathGetFileName(path))
> > +    // We don't use MIMETypeRegistry::getMIMETypeForPath() because it returns "application/octet-stream" upon failure
> > +    , m_type(MIMETypeRegistry::getMIMETypeForExtension(path.substring(path.reverseFind('.') + 1)))
> 
> It looks too complex code to have it in initializer. Also, for the path "gif"
> it will return a valid mime type while it should return "".
> Could it be moved into ctor body with more checks?

Indeed the above code handles the file with name "gif" since we're finding in the full path.

Moved to the body and changed to use the name to make the logic clear.
> 
> Also, getMIMETypeForExtension on some platforms also returns non-empty string
> when it can't match the extension - see for example MIMETypeRegistryQt.cpp
> It might be a good idea to ask port developers if they want to fix those.
> 
I will ping qt developer on this.

> r- because of initializer.
Comment 7 Dmitry Titov 2010-02-25 13:01:24 PST
Comment on attachment 49514 [details]
Proposed Patch

r=me

> diff --git a/WebCore/html/File.cpp b/WebCore/html/File.cpp
> +    // We don't use MIMETypeRegistry::getMIMETypeForPath() because it returns "application/octet-stream" upon failure

Nit - please add a "." at the end of this sentence.
Comment 8 Jian Li 2010-02-25 14:31:47 PST
Committed as http://trac.webkit.org/changeset/55257.