Bug 35361

Summary: File.type support
Product: WebKit Reporter: Jian Li <jianli>
Component: WebCore JavaScriptAssignee: Jian Li <jianli>
Status: RESOLVED FIXED    
Severity: Normal CC: dimich
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed Patch
jianli: commit-queue-
Proposed Patch
dimich: review-, jianli: commit-queue-
Proposed Patch dimich: review+, jianli: commit-queue-

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.