Bug 118720 – Remove PDFViewController and WKView "custom representations"

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

They're no longer needed in light of PDFPlugin. None of the other ports use them at all.

[Tim Horton]

Created attachment 206739 [details]
patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alexey Proskuryakov]

Comment on attachment 206739 [details]
patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1636
>  bool WebPageProxy::supportsTextEncoding() const
>  {
> -    return !m_mainFrameHasCustomRepresentation && m_mainFrame && !m_mainFrame->isDisplayingStandaloneImageDocument();
> +    return m_mainFrame && !m_mainFrame->isDisplayingStandaloneImageDocument();

This looks wrong, image documents are not the only ones that don't support text encoding. Before PDFPlugin, this code did the same for PDFs, and really, shouldn't it be all non-text documents?

Could you file a bug and add a FIXME here?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:-1678
>  bool WebPageProxy::supportsTextZoom() const
>  {
> -    if (m_mainFrameHasCustomRepresentation)
> -        return false;

Ditto.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:-1693
> -    if (m_mainFrameHasCustomRepresentation)
> -        return;

Ditto?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:-1715
> -    if (m_mainFrameHasCustomRepresentation) {
> -        m_pageClient->setCustomRepresentationZoomFactor(zoomFactor);
> -        return;
> -    }

Hmm.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:-1732
> -    if (m_mainFrameHasCustomRepresentation) {
> -        m_pageClient->setCustomRepresentationZoomFactor(pageZoomFactor);
> -        return;
> -    }

Hmm!

> Source/WebKit2/UIProcess/WebPageProxy.cpp:-2314
> -            m_mainFrameIsPinnedToLeftSide = true;
> -            m_mainFrameIsPinnedToRightSide = true;
> -            m_mainFrameIsPinnedToTopSide = true;
> -            m_mainFrameIsPinnedToBottomSide = true;

Can all associated code be deleted now, too?

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.h:47
> +    virtual bool hasHTMLView() const OVERRIDE { return true; }
>      virtual bool hasWebView() const OVERRIDE;

This is confusing. Not sure how to unconfuse.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:120
>  bool WebPage::shouldUsePDFPlugin() const

When will be the right time to delete this?

[Tim Horton]

Comment on attachment 206739 [details]
patch

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

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:-2314
>> -            m_mainFrameIsPinnedToBottomSide = true;
> 
> Can all associated code be deleted now, too?

No, it's used for things that are still important in normal web views.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.h:47
>>      virtual bool hasWebView() const OVERRIDE;
> 
> This is confusing. Not sure how to unconfuse.

Could remove the override… the base class implementation just returns true. Wasn't sure that was sufficiently explicit.

>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:120
>>  bool WebPage::shouldUsePDFPlugin() const
> 
> When will be the right time to delete this?

Soon! There are more patches in this vein coming.

[Tim Horton]

http://trac.webkit.org/changeset/152841