Bug 97547 – Code inside FrameLoaderClient::canShowMIMEType() implementations can be shared among different WK ports

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mikhail Pozdnyakov]

Currently the code inside FrameLoaderClient::canShowMIMEType() implementations is pretty much similar among
different WK ports:
EFL, Qt, GTK, WinCe have following snippet inside FrameLoaderClient canShowMIMEType() function:
MIMETypeRegistry::isSupportedImageMIMEType(type)
            || MIMETypeRegistry::isSupportedNonImageMIMEType(type)
            || MIMETypeRegistry::isSupportedMediaMIMEType(type)

WK2 has similar logic inside WebKit::WebPageProxy::canShowMIMEType()
    if (MIMETypeRegistry::isSupportedNonImageMIMEType(mimeType))
        return true;

    if (MIMETypeRegistry::isSupportedImageMIMEType(mimeType))
        return true;

    if (mimeType.startsWith("text/", false))
        return !MIMETypeRegistry::isUnsupportedTextMIMEType(mimeType);

Think it would be wise to have a new function inside MIMETypeRegistry, that would encapsulate this logic.

[Mikhail Pozdnyakov]

Created attachment 165593 [details]
patch

[Build Bot]

Comment on attachment 165593 [details]
patch

Attachment 165593 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14030057

[Mikhail Pozdnyakov]

Created attachment 165594 [details]
patch v2

Rebased. Build on MAC should be fixed.

[Kenneth Rohde Christiansen]

Comment on attachment 165594 [details]
patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=165594&action=review

> Source/WebCore/platform/MIMETypeRegistry.cpp:582
>  
> +bool MIMETypeRegistry::isVisuallyRepresentableMIMEType(const String& mimeType)

Isnt that like a really weird name. What if it contains only javascript?

[Mikhail Pozdnyakov]

(In reply to comment #4)
> (From update of attachment 165594 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165594&action=review
> 
> > Source/WebCore/platform/MIMETypeRegistry.cpp:582
> >  
> > +bool MIMETypeRegistry::isVisuallyRepresentableMIMEType(const String& mimeType)
> 
> Isnt that like a really weird name. What if it contains only javascript?

right. I need to find a better one..

[Mikhail Pozdnyakov]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 165594 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=165594&action=review
> > 
> > > Source/WebCore/platform/MIMETypeRegistry.cpp:582
> > >  
> > > +bool MIMETypeRegistry::isVisuallyRepresentableMIMEType(const String& mimeType)
> > 
> > Isnt that like a really weird name. What if it contains only javascript?
> 
> right. I need to find a better one..

How about 'isMIMETypeHandledByWebPage' ?

[Benjamin Poulain]

Comment on attachment 165594 [details]
patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=165594&action=review

> Source/WebCore/WebCore.exp.in:422
>  __ZN7WebCore16MIMETypeRegistry27getUnsupportedTextMIMETypesEv
>  __ZN7WebCore16MIMETypeRegistry27isSupportedNonImageMIMETypeERKN3WTF6StringE
>  __ZN7WebCore16MIMETypeRegistry29getSupportedNonImageMIMETypesEv
> +__ZN7WebCore16MIMETypeRegistry31isVisuallyRepresentableMIMETypeERKN3WTF6StringE

Are all the other exports still needed?

[Mikhail Pozdnyakov]

(In reply to comment #7)
> (From update of attachment 165594 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165594&action=review
> 
> > Source/WebCore/WebCore.exp.in:422
> >  __ZN7WebCore16MIMETypeRegistry27getUnsupportedTextMIMETypesEv
> >  __ZN7WebCore16MIMETypeRegistry27isSupportedNonImageMIMETypeERKN3WTF6StringE
> >  __ZN7WebCore16MIMETypeRegistry29getSupportedNonImageMIMETypesEv
> > +__ZN7WebCore16MIMETypeRegistry31isVisuallyRepresentableMIMETypeERKN3WTF6StringE
> 
> Are all the other exports still needed?

Looks MIMETypeRegistry::getUnsupportedTextMIMETypes does not need to be exported anymore.

[Mikhail Pozdnyakov]

Created attachment 165953 [details]
patch v3 (do not put flags)

Having an IRC discussion with Kenneth came to conclusion that the already used "canShowMIMEType" looks like the most appropriate name for the new function. 
Do not require any flags as need to get mangled name for the new function from MAC bot first :)

[Simon Hausmann]

Comment on attachment 165953 [details]
patch v3 (do not put flags)

Yay for removal of duplicated code!

[Build Bot]

Comment on attachment 165953 [details]
patch v3 (do not put flags)

Attachment 165953 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14037732

[Mikhail Pozdnyakov]

Created attachment 165955 [details]
patch v4

Now should build on MAC. Thanks, bot.

[Mikhail Pozdnyakov]

Created attachment 165980 [details]
patch v5

Rebased.

[Mikhail Pozdnyakov]

Could please anyone review?

[Adam Barth]

Comment on attachment 165980 [details]
patch v5

Looks great.

[WebKit Review Bot]

Comment on attachment 165980 [details]
patch v5

Clearing flags on attachment: 165980

Committed r129924: <http://trac.webkit.org/changeset/129924>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.