Summary: | Allow a plugin to participate in the browser's print workflow. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | sanjeevr | ||||||||||||||
Component: | Plug-ins | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | andersca, ap, fishd, jam, kdecker, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
sanjeevr
2010-03-01 15:24:36 PST
Created attachment 49757 [details]
WebCore changes for plugin print.
Created attachment 49758 [details]
Chromium changes for plugin print
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. 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. 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
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 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.
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. Landed as http://trac.webkit.org/changeset/55814 |