[webkit-reviews] review requested: [Bug 52595] Make basic printing work in WebKit2 : [Attachment 79225] proposed patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has asked  for review:
Bug 52595: Make basic printing work in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=52595

Attachment 79225: proposed patch
https://bugs.webkit.org/attachment.cgi?id=79225&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
>> +	return decoder->decode(CoreIPC::Out(t.pageSetupScaleFactor,
t.availablePaperWidth, t.availablePaperHeight));
> > +}
>
> t, eh?

There's a lot of precedent for "t" (all WebEvent code, for example). But I
can't see any reason for that, so changed to info.

> I think our usual unwritten style rule is that if we use public data members
without m_ prefixes, 
> I use “struct”. Making this struct instead of class would follow that
convention.

I didn't think that it could be done here, because autogenerated code was
making "class" forward declarations. But turns out that there is an override in
code generation script.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2357
> +#if PLATFORM(MAC)
>
> Seems a shame to have this all inside #if. Can we compile more of this on
non-Mac
> just to keep things a bit more tidy? What’s the downside of doing that?

I'd like to think about how the data should be passed on other platforms later.
It doesn't seem great to have several seemingly equal drawRectToXXX() methods,
of which only one will work for any given platform.

> Mitz told me that it’s OK to use a Vector as a return result in cases like
this and it won’t copy the data.

I think that it's easier to use an out argument than to worry whether return
value optimization will work in each particular case.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:169
> +NSString * const PrintedFrameKey = @"WebKitPrintedFrameKey";
>
> Is this another case where const gives internal linkage without using the
static keyword explicitly?

Yes.

> +    if (frame->isMainFrame() && _data->_pdfViewController)
> +	   return [super knowsPageRange:range];
>
> Is the isMainFrame test needed here? Is it helpful?

I'm not entirely sure. Obviously, PDF views don't have subframes, but I wanted
to be explicit about what case is being handled here.

> > +	 if ([NSGraphicsContext currentContextDrawingToScreen]) {
>
> I believe this will return NO for some non-printing cases

Ugh. I've added a FIXME for now.

> > +	 DrawRectToPDF(uint64_t frameID, WebCore::IntRect rect) ->
(Vector<uint8_t> pdfData)
> 
> Will it be more efficient to do this with DataReference?

I tried that at first, but DataReference doesn't take ownership of the data, so
it can't be used for "out" arguments. Having MallocScribble always on in Xcode
debugger saved me this time.