[webkit-reviews] review denied: [Bug 35550] Allow a plugin to participate in the browser's print workflow. : [Attachment 50362] Updated patch with code review changes
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 9 21:07:33 PST 2010
Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied
sanjeevr at chromium.org's request for review:
Bug 35550: Allow a plugin to participate in the browser's print workflow.
https://bugs.webkit.org/show_bug.cgi?id=35550
Attachment 50362: Updated patch with code review changes
https://bugs.webkit.org/attachment.cgi?id=50362&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> 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?
More information about the webkit-reviews
mailing list