r203378 made it possible to cache only a sub-image of the PDF first and then draw this sub-image to the destination context. What was left is fixing the source and destination rectangles when drawing from the PDF to the cached image and when drawing from the cached image to the destination context. Mostly the old code, which was using the whole image rectangle for both srcRect and dstRect, were left as it was before r203378.
Created attachment 285462 [details] Patch
<rdar://problem/27627628>
Created attachment 285466 [details] Patch
Created attachment 285472 [details] The fix without the test case
Comment on attachment 285472 [details] The fix without the test case View in context: https://bugs.webkit.org/attachment.cgi?id=285472&action=review > Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:170 > + FloatRect rect = context.clipBounds(); This could use a better name; maybe "dirtyRect". It's not clear why you don't intersect with dstRect up front. Why shift the edges to the edge of the clip bounds, when the clip bounds may be far outside dstRect?
Created attachment 285742 [details] Patch
Created attachment 285743 [details] The fix without the test case 2
Comment on attachment 285472 [details] The fix without the test case View in context: https://bugs.webkit.org/attachment.cgi?id=285472&action=review >> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:170 >> + FloatRect rect = context.clipBounds(); > > This could use a better name; maybe "dirtyRect". > > It's not clear why you don't intersect with dstRect up front. Why shift the edges to the edge of the clip bounds, when the clip bounds may be far outside dstRect? I changed the logic in this function. Please see the new uploaded patches.
Comment on attachment 285742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285742&action=review > Source/WebCore/ChangeLog:23 > + rename it cachingPDFImage. Allow "limited" option to be averrable for other Did you mean "available"? > Source/WebCore/page/Settings.h:80 > +enum class CachingPDFImage { I'm not sure about the name here. CachingPDFImagePolicy? PDFImageCachingPolicy? PDFImageCachingMode? It would probably be worth a comment about expected behavior. Limited and Clipped don't explain enough. And maybe Clipped should be renamed ClippedForTestingOnly. > Source/WebCore/page/Settings.h:376 > + CachingPDFImage m_cachingPDFImage { CachingPDFImage::Enabled }; Please rename. The variable sounds like it's an image, not a policy or strategy. > Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:105 > + m_cachingPDFImage = caching; Is it correct to always destroy the data when this is called, even if m_cachingPDFImage == caching?
Created attachment 285878 [details] Patch
Comment on attachment 285742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285742&action=review >> Source/WebCore/ChangeLog:23 >> + rename it cachingPDFImage. Allow "limited" option to be averrable for other > > Did you mean "available"? Yes I meant "available" but autocorrect changed it to "averrable". >> Source/WebCore/page/Settings.h:80 >> +enum class CachingPDFImage { > > I'm not sure about the name here. CachingPDFImagePolicy? PDFImageCachingPolicy? PDFImageCachingMode? > > It would probably be worth a comment about expected behavior. Limited and Clipped don't explain enough. And maybe Clipped should be renamed ClippedForTestingOnly. Done. I changed it to PDFImageCachingPolicy. >> Source/WebCore/page/Settings.h:376 >> + CachingPDFImage m_cachingPDFImage { CachingPDFImage::Enabled }; > > Please rename. The variable sounds like it's an image, not a policy or strategy. Done. But I am not sure about the coding style with a variable whose type starts with capital letters as PDFImageCachingPolicy. I renamed this member to be PDFImageCachingPolicy m_pdfImageCachingPolicy { PDFImageCachingPolicy::Enabled }; >> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:105 >> + m_cachingPDFImage = caching; > > Is it correct to always destroy the data when this is called, even if m_cachingPDFImage == caching? Done. I added the following if-statement at the beginning of this function: if (m_pdfImageCachingPolicy == pdfImageCachingPolicy) return;
Comment on attachment 285878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285878&action=review > Source/WebCore/page/Settings.h:373 > +#if PLATFORM(IOS) I'm not sure that this should be platform-specific. Or at least not here? You should ask around. > Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:251 > + // We need to transform the coordinates system such that top-left of m_cachedImageRect will be mapped to the s/coordinates/coordinate/ > Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:252 > + // top-left of dstRect. Although only m_cachedImageRect.size() of the image copied, the sizes of srcRect You have two spaces between image and copied. > Source/WebCore/testing/InternalSettings.cpp:491 > + if (equalLettersIgnoringASCIICase(policy, "disabled")) { Not an enum?
(In reply to comment #12) > Comment on attachment 285878 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=285878&action=review > > > Source/WebCore/page/Settings.h:373 > > +#if PLATFORM(IOS) > > I'm not sure that this should be platform-specific. Or at least not here? > You should ask around. I do like that you've made the various different modes testable on all platforms, though, that's a big improvement!
Created attachment 286734 [details] Patch
Comment on attachment 285878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285878&action=review >>> Source/WebCore/page/Settings.h:373 >>> +#if PLATFORM(IOS) >> >> I'm not sure that this should be platform-specific. Or at least not here? You should ask around. > > I do like that you've made the various different modes testable on all platforms, though, that's a big improvement! I asked around if we need to apply this fix on other platforms and the response I got was "No we do not need to". The reasons are 1. We are fixing a memory jetsam limitation on iOS which does not exist on other platforms. So why do we apply the same fix for platforms which we have not encountered any problem with in this scenario? 2. We do not have layout tests or benchmarks for rendering large PDF images to know the impact of this fix on other platforms. All the tests and the verifications of the fixes are done manually. 3. By enabling limiting caching the PDF to a certain size and rectangle on other platforms. we may encounter other bugs which do not happen now and hard to reproduce on iOS. So why do we take the risk for that? I also moved the definition of this member to Settings.in. I added an enum value which is named PDFImageCachingDefault. Its value can be either PDFImageCachingEnabled or PDFImageCachingBelowMemoryLimit. I initialized Setting::m_pdfImageCachingPolicy to PDFImageCachingDefault. >> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:251 >> + // We need to transform the coordinates system such that top-left of m_cachedImageRect will be mapped to the > > s/coordinates/coordinate/ Fixed. >> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:252 >> + // top-left of dstRect. Although only m_cachedImageRect.size() of the image copied, the sizes of srcRect > > You have two spaces between image and copied. Fixed.
Comment on attachment 286734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286734&action=review > Source/WebCore/testing/InternalSettings.idl:71 > + [RaisesException] void setPdfImageCachingPolicy(DOMString policy); This really should be PDF
Created attachment 286983 [details] Patch
Comment on attachment 286983 [details] Patch Clearing flags on attachment: 286983 Committed r204983: <http://trac.webkit.org/changeset/204983>
All reviewed patches have been landed. Closing bug.
Reopening to attach new patch.
Created attachment 287045 [details] Patch
(In reply to comment #21) > Created attachment 287045 [details] > Patch I uploaded this patch by mistake.