[webkit-reviews] review denied: [Bug 35550] Allow a plugin to participate in the browser's print workflow. : [Attachment 50133] Updated patch with code review changes

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied  review:
Bug 35550: Allow a plugin to participate in the browser's print workflow.
https://bugs.webkit.org/show_bug.cgi?id=35550

Attachment 50133: Updated patch with code review changes
https://bugs.webkit.org/attachment.cgi?id=50133&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebCore/loader/PluginDocument.cpp
...
> -    
> +
> +Widget* PluginDocument::pluginWidget()
> +{
> +    return PluginTokenizer::pluginWidgetFromDocument(this);
>  }
> +}

nit: please preserve the new line before the last "}"


> Index: WebCore/loader/PluginDocument.h
...

nit: please preserve the new line after the "{"

>  namespace WebCore {
> -    
> +class Widget;
>  class PluginDocument : public HTMLDocument {


> Index: WebKit/chromium/public/WebPlugin.h
...
> +    // Printing interface. In the below methods, printableArea
> +    // is in points (a point is 1/72 of an inch).
> +    // These methods have default implementations so as to not break
existing
> +    // Chromium builds. They will be made pure virtual once the
corresponding
> +    // Chromium patch lands.
> +    virtual bool supportsCustomPrint() { return false; }

Instead of supportsCustomPrint, how about a name that is consistent with
printPage?  Like supportsPaginatedPrinting or canPrintPage?  That
way it is clear that we mean that the printPage method is supported.


> +    virtual int getNumberOfPages(const WebRect& printableArea, int
printerDPI) { return 0; }
> +    virtual bool printPage(int pageNumber, WebCanvas* canvas, const WebRect&
printableArea, int printerDPI) { return false; }

I'm still a bit surprised that we wouldn't want there to be some state
held by the plugin here.  This API suggests that any complex page layout
required to compute the number of pages in a document would need to be
done twice.  Or, the implementation would need to do some caching.

This is why the begin/print/end pattern could be better.


R- because of nits, but I would like to discuss the API a bit more.