Bug 67774 – PDF in a frameset is not displayed, always downloads

• Component: WebKit2
• Status: RESOLVED FIXED

[Kevin Pearcey]

If you browse to a site that is using a frameset, and one of those frames contains a PDF, it does not display but is always downloaded.

A suitable example is: https://bug-4091-attachments.webkit.org/attachment.cgi?id=3092

The top right frame should display a PDF, but instead it just gets downloaded.

This is a regression from previous behaviour.

[Alexey Proskuryakov]

Yes, PDF is not yet working in subframes in WebKit2. I'm surprised that there doesn't seem to be an earlier report in Bugzilla.

<rdar://problem/8750378>

[Alexey Proskuryakov]

Created attachment 109668 [details]
part 1

First part - not very usable yet, but some things work, and I'd like to proceed with smaller patches. Known limitations:

Zooming, HiDPI
Accessibility (not sure how much can be done for PDF yet).
Not all mouse events are handled yet. You can click in scroll bar, but not drag.
Keyboard events (PgUp/PgDown etc) are not handled yet.
No support for annotations, such as forms.
No support for auto-printing (scripts in PDF).
No progress when loading.
No HUD.
Not sure if I need to implement Plugin::snapshot().
Dynamic scroll bar style changes don't work.
No rubberbanding when scrolling (that doesn't work for iframes either yet).

[WebKit Review Bot]

Attachment 109668 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 109668 [details]
part 1

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

Seems the unused argument warning is not on for WebKit2?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:48
> +const uint64_t pdfDocumentRequestID = 1;

I don’t understand the significance of this constant. Is there a comment that could help make it clearer?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:75
> +    info.name = "WebKit built-in PDF";

Are these ever localized?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:77
> +    MimeClassInfo pdfMimeClassInfo;

Since this is local to the function, I think it could just be classInfo.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:79
> +    pdfMimeClassInfo.desc = "Portable Document Format";

Are these ever localized?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:94
> +PluginView* BuiltinPDFView::pluginView()
> +{
> +    return static_cast<PluginView*>(controller());
> +}
> +
> +const PluginView* BuiltinPDFView::pluginView() const
> +{
> +    return static_cast<const PluginView*>(controller());
> +}

Is there a way to arrange for these to be inlined?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:100
> +    CGPDFPageRef pdfPage = CGPDFDocumentGetPage(m_pdfDocument.get(), 1); // FIXME: Draw all pages of a document.
> +    if (!pdfPage)
> +        return;

Seems that here in this local function we could just call this “page”.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:156
> +    // FIXME: maybe need a separate ScrollableArea::didRemoveHorizontalScrollbar callback?

Capitalize the "m"?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:158
> +    PluginView* pluginView = this->pluginView();
> +    if (pluginView)

Define inside the if statement?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:177
> +    if (scrollbar) {

Early return instead of nesting the if?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:242
> +    stateSaver.restore();
> +
> +    stateSaver.save();

The idiom here is a bit awkward. Could we instead have sets of braces and two separate state savers?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:245
> +    IntRect dirtyRect = pluginView()->parent()->windowToContents(dirtyRectInWindowCoordinates);
> +    IntPoint documentOriginInWindowCoordinates = pluginView()->parent()->windowToContents(IntPoint());

Should these be LayoutRect and LayoutPoint?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:254
> +    IntRect dirtyCornerRect(scrollCornerRect());
> +    dirtyCornerRect.intersect(dirtyRect);
> +    ScrollbarTheme::nativeTheme()->paintScrollCorner(0, graphicsContext, dirtyCornerRect);

If intersect was a free function we could write this in a more natural way.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:266
> +    return;

Please omit this.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:319
> +void BuiltinPDFView::streamDidReceiveResponse(uint64_t streamID, const KURL& responseURL, uint32_t streamLength, uint32_t lastModifiedTime, const WTF::String& mimeType, const WTF::String& headers)

Can we leave the names of the arguments out to avoid unused variable warnings?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:321
> +    ASSERT(streamID == pdfDocumentRequestID);

This should be ASSERT_UNUSED.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:326
> +    ASSERT(streamID == pdfDocumentRequestID);

This should be ASSERT_UNUSED.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:408
> +    PlatformWheelEvent platformEvent = platform(event);
> +    ScrollableArea::handleWheelEvent(platformEvent);
> +    return platformEvent.isAccepted();

Seems like this collides with Anders’s recent patch where he got rid of isAccepted.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:438
> +#if PLATFORM(MAC)
> +void BuiltinPDFView::windowFocusChanged(bool hasFocus)

Should leave a blank line here to match the #endif below.

Should omit the argument names to avoid the warning.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:442
> +void BuiltinPDFView::windowAndViewFramesChanged(const WebCore::IntRect& windowFrameInScreenCoordinates, const WebCore::IntRect& viewFrameInWindowCoordinates)

Should omit the argument names to avoid the warning.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:446
> +void BuiltinPDFView::windowVisibilityChanged(bool isVisible)

Should omit the argument names to avoid the warning.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:455
> +void BuiltinPDFView::sendComplexTextInput(const String& textInput)

Should omit the argument names to avoid the warning.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.h:42
> +class BuiltinPDFView : public Plugin, private WebCore::ScrollableArea {

If the word is “built-in” then I think it should be BuiltIn rather than Builtin.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.h:64
> +    // Plugin methods

Plug-in

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.h:72
> +#if PLATFORM(MAC)
> +    virtual PlatformLayer* pluginLayer();
> +#endif

I think it’s nicer to separate out #if things rather than having them in the same paragraph with the rest.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.h:130
> +    RetainPtr<CGPDFDocumentRef> m_pdfDocument;

The m_pdf prefix here is a little ugly. Is there some other clever way to name this that avoids the acronym. Maybe m_contentDocument?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.h:131
> +    WebCore::IntSize m_pdfDocumentSize; // All pages, including gaps.

Ditto, about m_pdf.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:336
> +#if PLATFORM(MAC)

Ideally these are all #if ENABLE(BUILT_IN_PDF_VIEW) instead of #if PLATFORM(MAC). Or something like that. Direct PLATFORM #ifs are not great.

[Anders Carlsson]

(In reply to comment #4)
> (From update of attachment 109668 [details])
> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltinPDFView.cpp:254
> > +    IntRect dirtyCornerRect(scrollCornerRect());
> > +    dirtyCornerRect.intersect(dirtyRect);
> > +    ScrollbarTheme::nativeTheme()->paintScrollCorner(0, graphicsContext, dirtyCornerRect);
> 
> If intersect was a free function we could write this in a more natural way.

inline IntRect intersection(const IntRect& a, const IntRect& b);

[Alexey Proskuryakov]

> Are these ever localized?

Most plug-ins don't have localized descriptions, with the exception of Java and QuickTime. I'm planning to make these localizable in a later patch (forgot to mention that in the list of unfinished things).

> Is there a way to arrange for these to be inlined?

I wanted to avoid including PluginView.h in BuiltinPDFView.h. Not sure which consideration should be stronger in this case.

> Seems that here in this local function we could just call this “page”.

Not changing yet, since this code is largely a placeholder (we need to show all pages, and nicely position them).

> Define inside the if statement?
> Early return instead of nesting the if?

Changed. This is copy/pasted code, and I initially wanted to keep it identical to original to make refactoring a little easier, but that's extremely insignificant compared to other code differences with FrameView and RenderLayer.

> The idiom here is a bit awkward. Could we instead have sets of braces and two separate state savers?

These will be separate functions soon, when full PDF is rendered.

> I think it’s nicer to separate out #if things rather than having them in the same paragraph with the rest.

Just staying in sync with Plugin.h here.

> The m_pdf prefix here is a little ugly. Is there some other clever way to name this that avoids the acronym. Maybe m_contentDocument?

I've been thinking about this for some time (having the same alternative in mind), and I'm not sure if it helps. There are so many "content" things in view and widget classes that I don't want to add more. Also, PDF document is not what we usually think of as "document" in WebKit.

> Ideally these are all #if ENABLE(BUILT_IN_PDF_VIEW) instead of #if PLATFORM(MAC). Or something like that. Direct PLATFORM #ifs are not great.

I chose to match existing WebKit2 checks for main frame PDF for now. Also, historically there was no interest to implementing built-in PDF support from other ports (e.g. PDFDocumentImage is still under USE(CG)).

Addressed the rest of comments. Thanks for review!

[Alexey Proskuryakov]

Comment on attachment 109668 [details]
part 1

Committed <http://trac.webkit.org/changeset/96653>. Clearing review flag and not closing the bug, since it's not done yet.

[Alexey Proskuryakov]

Remaining issues:

Zooming, HiDPI
No support for annotations, such as forms.
No support for auto-printing (scripts in PDF).
No progress when loading.
No HUD.
Context menu lacks PDF specific items.
Custom CSS scrollbars are ignored.
PostScript documents are not automatically converted to PDF for display.
No interaction with PDF content (text selection, search, links or forms).
No rubberbanding when scrolling (that doesn't work for iframes either yet).
Accessibility (not sure if there is any meaningful work to do here).

[Alexey Proskuryakov]

> Custom CSS scrollbars are ignored.

Simon tells me that this is correct. An embedding document should not affect embedded one.

I think that current functionality lets us close this bug. Please file separate bugs for details that still don't work and are important for you.

[Mat]

Comment on attachment 109668 [details]
part 1

How do I get use the patch? I just want my device to work normally and not download pdf automatically..

[Alexey Proskuryakov]

This fix has shipped well over a decade ago.

If you are still seeing PDFs download, chances are that this a website decision, which browsers ought to respect.