Bug 172495 – Zoom in/out is slow in Safari with large PDFs

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Zoom in/out is slow in Safari with large PDFs

[Tim Horton]

Created attachment 310987 [details]
Patch

[Andreas Kling]

Comment on attachment 310987 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310987&action=review

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1156
> +    size_t pageCount = [m_pdfDocument pageCount];
> +    for (size_t i = 0; i < pageCount; ++i)
> +        m_pageBoxes.append(enclosingIntRect([[m_pdfDocument pageAtIndex:i] boundsForBox:kPDFDisplayBoxCropBox]));

I'm no expert on this code, but shouldn't we clear m_pageBoxes somehow before appending all the boxes to it?
Or is this going to only run once?

[Sam Weinig]

Comment on attachment 310987 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310987&action=review

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1156
>> +        m_pageBoxes.append(enclosingIntRect([[m_pdfDocument pageAtIndex:i] boundsForBox:kPDFDisplayBoxCropBox]));
> 
> I'm no expert on this code, but shouldn't we clear m_pageBoxes somehow before appending all the boxes to it?
> Or is this going to only run once?

Also, since we know how many things are getting appened, you can reserve capacity and then do uncheckedAppends.

[Tim Horton]

(In reply to Andreas Kling from comment #2)
> Comment on attachment 310987 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310987&action=review
> 
> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1156
> > +    size_t pageCount = [m_pdfDocument pageCount];
> > +    for (size_t i = 0; i < pageCount; ++i)
> > +        m_pageBoxes.append(enclosingIntRect([[m_pdfDocument pageAtIndex:i] boundsForBox:kPDFDisplayBoxCropBox]));
> 
> I'm no expert on this code, but shouldn't we clear m_pageBoxes somehow
> before appending all the boxes to it?
> Or is this going to only run once?

LOL, yes, and this has always been broken, but has never really mattered much because we only use the first box... which means we can simplify this a great deal and do even less work!

[Tim Horton]

Created attachment 311035 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 311035 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311035&action=review

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1160
> +    m_firstPageHeight = static_cast<unsigned>(CGCeiling([[m_pdfDocument pageAtIndex:0] boundsForBox:kPDFDisplayBoxCropBox].size.height));

Do we always have a page 0?

[Tim Horton]

http://trac.webkit.org/changeset/217288/webkit