Bug 155498 - Move caching the ImageFrame from BitmapImage to ImageSource
Summary: Move caching the ImageFrame from BitmapImage to ImageSource
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on: 155456 159819
Blocks: 155322 155546
  Show dependency treegraph
 
Reported: 2016-03-15 11:44 PDT by Said Abou-Hallawa
Modified: 2017-05-25 11:09 PDT (History)
12 users (show)

See Also:


Attachments
Patch (162.95 KB, patch)
2016-03-15 11:51 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (162.76 KB, patch)
2016-03-15 12:08 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (159.70 KB, patch)
2016-03-15 12:34 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (159.82 KB, patch)
2016-03-15 12:55 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (160.68 KB, patch)
2016-03-15 13:25 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch for review (66.77 KB, patch)
2016-03-15 13:56 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (308.18 KB, patch)
2016-08-10 15:31 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (919.71 KB, application/zip)
2016-08-10 16:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (482.52 KB, application/zip)
2016-08-10 16:24 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.14 MB, application/zip)
2016-08-10 16:34 PDT, Build Bot
no flags Details
Patch (314.47 KB, patch)
2016-08-10 17:33 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (314.71 KB, patch)
2016-08-10 17:42 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (314.81 KB, patch)
2016-08-10 18:20 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (316.90 KB, patch)
2016-08-10 18:56 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (317.10 KB, patch)
2016-08-10 19:08 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (319.68 KB, patch)
2016-08-10 19:26 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (716.94 KB, application/zip)
2016-08-10 20:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (471.81 KB, application/zip)
2016-08-10 20:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.03 MB, application/zip)
2016-08-10 20:22 PDT, Build Bot
no flags Details
Patch (319.69 KB, patch)
2016-08-10 23:22 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (319.70 KB, patch)
2016-08-11 00:11 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (526.65 KB, application/zip)
2016-08-11 01:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (474.25 KB, application/zip)
2016-08-11 01:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (946.13 KB, application/zip)
2016-08-11 01:18 PDT, Build Bot
no flags Details
Patch (319.70 KB, patch)
2016-08-11 09:22 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (704.77 KB, application/zip)
2016-08-11 10:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (852.96 KB, application/zip)
2016-08-11 10:24 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (1.16 MB, application/zip)
2016-08-11 10:43 PDT, Build Bot
no flags Details
Patch (320.13 KB, patch)
2016-08-11 11:23 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-yosemite (934.98 KB, application/zip)
2016-08-11 13:21 PDT, Build Bot
no flags Details
Patch (320.07 KB, patch)
2016-08-11 15:34 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-yosemite (1.51 MB, application/zip)
2016-08-11 16:47 PDT, Build Bot
no flags Details
Patch (320.12 KB, patch)
2016-08-11 16:57 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (320.12 KB, patch)
2016-08-11 18:33 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (320.12 KB, patch)
2016-08-12 10:09 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch for review (118.56 KB, patch)
2016-08-12 11:36 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (92.33 KB, patch)
2016-09-21 16:59 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (97.72 KB, patch)
2016-09-21 17:30 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (111.51 KB, patch)
2016-09-21 17:59 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (113.12 KB, patch)
2016-09-21 18:16 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-yosemite (1.83 MB, application/zip)
2016-09-21 19:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-yosemite (1.29 MB, application/zip)
2016-09-21 19:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.48 MB, application/zip)
2016-09-21 19:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2 (1.22 MB, application/zip)
2016-09-21 19:22 PDT, Build Bot
no flags Details
Patch (119.27 KB, patch)
2016-09-22 09:30 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (119.78 KB, patch)
2016-09-22 09:46 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-yosemite (1.90 MB, application/zip)
2016-09-22 10:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (1.15 MB, application/zip)
2016-09-22 10:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 (1.14 MB, application/zip)
2016-09-22 10:51 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.20 MB, application/zip)
2016-09-22 10:51 PDT, Build Bot
no flags Details
Patch (120.32 KB, patch)
2016-09-22 14:22 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (120.25 KB, patch)
2016-09-22 14:49 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-yosemite (1.74 MB, application/zip)
2016-09-22 15:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2 (7.07 MB, application/zip)
2016-09-22 15:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (1.20 MB, application/zip)
2016-09-22 16:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.28 MB, application/zip)
2016-09-22 16:09 PDT, Build Bot
no flags Details
Patch (120.26 KB, patch)
2016-09-22 17:19 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (117.35 KB, patch)
2016-09-26 13:40 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (121.65 KB, patch)
2016-09-26 14:37 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (121.70 KB, patch)
2016-09-26 15:55 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (121.74 KB, patch)
2016-09-26 16:29 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (124.75 KB, patch)
2016-09-26 19:06 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (125.70 KB, patch)
2016-09-27 09:44 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (126.24 KB, patch)
2016-09-27 10:57 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (126.23 KB, patch)
2016-09-27 11:38 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (127.38 KB, patch)
2016-09-27 16:06 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (127.41 KB, patch)
2016-09-27 17:47 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2016-03-15 11:44:25 PDT
The goal is to have caching the ImageFrameData be managed by the ImageSource which can be delegated to the ImageDecoder in the case of asynchronous image decoding case.
Comment 1 Said Abou-Hallawa 2016-03-15 11:51:24 PDT
Created attachment 274105 [details]
Patch
Comment 2 Said Abou-Hallawa 2016-03-15 12:08:02 PDT
Created attachment 274108 [details]
Patch
Comment 3 Said Abou-Hallawa 2016-03-15 12:34:03 PDT
Created attachment 274111 [details]
Patch
Comment 4 Said Abou-Hallawa 2016-03-15 12:55:54 PDT
Created attachment 274115 [details]
Patch
Comment 5 Said Abou-Hallawa 2016-03-15 13:25:04 PDT
Created attachment 274118 [details]
Patch
Comment 6 Said Abou-Hallawa 2016-03-15 13:56:37 PDT
Created attachment 274124 [details]
Patch for review
Comment 7 Brent Fulgham 2016-03-25 11:09:13 PDT
Comment on attachment 274124 [details]
Patch for review

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

I think this looks good, but I wonder if hiding more of the direct m_source access wouldn't be better.

> Source/WebCore/platform/graphics/BitmapImage.cpp:-57
> -    , m_frameCount(0)

As long as you're cleaning this up, could you change these other initializations to use the newer C++ style?

> Source/WebCore/platform/graphics/BitmapImage.cpp:92
> +    ImageFrameIndex clearBeforeFrame = destroyAll ? m_source.m_frames.size() : m_currentFrame;

It's a little weird that we expose the m_frames member like this, especially when you have other methods like "frameSizeAtIndex". Would it be better to do something like m_source.frameCount() or similar?

> Source/WebCore/platform/graphics/BitmapImage.cpp:109
> +    if (!data() && m_source.m_frames.size())

Ditto m_frames

> Source/WebCore/platform/graphics/BitmapImage.cpp:112
> +    if (m_source.m_frames.framesBytes() > largeAnimationCutoff)

m_source.bytesInAllFrames()?

> Source/WebCore/platform/graphics/ImageFrameData.cpp:79
> +        m_duration = (duration < 0.011f) ? 0.100f : duration;

This change isn't really explained in your ChangeLog. Why are you allowing these short durations now?
Comment 8 Said Abou-Hallawa 2016-08-10 15:31:53 PDT
Created attachment 285774 [details]
Patch
Comment 9 Build Bot 2016-08-10 16:17:15 PDT
Comment on attachment 285774 [details]
Patch

Attachment 285774 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1848238

Number of test failures exceeded the failure limit.
Comment 10 Build Bot 2016-08-10 16:17:18 PDT
Created attachment 285778 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 11 Build Bot 2016-08-10 16:24:07 PDT
Comment on attachment 285774 [details]
Patch

Attachment 285774 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1848244

Number of test failures exceeded the failure limit.
Comment 12 Build Bot 2016-08-10 16:24:10 PDT
Created attachment 285780 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 13 Build Bot 2016-08-10 16:34:55 PDT
Comment on attachment 285774 [details]
Patch

Attachment 285774 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1848264

New failing tests:
media/video-controls-drag.html
fast/events/drag-and-drop-autoscroll-inner-frame.html
Comment 14 Build Bot 2016-08-10 16:34:59 PDT
Created attachment 285781 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 15 Said Abou-Hallawa 2016-08-10 17:33:39 PDT
Created attachment 285789 [details]
Patch
Comment 16 Said Abou-Hallawa 2016-08-10 17:42:00 PDT
Created attachment 285791 [details]
Patch
Comment 17 Said Abou-Hallawa 2016-08-10 18:20:09 PDT
Created attachment 285796 [details]
Patch
Comment 18 Said Abou-Hallawa 2016-08-10 18:56:56 PDT
Created attachment 285800 [details]
Patch
Comment 19 Said Abou-Hallawa 2016-08-10 19:08:01 PDT
Created attachment 285802 [details]
Patch
Comment 20 Said Abou-Hallawa 2016-08-10 19:26:13 PDT
Created attachment 285806 [details]
Patch
Comment 21 Build Bot 2016-08-10 20:12:50 PDT
Comment on attachment 285806 [details]
Patch

Attachment 285806 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1849142

Number of test failures exceeded the failure limit.
Comment 22 Build Bot 2016-08-10 20:12:54 PDT
Created attachment 285808 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 23 Build Bot 2016-08-10 20:18:51 PDT
Comment on attachment 285806 [details]
Patch

Attachment 285806 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1849143

Number of test failures exceeded the failure limit.
Comment 24 Build Bot 2016-08-10 20:18:55 PDT
Created attachment 285809 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 25 Build Bot 2016-08-10 20:22:26 PDT
Comment on attachment 285806 [details]
Patch

Attachment 285806 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1849145

New failing tests:
media/video-controls-drag.html
fast/events/drag-and-drop-autoscroll-inner-frame.html
Comment 26 Build Bot 2016-08-10 20:22:30 PDT
Created attachment 285810 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 27 Said Abou-Hallawa 2016-08-10 23:22:26 PDT
Created attachment 285815 [details]
Patch
Comment 28 Said Abou-Hallawa 2016-08-11 00:11:04 PDT
Created attachment 285816 [details]
Patch
Comment 29 Build Bot 2016-08-11 01:02:12 PDT
Comment on attachment 285816 [details]
Patch

Attachment 285816 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1850290

Number of test failures exceeded the failure limit.
Comment 30 Build Bot 2016-08-11 01:02:16 PDT
Created attachment 285818 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 31 Build Bot 2016-08-11 01:08:01 PDT
Comment on attachment 285816 [details]
Patch

Attachment 285816 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1850294

Number of test failures exceeded the failure limit.
Comment 32 Build Bot 2016-08-11 01:08:05 PDT
Created attachment 285819 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 33 Build Bot 2016-08-11 01:18:38 PDT
Comment on attachment 285816 [details]
Patch

Attachment 285816 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1850323

New failing tests:
media/video-controls-drag.html
fast/events/drag-and-drop-autoscroll-inner-frame.html
Comment 34 Build Bot 2016-08-11 01:18:42 PDT
Created attachment 285820 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 35 Said Abou-Hallawa 2016-08-11 09:22:14 PDT
Created attachment 285829 [details]
Patch
Comment 36 Build Bot 2016-08-11 10:09:02 PDT
Comment on attachment 285829 [details]
Patch

Attachment 285829 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1852560

Number of test failures exceeded the failure limit.
Comment 37 Build Bot 2016-08-11 10:09:07 PDT
Created attachment 285831 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 38 Build Bot 2016-08-11 10:24:22 PDT
Comment on attachment 285829 [details]
Patch

Attachment 285829 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1852592

New failing tests:
media/video-controls-drag.html
fast/events/drag-and-drop-autoscroll-inner-frame.html
Comment 39 Build Bot 2016-08-11 10:24:26 PDT
Created attachment 285832 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 40 Build Bot 2016-08-11 10:43:40 PDT
Comment on attachment 285829 [details]
Patch

Attachment 285829 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1852682

Number of test failures exceeded the failure limit.
Comment 41 Build Bot 2016-08-11 10:43:43 PDT
Created attachment 285834 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 42 Said Abou-Hallawa 2016-08-11 11:23:12 PDT
Created attachment 285837 [details]
Patch
Comment 43 Build Bot 2016-08-11 13:21:32 PDT
Comment on attachment 285837 [details]
Patch

Attachment 285837 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1853286

Number of test failures exceeded the failure limit.
Comment 44 Build Bot 2016-08-11 13:21:36 PDT
Created attachment 285846 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 45 Said Abou-Hallawa 2016-08-11 15:34:10 PDT
Created attachment 285862 [details]
Patch
Comment 46 Build Bot 2016-08-11 16:47:02 PDT
Comment on attachment 285862 [details]
Patch

Attachment 285862 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1854176

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 47 Build Bot 2016-08-11 16:47:05 PDT
Created attachment 285867 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 48 Said Abou-Hallawa 2016-08-11 16:57:25 PDT
Created attachment 285869 [details]
Patch
Comment 49 Said Abou-Hallawa 2016-08-11 18:33:32 PDT
Created attachment 285879 [details]
Patch
Comment 50 Said Abou-Hallawa 2016-08-12 10:09:42 PDT
Created attachment 285917 [details]
Patch
Comment 51 Said Abou-Hallawa 2016-08-12 11:36:55 PDT
Created attachment 285923 [details]
Patch for review
Comment 52 Said Abou-Hallawa 2016-09-21 16:59:34 PDT
Created attachment 289492 [details]
Patch
Comment 53 Said Abou-Hallawa 2016-09-21 17:30:06 PDT
Created attachment 289494 [details]
Patch
Comment 54 Said Abou-Hallawa 2016-09-21 17:59:40 PDT
Created attachment 289497 [details]
Patch
Comment 55 Said Abou-Hallawa 2016-09-21 18:16:54 PDT
Created attachment 289498 [details]
Patch
Comment 56 Build Bot 2016-09-21 19:05:24 PDT
Comment on attachment 289498 [details]
Patch

Attachment 289498 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2121947

New failing tests:
fast/replaced/outline-replaced-elements-offset.html
fast/images/favicon-as-image.html
http/tests/multipart/invalid-image-data-standalone.html
http/tests/misc/favicon-as-image.html
fast/events/standalone-image-drag-to-editable.html
http/tests/misc/acid3.html
Comment 57 Build Bot 2016-09-21 19:05:30 PDT
Created attachment 289503 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 58 Build Bot 2016-09-21 19:16:23 PDT
Comment on attachment 289498 [details]
Patch

Attachment 289498 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2122074

New failing tests:
fast/replaced/outline-replaced-elements-offset.html
fast/images/favicon-as-image.html
http/tests/multipart/invalid-image-data-standalone.html
http/tests/misc/favicon-as-image.html
fast/events/standalone-image-drag-to-editable.html
http/tests/misc/acid3.html
Comment 59 Build Bot 2016-09-21 19:16:29 PDT
Created attachment 289504 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 60 Build Bot 2016-09-21 19:16:57 PDT
Comment on attachment 289498 [details]
Patch

Attachment 289498 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2122060

New failing tests:
fast/replaced/outline-replaced-elements-offset.html
http/tests/multipart/invalid-image-data-standalone.html
http/tests/misc/favicon-as-image.html
http/tests/misc/acid3.html
fast/images/favicon-as-image.html
Comment 61 Build Bot 2016-09-21 19:17:03 PDT
Created attachment 289505 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 62 Build Bot 2016-09-21 19:22:03 PDT
Comment on attachment 289498 [details]
Patch

Attachment 289498 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2122033

Number of test failures exceeded the failure limit.
Comment 63 Build Bot 2016-09-21 19:22:07 PDT
Created attachment 289507 [details]
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 64 Said Abou-Hallawa 2016-09-22 09:30:29 PDT
Created attachment 289566 [details]
Patch
Comment 65 Said Abou-Hallawa 2016-09-22 09:46:54 PDT
Created attachment 289568 [details]
Patch
Comment 66 Build Bot 2016-09-22 10:35:41 PDT
Comment on attachment 289568 [details]
Patch

Attachment 289568 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2126519

New failing tests:
fast/replaced/outline-replaced-elements-offset.html
fast/images/favicon-as-image.html
http/tests/multipart/invalid-image-data-standalone.html
http/tests/misc/favicon-as-image.html
fast/events/standalone-image-drag-to-editable.html
http/tests/misc/acid3.html
Comment 67 Build Bot 2016-09-22 10:35:46 PDT
Created attachment 289571 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 68 Build Bot 2016-09-22 10:46:05 PDT
Comment on attachment 289568 [details]
Patch

Attachment 289568 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2126606

New failing tests:
fast/replaced/outline-replaced-elements-offset.html
fast/images/favicon-as-image.html
http/tests/multipart/invalid-image-data-standalone.html
http/tests/misc/favicon-as-image.html
fast/events/standalone-image-drag-to-editable.html
http/tests/misc/acid3.html
Comment 69 Build Bot 2016-09-22 10:46:10 PDT
Created attachment 289572 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 70 Build Bot 2016-09-22 10:51:06 PDT
Comment on attachment 289568 [details]
Patch

Attachment 289568 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2126604

Number of test failures exceeded the failure limit.
Comment 71 Build Bot 2016-09-22 10:51:09 PDT
Comment on attachment 289568 [details]
Patch

Attachment 289568 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2126607

New failing tests:
fast/replaced/outline-replaced-elements-offset.html
http/tests/multipart/invalid-image-data-standalone.html
http/tests/misc/favicon-as-image.html
http/tests/misc/acid3.html
fast/images/favicon-as-image.html
Comment 72 Build Bot 2016-09-22 10:51:10 PDT
Created attachment 289573 [details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 73 Build Bot 2016-09-22 10:51:14 PDT
Created attachment 289574 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 74 Said Abou-Hallawa 2016-09-22 14:22:40 PDT
Created attachment 289597 [details]
Patch
Comment 75 Said Abou-Hallawa 2016-09-22 14:49:43 PDT
Created attachment 289605 [details]
Patch
Comment 76 Build Bot 2016-09-22 15:46:28 PDT
Comment on attachment 289605 [details]
Patch

Attachment 289605 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2128020

New failing tests:
http/tests/cache/memory-cache-pruning.html
Comment 77 Build Bot 2016-09-22 15:46:34 PDT
Created attachment 289615 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 78 Build Bot 2016-09-22 15:49:01 PDT
Comment on attachment 289605 [details]
Patch

Attachment 289605 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2128025

Number of test failures exceeded the failure limit.
Comment 79 Build Bot 2016-09-22 15:49:08 PDT
Created attachment 289617 [details]
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 80 Build Bot 2016-09-22 16:04:12 PDT
Comment on attachment 289605 [details]
Patch

Attachment 289605 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2128163

New failing tests:
http/tests/cache/memory-cache-pruning.html
Comment 81 Build Bot 2016-09-22 16:04:18 PDT
Created attachment 289623 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 82 Build Bot 2016-09-22 16:09:20 PDT
Comment on attachment 289605 [details]
Patch

Attachment 289605 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2128167

New failing tests:
http/tests/cache/memory-cache-pruning.html
Comment 83 Build Bot 2016-09-22 16:09:26 PDT
Created attachment 289625 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 84 Said Abou-Hallawa 2016-09-22 17:19:06 PDT
Created attachment 289633 [details]
Patch
Comment 85 Said Abou-Hallawa 2016-09-26 13:40:53 PDT
Created attachment 289863 [details]
Patch
Comment 86 Said Abou-Hallawa 2016-09-26 14:37:37 PDT
Created attachment 289871 [details]
Patch
Comment 87 Said Abou-Hallawa 2016-09-26 15:55:44 PDT
Created attachment 289882 [details]
Patch
Comment 88 Said Abou-Hallawa 2016-09-26 16:29:32 PDT
Created attachment 289889 [details]
Patch
Comment 89 Tim Horton 2016-09-26 17:19:12 PDT
Comment on attachment 289889 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:398
> +    const_cast<ImageSource&>(m_source).dump(ts);

Why the const_cast? Is something wrong somewhere else? Not usually a good sign.

> Source/WebCore/platform/graphics/BitmapImage.h:76
> +    String filenameExtension() const override { return const_cast<ImageSource&>(m_source).filenameExtension(); }
> +    Optional<IntPoint> hotSpot() const override { return const_cast<ImageSource&>(m_source).hotSpot(); }

Ditto on the const_casts. Lots around here. What's up with that?

> Source/WebCore/platform/graphics/ImageFrameCache.h:53
> +    // Image metadata which is calculated by the decoder or can deduced by the case of the memory NativeImage.

I can't parse the second half of this comment.

> Source/WebCore/platform/graphics/ImageFrameCache.h:107
> +    ImageDecoder *m_decoder { nullptr };

Star's on the wrong side.

> Source/WebCore/platform/graphics/ImageSource.cpp:91
> +    // If we have decoded frames but there is no encoded data, we shouldn't destroy
> +    // the decoded image since we won't be able to reconstruct it later.

How does that happen?

> Source/WebCore/platform/graphics/ImageSource.h:116
> +    // FIXME: We should expose a setting to enable/disable progressive loading remove the PLATFORM(IOS)-guard.

Grammar is not quite right here ("so that we can remove", maybe?)
Comment 90 Said Abou-Hallawa 2016-09-26 19:06:56 PDT
Created attachment 289899 [details]
Patch
Comment 91 Said Abou-Hallawa 2016-09-27 09:44:08 PDT
Created attachment 289957 [details]
Patch
Comment 92 Said Abou-Hallawa 2016-09-27 10:18:31 PDT
Comment on attachment 289889 [details]
Patch

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

>> Source/WebCore/platform/graphics/BitmapImage.cpp:398
>> +    const_cast<ImageSource&>(m_source).dump(ts);
> 
> Why the const_cast? Is something wrong somewhere else? Not usually a good sign.

Done. ImageSource::dump() is now a const function. All it does it calls ImageFrameCache::dump() which is also a const function. ImageFrameCache::dump() dumps the image metadata. It does not try to cache any of them. It dumps whatever is already cached.

The reason for having ImageSource::dump() not const from the beginning was, it was considered logically const. The only side effect, it had, was lazily caching some of the image metadata. But since this does not seem right, we can dump the existing values without having to mix const calls and non-const calls.

>> Source/WebCore/platform/graphics/BitmapImage.h:76
>> +    Optional<IntPoint> hotSpot() const override { return const_cast<ImageSource&>(m_source).hotSpot(); }
> 
> Ditto on the const_casts. Lots around here. What's up with that?

ImageSource::hotSpot() and ImageSource::filenameExtension() were not const because they may need to cache their values reading from the ImageDecoder. I could have done of one of two things to remove these const_cast:
1. Make ImageSource::hotSpot() and ImageSource::filenameExtension() const which would require ImageFrameCache::hotSpot() and ImageFrameCache::filenameExtension() to be const also. I tried that but I end up marking almost all of the functions in ImageFrameCache const and all the data members to be mutable. This did not seem right when marking functions like ImageFrameCache ::setFrameMetadata() or ImageFrameCache::decodedSizeChanged() be const. 
2. Remove the const qualifiers from the base class functions Image::hotSpot() and Image::filenameExtension(). Although this does not very elegant but I think this is the best we can do to fix this issue.

>> Source/WebCore/platform/graphics/ImageFrameCache.h:53
>> +    // Image metadata which is calculated by the decoder or can deduced by the case of the memory NativeImage.
> 
> I can't parse the second half of this comment.

This comment is now read as:

    // Image metadata which is calculated either by the ImageDecoder or directly
    // from the NativeImage if this class was created for a memory image.

>> Source/WebCore/platform/graphics/ImageFrameCache.h:107
>> +    ImageDecoder *m_decoder { nullptr };
> 
> Star's on the wrong side.

Fixed.

>> Source/WebCore/platform/graphics/ImageSource.cpp:91
>> +    // the decoded image since we won't be able to reconstruct it later.
> 
> How does that happen?

I think this can happen if the BitmapImage is created for a memory image in which case no ImageDecoder is needed and no encoded data is available. All we have is the NativeImagePtr and in this case we should not delete this frame since there is no way to get it back.

>> Source/WebCore/platform/graphics/ImageSource.h:116
>> +    // FIXME: We should expose a setting to enable/disable progressive loading remove the PLATFORM(IOS)-guard.
> 
> Grammar is not quite right here ("so that we can remove", maybe?)

Done.
Comment 93 Said Abou-Hallawa 2016-09-27 10:57:54 PDT
Created attachment 289981 [details]
Patch
Comment 94 Said Abou-Hallawa 2016-09-27 11:38:05 PDT
Created attachment 289985 [details]
Patch
Comment 95 Simon Fraser (smfr) 2016-09-27 13:08:18 PDT
Comment on attachment 289985 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        To remove this burden from the BitmapImage the member 'm_frames' is removed
> +        from BitmapImage. A new member named 'm_frameCache' of type ImageFrameCache
> +        is added to ImageSource. This class handles caching and recaching the image
> +        metadata and the ImageFrames if the image needs decoding. When the BitmapImage
> +        is initialized with a memory image, the ImageFrameCache initializes its
> +        metadata and ImageFrames directly from the NativeImagePtr.

I'm worried this is a bit too much splitting. Could ImageFrameCache be internal to ImageSource? Or can you more strongly justify splitting out ImageFrameCache because of future async decoding changes?

> Source/WebCore/ChangeLog:21
> +        When the BitmapImage replying ImageFrame queries, it will ask the ImageSource

is replying to?

> Source/WebCore/platform/graphics/BitmapImage.cpp:156
> +    float scale = subsamplingScale(context, destRect, srcRect);

If scale is only used when LOG() is enabled (debug), then won't this generate a warning in release builds?

> Source/WebCore/platform/graphics/BitmapImage.h:154
> +    bool isAnimated() const override { return const_cast<BitmapImage*>(this)->frameCount() > 1; }

This is a bit gross.

> Source/WebCore/platform/graphics/ImageFrame.h:121
> +    void setDuration(float duration) { m_duration = duration; }

Times are normally doubles. What units is this in?

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:56
> +    // Since we don't have a decoder, we can't figure out the image orientation.
> +    // Set m_sizeRespectingOrientation to be the same as m_size so it's not 0x0.

Can't we query CG for image metadata?

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:68
> +    unsigned decodedSize = 0;
> +    for (size_t i = 0; i <  count; ++i)
> +        decodedSize += m_frames[i].clearImage();

It's odd that we always clear data from the first frame. Don't we want to do so taking into account which frame is currently displayed?

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:155
> +void ImageFrameCache::ensureFramesSize()

This function has a confusing name, since "size" in most of the file refers to image size.

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:197
> +    frame.m_subsamplingLevel = subsamplingLevel;

It's weird that we consider subsamplingLevel as part of metadata, since it depends on the size at which the image is going to be rendered.
Comment 96 Said Abou-Hallawa 2016-09-27 16:04:31 PDT
Comment on attachment 289985 [details]
Patch

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

>> Source/WebCore/ChangeLog:19
>> +        metadata and ImageFrames directly from the NativeImagePtr.
> 
> I'm worried this is a bit too much splitting. Could ImageFrameCache be internal to ImageSource? Or can you more strongly justify splitting out ImageFrameCache because of future async decoding changes?

I added the following statement to the ChangeLog:

        The plan for ImageFrameCache is to be extended for the asynchronous image
        decoding and also to be used by the non CG image decoders which cache
        other copies of the ImageFrames. This double caching should be removed.

>> Source/WebCore/ChangeLog:21
>> +        When the BitmapImage replying ImageFrame queries, it will ask the ImageSource
> 
> is replying to?

Fixed.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:156
>> +    float scale = subsamplingScale(context, destRect, srcRect);
> 
> If scale is only used when LOG() is enabled (debug), then won't this generate a warning in release builds?

No. Because scale is used in this statement also:

    SubsamplingLevel subsamplingLevel = m_source.subsamplingLevelForScale(scale);

And subsamplingLevel is used in the LOG statement and in the following statement also:

    auto image = frameImageAtIndex(m_currentFrame, subsamplingLevel);

>> Source/WebCore/platform/graphics/BitmapImage.h:154
>> +    bool isAnimated() const override { return const_cast<BitmapImage*>(this)->frameCount() > 1; }
> 
> This is a bit gross.

I fixed all the const issues by making the BitmapImage::m_source member be mutable.

>> Source/WebCore/platform/graphics/ImageFrame.h:121
>> +    void setDuration(float duration) { m_duration = duration; }
> 
> Times are normally doubles. What units is this in?

We set ImageFrame::m_duration to the output of ImageDecoder::frameDurationAtIndex() which is float. It defined to be float for all ports.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:56
>> +    // Set m_sizeRespectingOrientation to be the same as m_size so it's not 0x0.
> 
> Can't we query CG for image metadata?

I do not think we can query CG for the image orientation and I do not think we need to do. This is the case where a BitmapImage, an ImageSource and an ImageFrameCache are created for a memory image. In this case the assumption is the memory image will be displayed with the default orientation.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:68
>> +        decodedSize += m_frames[i].clearImage();
> 
> It's odd that we always clear data from the first frame. Don't we want to do so taking into account which frame is currently displayed?

Yes we do. We always clear all the frames that comes before count = BitmapImage::m_currentFrame if destroyAll is false. I think this is the best choice since we are clearing the frames that have already been displayed and we are keeping the current one and any other one that comes after it.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:155
>> +void ImageFrameCache::ensureFramesSize()
> 
> This function has a confusing name, since "size" in most of the file refers to image size.

I renamed it to growFrames().

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:197
>> +    frame.m_subsamplingLevel = subsamplingLevel;
> 
> It's weird that we consider subsamplingLevel as part of metadata, since it depends on the size at which the image is going to be rendered.

Yes this is true. But the ImageFrame::m_size and ImageFrame::m_nativeImage are different for different subsamplingLevel. We want to check always if an ImageFrame is valid for a given subsamplingLevel. If it is not, we need to recache it. If the ImageFrames of an image were cached for a certain subsamplingLevel and then the subsamplingLevel got changed, we are going to recache the ImageFrames when any of them is requested. So at some point the ImageFrames of this image will have different subsamplingLevel. But this is fine since we may fix all of them later when we need to render them.
Comment 97 Said Abou-Hallawa 2016-09-27 16:06:10 PDT
Created attachment 290017 [details]
Patch
Comment 98 WebKit Commit Bot 2016-09-27 17:46:19 PDT
Comment on attachment 290017 [details]
Patch

Rejecting attachment 290017 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 290017, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2158164
Comment 99 Said Abou-Hallawa 2016-09-27 17:47:56 PDT
Created attachment 290033 [details]
Patch
Comment 100 WebKit Commit Bot 2016-09-27 18:09:18 PDT
Comment on attachment 290033 [details]
Patch

Clearing flags on attachment: 290033

Committed r206481: <http://trac.webkit.org/changeset/206481>
Comment 101 WebKit Commit Bot 2016-09-27 18:09:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 102 Radar WebKit Bug Importer 2017-05-25 11:09:53 PDT
<rdar://problem/32406251>