Cannot preview PDFs

Cannot preview PDFs

In r180112, I removed some seemly dead code, but that code actually existed to
make some functions on PrintContext virtual. Instead of hacking in virtuals via
a subclass in the web layer, we can just make the functions virtual in the base
class.

I've also added OVERRIDE annotations to catch this sort of error at compile
time. These annotations required me to fix the return type of one of these
functions to match. Finally, I've added a test, which appears to be the first
unit test to exercise this codepath.

R=vitalybuka@chromium.org
BUG=403467

[Vitaly Buka (NO REVIEWS), 2014-08-13 21:17:42]

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContext.h
File Source/core/page/PrintContext.h (right):

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContex...
Source/core/page/PrintContext.h:43: ~PrintContext();
virtual destructor?

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContex...
Source/core/page/PrintContext.h:59: const IntRect& pageRect(size_t pageNumber)
const { return m_pageRects[pageNumber]; }
pageRect maybe called with incorrect pageNumber if overrided pageCount() returns
different value.

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContex...
Source/core/page/PrintContext.h:60: const Vector<IntRect>& pageRects() const {
return m_pageRects; }
same

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContex...
Source/core/page/PrintContext.h:64: void begin(float width, float height = 0);
begin should be virtual for consistency

[abarth-chromium, 2014-08-13 21:23:14]

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContext.h
File Source/core/page/PrintContext.h (right):

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContex...
Source/core/page/PrintContext.h:43: ~PrintContext();
Will do.

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContex...
Source/core/page/PrintContext.h:60: const Vector<IntRect>& pageRects() const {
return m_pageRects; }
That's true.  What do you recommend?

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContex...
Source/core/page/PrintContext.h:64: void begin(float width, float height = 0);
Will do.

[Vitaly Buka (NO REVIEWS), 2014-08-13 21:43:44]

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContext.h
File Source/core/page/PrintContext.h (right):

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContex...
Source/core/page/PrintContext.h:55: void computePageRectsWithPageSize(const
FloatSize& pageSizeInPixels, bool allowHorizontalTiling);
virtual, because it's also m_pageRects related

and override with NOTREACHED for ChromePrintContext

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContex...
Source/core/page/PrintContext.h:60: const Vector<IntRect>& pageRects() const {
return m_pageRects; }
On 2014/08/13 21:23:14, abarth wrote:
> That's true.  What do you recommend?

maybe 
/*no virtual*/ size_t pageCount() const { return pageRects().size(); }

/*no virtual*/ const IntRect& pageRect(size_t pageNumber) const { return
pageRects()[pageNumber]; }

virtual const Vector<IntRect>& pageRects() const { return m_pageRects; }

and overrides pageRects() only

[Vitaly Buka (NO REVIEWS), 2014-08-13 21:45:02]

On 2014/08/13 21:43:44, Vitaly Buka wrote:
>
https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContext.h
> File Source/core/page/PrintContext.h (right):
> 
>
https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContex...
> Source/core/page/PrintContext.h:55: void computePageRectsWithPageSize(const
> FloatSize& pageSizeInPixels, bool allowHorizontalTiling);
> virtual, because it's also m_pageRects related
> 
> and override with NOTREACHED for ChromePrintContext
> 
>
https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContex...
> Source/core/page/PrintContext.h:60: const Vector<IntRect>& pageRects() const {
> return m_pageRects; }
> On 2014/08/13 21:23:14, abarth wrote:
> > That's true.  What do you recommend?
> 
> maybe 
> /*no virtual*/ size_t pageCount() const { return pageRects().size(); }
> 
> /*no virtual*/ const IntRect& pageRect(size_t pageNumber) const { return
> pageRects()[pageNumber]; }
> 
> virtual const Vector<IntRect>& pageRects() const { return m_pageRects; }
> 
> and overrides pageRects() only

Ideally to have PrintContext as pure interface without m_pageRects
implementation

[Vitaly Buka (NO REVIEWS), 2014-08-13 21:53:27]

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContext.h
File Source/core/page/PrintContext.h (right):

https://codereview.chromium.org/469833003/diff/1/Source/core/page/PrintContex...
Source/core/page/PrintContext.h:61: 
Actually even better to do following
all 3 non virtul with no any overrides in ChromePluginPrintContext:
pageCount(), pageRect(size_t pageNumber), pageRects()

but you just need to change following
ChromePluginPrintContext::computePageRects{
m_pageRects.resize(m_plugin->printBegin(m_printParams))
for each rect in m_pageRects rect.size = IntRect(printRect)
}

ChromePluginPrintContext::m_pageCount can be deleted

[Vitaly Buka (NO REVIEWS), 2014-08-13 22:03:52]

https://codereview.chromium.org/476463002

[abarth-chromium, 2014-08-13 22:08:05]

On 2014/08/13 at 22:03:52, vitalybuka wrote:
> https://codereview.chromium.org/476463002

Do you want to land that version or should I merge your changes into this CL?

[Vitaly Buka (NO REVIEWS), 2014-08-13 23:04:05]

On 2014/08/13 22:08:05, abarth wrote:
> On 2014/08/13 at 22:03:52, vitalybuka wrote:
> > https://codereview.chromium.org/476463002
> 
> Do you want to land that version or should I merge your changes into this CL?

Up to you. I have clicked CQ there already.

[abarth-chromium, 2014-08-13 23:04:52]

Closed in favor of vitalybuka's version