Bug 172495

Summary: Zoom in/out is slow in Safari with large PDFs
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, kling, sam, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch simon.fraser: review+

Description Tim Horton 2017-05-23 02:01:26 PDT
Zoom in/out is slow in Safari with large PDFs
Comment 1 Tim Horton 2017-05-23 02:01:52 PDT
Created attachment 310987 [details]
Patch
Comment 2 Andreas Kling 2017-05-23 09:49:38 PDT
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?
Comment 3 Sam Weinig 2017-05-23 10:10:01 PDT
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.
Comment 4 Tim Horton 2017-05-23 11:36:02 PDT
(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!
Comment 5 Tim Horton 2017-05-23 11:50:33 PDT
Created attachment 311035 [details]
Patch
Comment 6 Simon Fraser (smfr) 2017-05-23 11:53:06 PDT
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?
Comment 7 Tim Horton 2017-05-23 11:58:30 PDT
http://trac.webkit.org/changeset/217288/webkit