[webkit-reviews] review denied: [Bug 35550] Allow a plugin to participate in the browser's print workflow. : [Attachment 49758] Chromium changes for plugin print

[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 49758: Chromium changes for plugin print
https://bugs.webkit.org/attachment.cgi?id=49758&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> 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.