[webkit-reviews] review granted: [Bug 67774] PDF in a frameset is not displayed, always downloads : [Attachment 109668] part 1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 67774: PDF in a frameset is not displayed, always downloads
https://bugs.webkit.org/show_bug.cgi?id=67774

Attachment 109668: part 1
https://bugs.webkit.org/attachment.cgi?id=109668&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.