Bug 35550 – Allow a plugin to participate in the browser's print workflow.

• Component: Plug-ins
• Status: RESOLVED FIXED

I am working on extending the Pepper NPAPI extensions interface to allow a plugin to support printing. The current NPP_Print interface has a few drawbacks. For one, the interface is platform dependent (it expects an HDC on Windows etc). Also, it expects a full-page plugin to completely control the print workflow (i.e. show a page setup dialog print dialog etc). This may not be possible for plugins running in a sandbox (e.g. the Chromium sandbox). Instead, the idea is to extend the API to allow us to query information like number of pages to print from the plugin and have the browser put up the print dialog (just like it would in the case of printing a web page). In addition, the plugin would support APIs to print a specific page in a raster or a vector format given the user-selected print settings.

To make this possible, we need to add some printing interface methods to WebCore::Widget. In addition we need to add support in Chromium's implementation of WebKit::WebFrame to allow us to bypass Webkit and go to the plugin directly for printing needs (if the plugin supports the new custom print interfaces).

I am working on making these changes and plan to submit a patch for review soon.

Created attachment 49757 [details]
WebCore changes for plugin print.

Created attachment 49758 [details]
Chromium changes for plugin print

[Darin Fisher (:fishd, Google)]

Comment on attachment 49757 [details]
WebCore changes for plugin print.

> Index: loader/PluginDocument.cpp

> +    static Widget* PluginWidgetFromDocument(Document* doc);

nit: use camelCase naming


> +Widget* PluginTokenizer::PluginWidgetFromDocument(Document* doc)
> +{
> +    ASSERT(doc);
> +    RefPtr<Element> body = doc->body();
> +    if (body) {
> +        RefPtr<Node> embedNode = body->firstChild();
> +        RefPtr<HTMLEmbedElement> embedElement = static_cast<HTMLEmbedElement*>(embedNode.get());
> +        if (embedElement) {
> +            RenderWidget* renderer = toRenderWidget(embedElement->renderer());
> +            if (renderer)
> +                return renderer->widget();
> +        }
> +    }
> +    return 0;
> +}
> +

I would write the above a bit differently.  Like this:

Widget* PluginTokenizer::pluginWidgetFromDocument(Document* doc)
{
    ASSERT(doc);
    RefPtr<Element> body = doc->body();
    if (body) {
        RefPtr<Node> node = body->firstChild();
        if (node && node->renderer())
            return toRenderEmbeddedObject(node->renderer())->widget();
    }
    return 0;
}

We could also add a check to verify that node->renderer()->isEmbeddedObject()
returns true, but it's also fine to leave that as a debug-only check.  See
the implementation of toRenderEmbeddedObject.


> Index: platform/Widget.h
...
> +    // Virtual methods for printing. The widget can support custom printing
> +    // (which means it controls the layout, number of pages etc). This typically
> +    // happens if the widget is hosting a plugin. In the below methods, printableArea
> +    // is in points (a point is 1/72 of an inch).
> +    virtual bool supportsCustomPrint() const { return false; }
> +    virtual int getNumPages(const IntRect& printableArea, int printerDpi) const { return 0; }
> +    virtual void printPage(int pageNumber, GraphicsContext* gc, const IntRect& printableArea, int printerDpi) {}

Since you aren't calling these methods from WebCore, it turns out that
there is another option.  From the WebKit layer, if the Document is a
PluginDocument, then we can just get the pluginWidget, and cast that to
WebPluginContainerImpl.  That might be better than adding these methods
to WebCore::Widget.

[Darin Fisher (:fishd, Google)]

Comment on attachment 49758 [details]
Chromium changes for plugin print

> Index: public/WebFrame.h
...
> -    // Reformats the WebFrame for printing.  pageSize is the page size in
> -    // pixels.  Returns the number of pages that can be printed at the
> -    // given page size.
> -    virtual int printBegin(const WebSize& pageSize) = 0;
> +    // Reformats the WebFrame for printing. pageSize is the page size in
> +    // points (a point in 1/72 of an inch). printer_dpi is the user selected,

nit: printer_dpi -> printerDPI

in webkit, when you have an acronym, it should either be all uppercase or all
lowercase.  it is only all lowercase if it forms the first word of a name.

see http://webkit.org/coding/coding-style.html


> +    // DPI for the printer. Returns the number of pages that
> +    // can be printed at the given page size. The out param useBrowserOverlays
> +    // specifies whether the browser process should use its overlays (header,
> +    // footer, margins etc) or whether the renderer controls this.
> +    virtual int printBegin(const WebSize& pageSize, int printerDpi = 72,

printerDpi -> printerDPI


> Index: 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; }
> +    virtual int getNumPages(const WebRect& printableArea, int printerDpi) { return 0; }
> +    virtual bool printPage(int pageNumber, WebCanvas* canvas, const WebRect& printableArea, int printerDpi) { return false; }

in webkit style, names should be spelled out.  so, getNumPages -> getNumberOfPages.

Or, why not use a similar API as was designed for WebFrame?

  bool supportsPrinting();
  int printBegin(const WebSize& pageSize, int printerDPI);  // returns number of pages that can be printed
  void printPage(int pageNumber, WebCanvas*);
  void printEnd();

Maybe this is too incompatible with the ChromePrintContext?

Q: Do you need printableArea to be a rect?  It can't be a WebSize like it is for WebFrame?


> Index: src/WebFrameImpl.cpp
...
> +    virtual void end()
>      {
> +      PrintContext::end();

nit: indentation


> +    virtual void computePageRects(const FloatRect& printRect, float headerHeight, float footerHeight, float userScaleFactor, float& outPageHeight)
> +    {
> +      return PrintContext::computePageRects(printRect, headerHeight, footerHeight, userScaleFactor, outPageHeight);
> +    }

nit: indentation


> +
> +    virtual int pageCount() const
> +    {
> +      return PrintContext::pageCount();

nit: indentation


> +    }
> +
> +    virtual bool ShouldUseBrowserOverlays() const

nit: ShouldUse -> shouldUse


> +int WebFrameImpl::printBegin(const WebSize& pageSize, int printerDpi, bool *useBrowserOverlays)
>  {
>      ASSERT(!frame()->document()->isFrameSet());
> +    // If this is a plugin document, check if the plugin supports its own
> +    // printing. If it does, we will delegate all printing to that.
> +    if (frame()->document()->isPluginDocument()) {
> +        PluginDocument* pluginDocument = static_cast<PluginDocument*>(frame()->document());
> +        WebCore::Widget * pluginWidget = pluginDocument->pluginWidget();

There is a 'using namespace WebCore' at the top of this file, so you shouldn't
need to write "WebCore::" anywhere.

Created attachment 50133 [details]
Updated patch with code review changes

Updated patch with code review changes

I made almost all the changes you suggested. I used a rect for PrintableArea because it is possible that you want the plugin to print within a portion of the entire page.
Also I left the print interface for WebPluginContainerImpl as is because the plugin is not expected to maintain any state between printBegin and printEnd.

[Darin Fisher (:fishd, Google)]

Comment on attachment 50133 [details]
Updated patch with code review changes

> 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.

Created attachment 50362 [details]
Updated patch with code review changes

[Darin Fisher (:fishd, Google)]

Comment on attachment 50362 [details]
Updated patch with code review changes

> Index: WebCore/loader/PluginDocument.cpp
...
> +Widget* PluginTokenizer::pluginWidgetFromDocument(Document* doc)
> +{
> +    ASSERT(doc);
> +    RefPtr<Element> body = doc->body();
> +    if (body) {
> +        RefPtr<Node> node = body->firstChild();
> +        if (node && node->renderer()) {
> +            ASSERT(node->renderer()->isEmbeddedObject());
> +            return toRenderEmbeddedObject(node->renderer())->widget();
> +        }
> +    }
> +    return 0;
> +}
> +
> +
>  void PluginTokenizer::write(const SegmentedString&, bool)

nit: ^^^ only one new line between methods.


> 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 supportsPaginatedPrint() { return false; }
> +    virtual int printBegin(const WebRect& printableArea, int printerDPI) { return 0; }
> +    virtual bool printPage(int pageNumber, WebCanvas* canvas) { return false; }
> +    virtual void printEnd() { }

^^^ some documentation for these methods would be good since this is
the public API.

also, it is OK to have default implementations for methods in the
WebKit API.  i actually intended to give all methods on WebPlugin
default implementations but somehow forgot.


> Index: WebKit/chromium/src/WebFrameImpl.cpp

> +class ChromePluginPrintContext : public ChromePrintContext {
> +public:
> +    ChromePluginPrintContext(Frame* frame, int printerDPI)
> +        : ChromePrintContext(frame), m_pageCount(0), m_printerDPI(printerDPI)
> +    {
> +        // This HAS to be a frame hosting a full-mode plugin
> +        ASSERT(frame->document()->isPluginDocument());
> +    }
> +
> +    virtual void begin(float width)
> +    {
> +    }
> +
> +    virtual void end()
> +    {
> +        PluginDocument* pluginDocument = static_cast<PluginDocument*>(m_frame->document());
> +        WebPluginContainerImpl * pluginContainer = reinterpret_cast<WebPluginContainerImpl *>(pluginDocument->pluginWidget());

^^^ that reinterpret_cast can be a static_cast since a
WebPluginContainerImpl "is a" WebCore::Widget.

also, since the code to get the pluginContainer for this class
is repeated, it would be good to put it in a helper function.


> +int WebFrameImpl::printBegin(const WebSize& pageSize, int printerDPI, bool *useBrowserOverlays)
>  {
>      ASSERT(!frame()->document()->isFrameSet());
> +    // If this is a plugin document, check if the plugin supports its own
> +    // printing. If it does, we will delegate all printing to that.
> +    if (frame()->document()->isPluginDocument()) {
> +        PluginDocument* pluginDocument = static_cast<PluginDocument*>(frame()->document());
> +        WebPluginContainerImpl * pluginContainer = reinterpret_cast<WebPluginContainerImpl *>(pluginDocument->pluginWidget());

The same code appears here too.  Maybe the helper function should be
at file scope so it can be used by this class as well.


> Index: WebKit/chromium/src/WebPluginContainerImpl.h
...
> +    // Virtual methods for printing. The plugin can support custom printing
> +    // (which means it controls the layout, number of pages etc). This typically
> +    // happens if the widget is hosting a plugin. In the below methods, printableArea
> +    // is in points (a point is 1/72 of an inch) with the origin of the rect being
> +    // the top-left corner of the frame
> +    virtual bool supportsPaginatedPrint() const;
> +    virtual int printBegin(const WebCore::IntRect& printableArea, int printerDPI) const;
> +    virtual void printPage(int pageNumber, WebCore::GraphicsContext* gc);
> +    virtual void printEnd();

do these ones need to be virtual?  maybe that is leftover from
when these were overrides of WebCore::Widget methods?

Created attachment 50370 [details]
Updated patch with code review changes

Made all the changes and uploaded a new patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebFrameImpl.cpp:256:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/chromium/src/WebFrameImpl.cpp:258:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 10 files


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

[Darin Fisher (:fishd, Google)]

Comment on attachment 50370 [details]
Updated patch with code review changes

> +static WebPluginContainerImpl* pluginContainerFromFrame(Frame* frame)
> +{
> +    if (!frame)
> +        return NULL;
> +    if (!frame->document() || !frame->document()->isPluginDocument())
> +        return NULL;
> +    PluginDocument* pluginDocument = static_cast<PluginDocument*>(frame->document());
> +    return static_cast<WebPluginContainerImpl *>(pluginDocument->pluginWidget());
> +}

As the style bot says, NULL -> 0

R=me otherwise

Created attachment 50407 [details]
Fixed style issues

Fixed style issues and uploaded a new patch.

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/55814