Bug 172495 - Zoom in/out is slow in Safari with large PDFs
Summary: Zoom in/out is slow in Safari with large PDFs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-23 02:01 PDT by Tim Horton
Modified: 2017-05-23 11:58 PDT (History)
4 users (show)

See Also:

Attachments
Patch (2.13 KB, patch)
2017-05-23 02:01 PDT, Tim Horton 		no flags Details | Formatted Diff | Diff
Patch (4.98 KB, patch)
2017-05-23 11:50 PDT, Tim Horton 		simon.fraser: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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