[webkit-reviews] review granted: [Bug 118720] Remove PDFViewController and WKView "custom representations" : [Attachment 206739] patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 118720: Remove PDFViewController and WKView "custom representations"
https://bugs.webkit.org/show_bug.cgi?id=118720

Attachment 206739: patch
https://bugs.webkit.org/attachment.cgi?id=206739&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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?