Bug 134488 - Async loading of image resources
Summary: Async loading of image resources
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 143326
Blocks:
  Show dependency treegraph
 
Reported: 2014-07-01 04:18 PDT by Yoav Weiss
Modified: 2016-01-05 10:33 PST (History)
18 users (show)

See Also:


Attachments
Patch (12.39 KB, patch)
2014-07-01 04:26 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (169.65 KB, application/zip)
2014-07-01 05:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (162.67 KB, application/zip)
2014-07-01 06:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (168.90 KB, application/zip)
2014-07-01 06:32 PDT, Build Bot
no flags Details
Patch (17.38 KB, patch)
2014-10-01 06:32 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (23.23 KB, patch)
2014-10-01 13:23 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (344.74 KB, application/zip)
2014-10-01 15:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (235.49 KB, application/zip)
2014-10-01 15:56 PDT, Build Bot
no flags Details
Patch (19.12 KB, patch)
2014-10-06 03:06 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (24.50 KB, patch)
2014-10-06 03:57 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (24.49 KB, patch)
2014-10-06 04:09 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (244.83 KB, application/zip)
2014-10-06 05:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (246.93 KB, application/zip)
2014-10-06 06:02 PDT, Build Bot
no flags Details
Patch (15.80 KB, patch)
2015-03-08 19:24 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-mavericks (162.82 KB, application/zip)
2015-03-08 20:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (156.85 KB, application/zip)
2015-03-08 20:08 PDT, Build Bot
no flags Details
Patch (26.19 KB, patch)
2015-03-25 03:47 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (27.91 KB, patch)
2015-03-25 06:26 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (239.71 KB, application/zip)
2015-03-25 07:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (240.78 KB, application/zip)
2015-03-25 07:12 PDT, Build Bot
no flags Details
Patch (28.82 KB, patch)
2015-03-27 01:53 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (740.99 KB, application/zip)
2015-03-27 02:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (791.82 KB, application/zip)
2015-03-27 02:49 PDT, Build Bot
no flags Details
Patch (42.15 KB, patch)
2015-03-30 01:20 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (738.95 KB, application/zip)
2015-03-30 02:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-mavericks (716.13 KB, application/zip)
2015-03-30 02:22 PDT, Build Bot
no flags Details
Patch (42.19 KB, patch)
2015-03-30 05:21 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (42.11 KB, patch)
2015-03-30 05:30 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (661.46 KB, application/zip)
2015-03-30 06:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (655.26 KB, application/zip)
2015-03-30 06:11 PDT, Build Bot
no flags Details
Patch (41.37 KB, patch)
2015-03-30 12:25 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (42.12 KB, patch)
2015-04-01 00:56 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (35.88 KB, patch)
2015-12-04 00:18 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-yosemite (292.75 KB, application/zip)
2015-12-04 00:50 PST, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-yosemite (478.33 KB, application/zip)
2015-12-04 00:52 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (360.19 KB, application/zip)
2015-12-04 00:54 PST, Build Bot
no flags Details
Patch (42.04 KB, patch)
2015-12-04 02:56 PST, Yoav Weiss
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (895.67 KB, application/zip)
2015-12-04 03:42 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.27 MB, application/zip)
2015-12-04 03:45 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.48 MB, application/zip)
2015-12-04 04:02 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yoav Weiss 2014-07-01 04:18:02 PDT
Async loading of image resources
Comment 1 Yoav Weiss 2014-07-01 04:26:02 PDT
Created attachment 234157 [details]
Patch
Comment 2 Yoav Weiss 2014-07-01 04:27:06 PDT
Comment on attachment 234157 [details]
Patch

This patch is far from being ready for review. I've uploaded it in order to be able to discuss its technical approach
Comment 3 WebKit Commit Bot 2014-07-01 04:27:56 PDT
Attachment 234157 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Build Bot 2014-07-01 05:40:37 PDT
Comment on attachment 234157 [details]
Patch

Attachment 234157 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6295351275290624

New failing tests:
canvas/philip/tests/2d.composite.canvas.destination-in.html
canvas/philip/tests/2d.composite.canvas.source-out.html
canvas/philip/tests/2d.composite.clip.copy.html
canvas/philip/tests/2d.composite.canvas.copy.html
accessibility/alt-tag-on-image-with-nonimage-role.html
canvas/philip/tests/2d.clearRect.zero.html
canvas/philip/tests/2d.clearRect.globalalpha.html
canvas/philip/tests/2d.composite.canvas.source-in.html
canvas/philip/tests/2d.composite.canvas.destination-out.html
canvas/philip/tests/2d.clearRect.globalcomposite.html
canvas/philip/tests/2d.clearRect.clip.html
canvas/philip/tests/2d.composite.canvas.source-atop.html
canvas/philip/tests/2d.composite.clip.destination-in.html
canvas/philip/tests/2d.clearRect.shadow.html
canvas/philip/tests/2d.composite.clip.destination-over.html
canvas/philip/tests/2d.clearRect.transform.html
canvas/philip/tests/2d.clearRect.negative.html
canvas/philip/tests/2d.clearRect.path.html
canvas/philip/tests/2d.composite.clip.destination-atop.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
canvas/philip/tests/2d.composite.canvas.destination-over.html
canvas/philip/tests/2d.composite.canvas.destination-atop.html
canvas/philip/tests/2d.composite.clip.destination-out.html
canvas/philip/tests/2d.composite.canvas.lighter.html
canvas/philip/tests/2d.composite.canvas.xor.html
canvas/philip/tests/2d.composite.canvas.source-over.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
canvas/philip/tests/2d.clearRect.nonfinite.html
Comment 5 Build Bot 2014-07-01 05:40:45 PDT
Created attachment 234160 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 6 Build Bot 2014-07-01 06:20:42 PDT
Comment on attachment 234157 [details]
Patch

Attachment 234157 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5532361072574464

New failing tests:
canvas/philip/tests/2d.composite.canvas.destination-in.html
canvas/philip/tests/2d.composite.canvas.source-out.html
canvas/philip/tests/2d.composite.clip.copy.html
canvas/philip/tests/2d.composite.canvas.copy.html
accessibility/alt-tag-on-image-with-nonimage-role.html
canvas/philip/tests/2d.clearRect.zero.html
canvas/philip/tests/2d.clearRect.globalalpha.html
canvas/philip/tests/2d.composite.canvas.source-in.html
canvas/philip/tests/2d.composite.canvas.destination-out.html
canvas/philip/tests/2d.clearRect.globalcomposite.html
canvas/philip/tests/2d.clearRect.clip.html
canvas/philip/tests/2d.composite.canvas.source-atop.html
canvas/philip/tests/2d.composite.clip.destination-in.html
canvas/philip/tests/2d.clearRect.shadow.html
canvas/philip/tests/2d.composite.clip.destination-over.html
canvas/philip/tests/2d.clearRect.transform.html
canvas/philip/tests/2d.clearRect.negative.html
canvas/philip/tests/2d.clearRect.path.html
canvas/philip/tests/2d.composite.clip.destination-atop.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
canvas/philip/tests/2d.composite.canvas.destination-over.html
canvas/philip/tests/2d.composite.canvas.destination-atop.html
canvas/philip/tests/2d.composite.clip.destination-out.html
canvas/philip/tests/2d.composite.canvas.lighter.html
canvas/philip/tests/2d.composite.canvas.xor.html
canvas/philip/tests/2d.composite.canvas.source-over.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
canvas/philip/tests/2d.clearRect.nonfinite.html
Comment 7 Build Bot 2014-07-01 06:20:49 PDT
Created attachment 234163 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 8 Build Bot 2014-07-01 06:32:25 PDT
Comment on attachment 234157 [details]
Patch

Attachment 234157 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4580072602206208

New failing tests:
canvas/philip/tests/2d.composite.canvas.destination-in.html
canvas/philip/tests/2d.composite.canvas.source-out.html
canvas/philip/tests/2d.composite.clip.copy.html
canvas/philip/tests/2d.composite.canvas.copy.html
accessibility/alt-tag-on-image-with-nonimage-role.html
canvas/philip/tests/2d.clearRect.zero.html
canvas/philip/tests/2d.clearRect.globalalpha.html
canvas/philip/tests/2d.composite.canvas.source-in.html
canvas/philip/tests/2d.composite.canvas.destination-out.html
canvas/philip/tests/2d.clearRect.globalcomposite.html
canvas/philip/tests/2d.clearRect.clip.html
canvas/philip/tests/2d.composite.canvas.source-atop.html
canvas/philip/tests/2d.composite.clip.destination-in.html
canvas/philip/tests/2d.clearRect.shadow.html
canvas/philip/tests/2d.composite.clip.destination-over.html
canvas/philip/tests/2d.clearRect.transform.html
canvas/philip/tests/2d.clearRect.negative.html
canvas/philip/tests/2d.clearRect.path.html
canvas/philip/tests/2d.composite.clip.destination-atop.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
canvas/philip/tests/2d.composite.canvas.destination-over.html
canvas/philip/tests/2d.composite.canvas.destination-atop.html
canvas/philip/tests/2d.composite.clip.destination-out.html
canvas/philip/tests/2d.composite.canvas.lighter.html
canvas/philip/tests/2d.composite.canvas.xor.html
canvas/philip/tests/2d.composite.canvas.source-over.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
canvas/philip/tests/2d.clearRect.nonfinite.html
Comment 9 Build Bot 2014-07-01 06:32:33 PDT
Created attachment 234164 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 10 Yoav Weiss 2014-10-01 06:32:35 PDT
Created attachment 239023 [details]
Patch
Comment 11 Yoav Weiss 2014-10-01 06:34:54 PDT
Added a microtask abstraction, and register the image loading off of that.

This is not yet destined for review. Uploading just to test it on the bots.
Comment 12 Yoav Weiss 2014-10-01 13:23:18 PDT
Created attachment 239044 [details]
Patch
Comment 13 Build Bot 2014-10-01 15:34:39 PDT
Comment on attachment 239044 [details]
Patch

Attachment 239044 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4828100001005568

New failing tests:
dom/xhtml/level2/html/HTMLDocument03.xhtml
dom/xhtml/level2/html/HTMLDocument16.xhtml
dom/xhtml/level2/html/HTMLDocument07.xhtml
dom/xhtml/level2/html/HTMLDocument13.xhtml
http/tests/history/popstate-fires-with-pending-requests.html
css3/flexbox/relayout-image-load.html
canvas/philip/tests/2d.pattern.image.incomplete.empty.html
canvas/philip/tests/2d.drawImage.incomplete.html
dom/xhtml/level2/html/HTMLDocument05.xhtml
dom/xhtml/level2/html/HTMLDocument01.xhtml
dom/xhtml/level2/html/HTMLDocument08.xhtml
dom/xhtml/level2/html/HTMLDocument11.xhtml
dom/xhtml/level2/html/HTMLDocument10.xhtml
dom/xhtml/level2/html/HTMLDocument15.xhtml
dom/xhtml/level2/html/HTMLBodyElement09.xhtml
dom/xhtml/level2/html/HTMLDocument14.xhtml
dom/xhtml/level2/html/HTMLDocument04.xhtml
dom/xhtml/level2/html/HTMLDocument09.xhtml
dom/xhtml/level2/html/HTMLBodyElement10.xhtml
dom/xhtml/level2/html/HTMLDocument02.xhtml
dom/xhtml/level2/html/HTMLBodyElement12.xhtml
dom/xhtml/level2/html/HTMLBodyElement07.xhtml
canvas/philip/tests/2d.pattern.image.incomplete.omitted.html
compositing/video/video-poster.html
dom/xhtml/level2/html/HTMLBodyElement11.xhtml
css2.1/t0801-c412-hz-box-00-b-a.html
dom/xhtml/level2/html/HTMLDocument12.xhtml
canvas/philip/tests/2d.pattern.image.incomplete.html
canvas/philip/tests/2d.drawImage.image.incomplete.omitted.html
dom/xhtml/level2/html/HTMLBodyElement08.xhtml
Comment 14 Build Bot 2014-10-01 15:34:47 PDT
Created attachment 239058 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 15 Build Bot 2014-10-01 15:56:23 PDT
Comment on attachment 239044 [details]
Patch

Attachment 239044 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4579083971198976

New failing tests:
dom/xhtml/level2/html/HTMLDocument20.xhtml
dom/xhtml/level2/html/HTMLDocument03.xhtml
dom/xhtml/level2/html/HTMLDocument16.xhtml
dom/xhtml/level2/html/HTMLDocument07.xhtml
dom/xhtml/level2/html/HTMLDocument13.xhtml
http/tests/history/popstate-fires-with-pending-requests.html
dom/xhtml/level2/html/HTMLBodyElement09.xhtml
dom/xhtml/level2/html/HTMLDocument05.xhtml
dom/xhtml/level2/html/HTMLDocument01.xhtml
dom/xhtml/level2/html/HTMLDocument08.xhtml
dom/xhtml/level2/html/HTMLDocument11.xhtml
dom/xhtml/level2/html/HTMLDocument10.xhtml
dom/xhtml/level2/html/HTMLDocument15.xhtml
dom/xhtml/level2/html/HTMLDocument19.xhtml
canvas/philip/tests/2d.drawImage.incomplete.html
dom/xhtml/level2/html/HTMLDocument14.xhtml
dom/xhtml/level2/html/HTMLDocument04.xhtml
dom/xhtml/level2/html/HTMLDocument09.xhtml
dom/xhtml/level2/html/HTMLDocument18.xhtml
dom/xhtml/level2/html/HTMLBodyElement10.xhtml
dom/xhtml/level2/html/HTMLDocument02.xhtml
dom/xhtml/level2/html/HTMLBodyElement12.xhtml
dom/xhtml/level2/html/HTMLBodyElement07.xhtml
dom/xhtml/level2/html/HTMLBodyElement11.xhtml
dom/xhtml/level2/html/HTMLDocument17.xhtml
dom/xhtml/level2/html/HTMLDocument12.xhtml
dom/xhtml/level2/html/HTMLDocument21.xhtml
css2.1/t0801-c412-hz-box-00-b-a.html
canvas/philip/tests/2d.drawImage.image.incomplete.omitted.html
dom/xhtml/level2/html/HTMLBodyElement08.xhtml
Comment 16 Build Bot 2014-10-01 15:56:28 PDT
Created attachment 239061 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 17 Yoav Weiss 2014-10-06 03:06:44 PDT
Created attachment 239322 [details]
Patch
Comment 18 Yoav Weiss 2014-10-06 03:57:21 PDT
Created attachment 239325 [details]
Patch
Comment 19 Yoav Weiss 2014-10-06 03:58:31 PDT
Comment on attachment 239325 [details]
Patch

This version of the patch seems to be working, but I still need to add tests before I can submit it for official review.
Comment 20 Yoav Weiss 2014-10-06 04:09:57 PDT
Created attachment 239327 [details]
Patch
Comment 21 Build Bot 2014-10-06 05:21:38 PDT
Comment on attachment 239327 [details]
Patch

Attachment 239327 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6256222529388544

Number of test failures exceeded the failure limit.
Comment 22 Build Bot 2014-10-06 05:21:44 PDT
Created attachment 239329 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 23 Build Bot 2014-10-06 06:01:56 PDT
Comment on attachment 239327 [details]
Patch

Attachment 239327 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6636140304007168

Number of test failures exceeded the failure limit.
Comment 24 Build Bot 2014-10-06 06:02:05 PDT
Created attachment 239332 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 25 Yoav Weiss 2015-03-08 19:24:55 PDT
Created attachment 248223 [details]
Patch
Comment 26 Darin Adler 2015-03-08 19:25:57 PDT
Comment on attachment 248223 [details]
Patch

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

> Source/WebCore/loader/ImageLoader.h:131
> +    class ImageLoaderTask : public MicroTask {
> +    public:
> +        static std::unique_ptr<ImageLoaderTask> create(WeakPtr<ImageLoader> loader)
> +        {
> +            return std::unique_ptr<ImageLoaderTask>(new ImageLoaderTask(loader));
> +        }
> +
> +        virtual void run()
> +        {
> +            if (m_loader && !m_loader->cancelledTask())
> +                m_loader->doUpdateFromElement();
> +        }
> +
> +    private:
> +        ImageLoaderTask(WeakPtr<ImageLoader> loader)
> +            : m_loader(loader)
> +        {
> +        }
> +
> +        WeakPtr<ImageLoader> m_loader;
> +    };

There’s no need to have this whole class in the header.
Comment 27 Yoav Weiss 2015-03-08 19:28:12 PDT
Thanks! I'll pull it out of there.

This patch is not yet in good shape for review. I mainly uploaded it to see if the tests are passing on the bots.
Comment 28 Darin Adler 2015-03-08 19:29:14 PDT
Comment on attachment 248223 [details]
Patch

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

> Source/WebCore/loader/ImageLoader.h:49
> +    WTF_MAKE_NONCOPYABLE(IncrementLoadEventDelayCount);

I’m not sure this needs to be here.

> Source/WebCore/loader/ImageLoader.h:57
> +    IncrementLoadEventDelayCount() { };
> +    IncrementLoadEventDelayCount(IncrementLoadEventDelayCount&& other)
> +        : m_document(WTF::move(other.m_document))
> +    {
> +        other.m_document.clear();
> +    }

These function bodies don’t need to be in the header. Also, there’s no need to declare the empty constructor at all.

The code here to move a WeakPtr should be put into the WeakPtr template. If that was done correctly, then we would not need to explicitly define a move constructor at all.

> Source/WebCore/loader/ImageLoader.h:67
> +    WeakPtr<Document> m_document;

I am not sure we need a WeakPtr here. I believe that callers already handle the lifetime for the document properly and destroy the image loader before the document is destroyed; this WeakPtr would add unnecessary overhead.
Comment 29 Build Bot 2015-03-08 20:05:06 PDT
Comment on attachment 248223 [details]
Patch

Attachment 248223 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5543544144003072

Number of test failures exceeded the failure limit.
Comment 30 Build Bot 2015-03-08 20:05:11 PDT
Created attachment 248225 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 31 Build Bot 2015-03-08 20:08:26 PDT
Comment on attachment 248223 [details]
Patch

Attachment 248223 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6608190301011968

Number of test failures exceeded the failure limit.
Comment 32 Build Bot 2015-03-08 20:08:32 PDT
Created attachment 248226 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 33 Yoav Weiss 2015-03-25 03:47:35 PDT
Created attachment 249397 [details]
Patch
Comment 34 Yoav Weiss 2015-03-25 03:54:40 PDT
I've uploaded a new patch that works (and passes tests), at least locally.

It includes a few additional `runMicroTasks()` calls after events and scheduled JS actions. I'm not sure these are required by spec, but without them, some images fail to load in certain tested scenarios. I'm also not sure if and how they should be protected against nested call scenarios. Advice would be appreciated.
Comment 35 Yoav Weiss 2015-03-25 06:26:32 PDT
Created attachment 249398 [details]
Patch
Comment 36 Build Bot 2015-03-25 07:08:31 PDT
Comment on attachment 249398 [details]
Patch

Attachment 249398 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5233336238735360

Number of test failures exceeded the failure limit.
Comment 37 Build Bot 2015-03-25 07:08:39 PDT
Created attachment 249399 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 38 Build Bot 2015-03-25 07:12:45 PDT
Comment on attachment 249398 [details]
Patch

Attachment 249398 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5707102806867968

Number of test failures exceeded the failure limit.
Comment 39 Build Bot 2015-03-25 07:12:54 PDT
Created attachment 249400 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 40 Yoav Weiss 2015-03-27 01:53:15 PDT
Created attachment 249558 [details]
Patch
Comment 41 Build Bot 2015-03-27 02:42:40 PDT
Comment on attachment 249558 [details]
Patch

Attachment 249558 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5466378211426304

New failing tests:
http/tests/security/frame-loading-via-document-write.html
http/tests/misc/image-blocked-src-change.html
http/tests/security/contentSecurityPolicy/report-blocked-file-uri.html
http/tests/security/isolatedWorld/bypass-main-world-csp.html
svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg
http/tests/misc/image-blocked-src-no-change.html
Comment 42 Build Bot 2015-03-27 02:42:48 PDT
Created attachment 249560 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 43 Build Bot 2015-03-27 02:49:34 PDT
Comment on attachment 249558 [details]
Patch

Attachment 249558 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5647604356481024

New failing tests:
http/tests/security/frame-loading-via-document-write.html
http/tests/misc/image-blocked-src-change.html
http/tests/security/contentSecurityPolicy/report-blocked-file-uri.html
http/tests/security/local-video-poster-from-remote.html
http/tests/security/isolatedWorld/bypass-main-world-csp.html
http/tests/security/local-image-from-remote.html
svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg
http/tests/misc/image-blocked-src-no-change.html
Comment 44 Build Bot 2015-03-27 02:49:40 PDT
Created attachment 249561 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 45 Yoav Weiss 2015-03-30 01:20:39 PDT
Created attachment 249716 [details]
Patch
Comment 46 Build Bot 2015-03-30 02:04:10 PDT
Comment on attachment 249716 [details]
Patch

Attachment 249716 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6447122182307840

New failing tests:
svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg
Comment 47 Build Bot 2015-03-30 02:04:17 PDT
Created attachment 249717 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 48 Build Bot 2015-03-30 02:21:57 PDT
Comment on attachment 249716 [details]
Patch

Attachment 249716 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6144721051189248

New failing tests:
svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg
Comment 49 Build Bot 2015-03-30 02:22:02 PDT
Created attachment 249718 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 50 Yoav Weiss 2015-03-30 05:21:34 PDT
Created attachment 249722 [details]
Patch
Comment 51 Yoav Weiss 2015-03-30 05:30:20 PDT
Created attachment 249723 [details]
Patch
Comment 52 Build Bot 2015-03-30 06:08:31 PDT
Comment on attachment 249723 [details]
Patch

Attachment 249723 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5804133902712832

New failing tests:
http/tests/security/local-video-poster-from-remote.html
Comment 53 Build Bot 2015-03-30 06:08:37 PDT
Created attachment 249724 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 54 Build Bot 2015-03-30 06:11:34 PDT
Comment on attachment 249723 [details]
Patch

Attachment 249723 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5998535396818944

New failing tests:
http/tests/security/local-video-poster-from-remote.html
Comment 55 Build Bot 2015-03-30 06:11:39 PDT
Created attachment 249725 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 56 Yoav Weiss 2015-03-30 12:25:40 PDT
Created attachment 249755 [details]
Patch
Comment 57 Yoav Weiss 2015-03-30 14:46:57 PDT
Finally all green! A review would be appreciated :)
Comment 58 Dean Jackson 2015-03-31 12:16:41 PDT
Comment on attachment 249755 [details]
Patch

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

> Source/WebCore/html/HTMLImageLoader.cpp:81
> +    if (!cachedImage)
> +        return;
> +    ASSERT(cachedImage);

I really hope that ASSERT can never fire :)

> Source/WebCore/loader/ImageLoader.cpp:200
> +    // set url value only if srcURI is not empty. Otherwise, url will be the URL for the document itself.

Nit: "Set" not "set"
Comment 59 Chris Dumez 2015-03-31 12:27:29 PDT
Comment on attachment 249755 [details]
Patch

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

> Source/WebCore/loader/ImageLoader.cpp:72
> +    static std::unique_ptr<ImageLoaderTask> create(WeakPtr<ImageLoader> loader, bool shouldBypassMainWorldContentSecurityPolicy)

nit: I think we decided to get rid of these factory methods for std::unique_ptr and use std::make_unique<>() at call sites. Note that it requires making the constructor public though.

> Source/WebCore/loader/ImageLoader.h:80
> +    bool pendingTask() const { return m_pendingTask; }

for booleans, we prefer names that start with "is" or "has", in this case, hasPendingTask() / m_hasPendingTask would be nicer.

> Source/WebCore/loader/ImageLoader.h:109
> +    bool shouldLoadImmediately(const AtomicString&) const;

I don't think we should omit the argument name here as it is not obvious what it is.
Comment 60 Chris Dumez 2015-03-31 12:36:37 PDT
Comment on attachment 249755 [details]
Patch

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

> Source/WebCore/loader/ImageLoader.cpp:77
> +    virtual void run()

Missing override. Does this really need to be public?

> Source/WebCore/loader/ImageLoader.cpp:196
> +    AtomicString attr = m_element.imageSourceURL();

attr is really not a great variable name.

> Source/WebCore/loader/ImageLoader.cpp:301
> +    AtomicString attr = m_element.imageSourceURL();

attr is not a great variable name.

> Source/WebCore/loader/ImageLoader.cpp:319
> +            doUpdateFromElement(false);

Boolean arguments like this are really not readable. We usually use enum classes to improve readability:
enum class ShouldBypassMainWorldContentSecurityPolicy { No, Yes };
doUpdateFromElement(ShouldBypassMainWorldContentSecurityPolicy::No);
Comment 61 Yoav Weiss 2015-04-01 00:56:55 PDT
Created attachment 249902 [details]
Patch
Comment 62 WebKit Commit Bot 2015-04-01 12:03:25 PDT
Comment on attachment 249902 [details]
Patch

Clearing flags on attachment: 249902

Committed r182247: <http://trac.webkit.org/changeset/182247>
Comment 63 WebKit Commit Bot 2015-04-01 12:03:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 64 Dean Jackson 2015-04-01 15:43:57 PDT
This has broken the following test:
fast/events/mouse-cursor-image-set.html
Comment 65 Dean Jackson 2015-04-01 15:44:24 PDT
I've filed  https://bugs.webkit.org/show_bug.cgi?id=143323
Comment 67 Dean Jackson 2015-04-01 17:12:12 PDT
I think we'll have to roll it out.
Comment 68 WebKit Commit Bot 2015-04-01 17:14:42 PDT
Re-opened since this is blocked by bug 143326
Comment 70 Jon Lee 2015-12-02 14:35:31 PST
Can we revive this patch? Is there an issue that we should be blocking this bug on?
Comment 71 Yoav Weiss 2015-12-02 14:41:36 PST
I think it'd be best to wait for https://bugs.webkit.org/show_bug.cgi?id=147933 to land and then base this patch on that microtask abstraction (which AFAIU should be more correct than it currently is)
Comment 72 Jon Lee 2015-12-03 11:51:38 PST
(In reply to comment #71)
> I think it'd be best to wait for
> https://bugs.webkit.org/show_bug.cgi?id=147933 to land and then base this
> patch on that microtask abstraction (which AFAIU should be more correct than
> it currently is)

It's landed now!
Comment 73 Yoav Weiss 2015-12-03 12:17:52 PST
Cool! I'll rebase this patch to see where we're at
Comment 74 Radar WebKit Bug Importer 2015-12-03 18:51:05 PST
<rdar://problem/23754810>
Comment 75 Yoav Weiss 2015-12-04 00:18:37 PST
Created attachment 266607 [details]
Patch
Comment 76 Build Bot 2015-12-04 00:50:01 PST
Comment on attachment 266607 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 77 Build Bot 2015-12-04 00:50:07 PST
Created attachment 266609 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 78 Build Bot 2015-12-04 00:52:10 PST
Comment on attachment 266607 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 79 Build Bot 2015-12-04 00:52:16 PST
Created attachment 266610 [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 80 Build Bot 2015-12-04 00:54:22 PST
Comment on attachment 266607 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 81 Build Bot 2015-12-04 00:54:29 PST
Created attachment 266611 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 82 Yoav Weiss 2015-12-04 02:56:20 PST
Created attachment 266613 [details]
Patch
Comment 83 Build Bot 2015-12-04 03:41:57 PST
Comment on attachment 266613 [details]
Patch

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

New failing tests:
fast/dom/MutationObserver/added-out-of-order.html
http/tests/misc/image-blocked-src-change.html
fast/dom/MutationObserver/parser-mutations.html
js/dom/Promise-static-resolve.html
webarchive/loading/test-loading-archive-subresource-null-mimetype.html
fast/dom/MutationObserver/removed-out-of-order.html
fast/dom/MutationObserver/takeRecords.html
webarchive/loading/cache-expired-subresource.html
http/tests/misc/image-blocked-src-no-change.html
Comment 84 Build Bot 2015-12-04 03:42:03 PST
Created attachment 266616 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 85 Build Bot 2015-12-04 03:45:46 PST
Comment on attachment 266613 [details]
Patch

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

New failing tests:
http/tests/contentextensions/domain-rules.html
http/tests/misc/image-blocked-src-change.html
fast/dom/MutationObserver/parser-mutations.html
http/tests/contentextensions/block-image-load-in-onunload.html
fast/mediastream/MediaDevices-getUserMedia.html
http/tests/contentextensions/basic-filter.html
fast/dom/MutationObserver/added-out-of-order.html
http/tests/security/isolatedWorld/image-load-should-not-bypass-main-world-csp.html
http/tests/misc/image-blocked-src-no-change.html
http/tests/contentextensions/filters-with-quantifiers-combined.html
http/tests/contentextensions/block-csp-report.html
fast/mediastream/getusermedia.html
fast/dom/MutationObserver/removed-out-of-order.html
fast/dom/MutationObserver/takeRecords.html
webarchive/loading/cache-expired-subresource.html
webarchive/loading/test-loading-archive-subresource-null-mimetype.html
js/dom/Promise-static-resolve.html
Comment 86 Build Bot 2015-12-04 03:45:52 PST
Created attachment 266617 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 87 Build Bot 2015-12-04 04:02:18 PST
Comment on attachment 266613 [details]
Patch

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

New failing tests:
fast/workers/worker-timeout.html
http/tests/misc/image-blocked-src-change.html
fast/dom/MutationObserver/parser-mutations.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-overrides.html
fast/workers/worker-strict.html
fast/dom/MutationObserver/added-out-of-order.html
http/tests/security/isolatedWorld/image-load-should-not-bypass-main-world-csp.html
http/tests/misc/image-blocked-src-no-change.html
fast/dom/MutationObserver/takeRecords.html
webarchive/loading/test-loading-archive-subresource-null-mimetype.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-simple.html
fast/dom/MutationObserver/removed-out-of-order.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-twice.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-synconworker.html
webarchive/loading/cache-expired-subresource.html
http/tests/websocket/tests/hybi/workers/no-onmessage-in-sync-op.html
js/dom/Promise-static-resolve.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-aborted.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-overridesexpires.html
Comment 88 Build Bot 2015-12-04 04:02:26 PST
Created attachment 266618 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 89 Darin Adler 2015-12-11 09:24:49 PST
Comment on attachment 266613 [details]
Patch

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

> Source/WebCore/loader/ImageLoader.cpp:70
> +class ImageLoader::ImageLoaderTask : public Microtask {

Should be marked final.

> Source/WebCore/loader/ImageLoader.cpp:72
> +    ImageLoaderTask(WeakPtr<ImageLoader> loader, CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy shouldBypassMainWorldContentSecurityPolicy)

This should take a WeakPtr&& and use WTF::move, and we should add a move constructor and move assignment operator to WeakPtr.

> Source/WebCore/loader/ImageLoader.cpp:192
>      AtomicString attr = element().imageSourceURL();

I don’t see any reason to put this into a local variable now that we are only calling a single function on it.

> Source/WebCore/loader/ImageLoader.cpp:193
> +    String srcURI = sourceURI(attr);

I would name this local variable sourceURLString. In fact, I think the virtual function sourceURI should be renamed to sourceURLString in the future. Then this line of code would be:

    String sourceURLString = this->sourceURLString(element().imageSourceURL());

> Source/WebCore/loader/ImageLoader.cpp:194
> +    URL url;

I would name this local variable sourceURL.

> Source/WebCore/loader/ImageLoader.cpp:201
>      CachedResourceHandle<CachedImage> newImage = nullptr;

No need for the "= nullptr" here.

> Source/WebCore/loader/ImageLoader.cpp:210
> +        AtomicString crossOriginMode = m_element.fastGetAttribute(HTMLNames::crossoriginAttr);

The type here should be auto& or const AtomicString&; there is no need to do a bit of reference count churn on the attribute value just to call equalIgnoringCase on it.

> Source/WebCore/loader/ImageLoader.cpp:211
>          if (!crossOriginMode.isNull()) {

Is behavior here correct? crossorigin="" means do not allow stored credentials? Do we have test cases covering this?

> Source/WebCore/loader/ImageLoader.cpp:312
> +    CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy shouldBypassMainWorldContentSecurityPolicy = CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy::No;

It would be better to use auto here so we didn’t have such a breathtakingly long line of code:

    auto shouldBypassMainWorldContentSecurityPolicy = CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy::No;

Also, for a local variable with small scope like this, I think we can use a bit of shorthand:

    auto shouldBypassPolicy = CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy::No

> Source/WebCore/loader/ImageLoader.cpp:326
> +bool ImageLoader::shouldLoadImmediately(const AtomicString& attribute) const

This function needs a why comment explaining its policies. Why are each of these valid reasons we should load immediately? I’m not even sure these are all correct or all tested.

> Source/WebCore/loader/ImageLoader.cpp:328
> +    String srcURI = sourceURI(attribute);

Please don’t use the name URI for any new code. I would call this sourceURLString. Also, I think early return would make this easier to read.

    auto sourceURLString = sourceURI(attribute);
    if (sourceURLString.isEmpty())
        return true;

> Source/WebCore/loader/ImageLoader.cpp:329
> +    URL url = element().document().completeURL(srcURI);

I would call this sourceURL.

> Source/WebCore/loader/ImageLoader.cpp:330
> +    ResourceRequest resourceRequest(url);

I suggest constructing this only where it’s used and not putting it into a local variable.

> Source/WebCore/loader/ImageLoader.cpp:331
> +    return (srcURI.isEmpty()

No need for the parentheses here.

> Source/WebCore/loader/ImageLoader.cpp:333
> +        || m_loadManually

This should be an early exit before we do any other work.

> Source/WebCore/loader/ImageLoader.cpp:334
> +        || !is<HTMLImageElement>(m_element)

This should be an early exit before we do work on the attribute value.

> Source/WebCore/loader/ImageLoader.cpp:336
> +        || MemoryCache::singleton().resourceForRequest(resourceRequest, element().document().page()->sessionID()));

What guarantees page() is non-null?

> Source/WebCore/loader/ImageLoader.cpp:349
> +    ASSERT(resource);
>      ASSERT(resource == m_image.get());

Not sure I understand the need for a separate assertion here.

Further, we should simply change notifyFinished to take a CachedResource& instead of a CachedResource* in the future. There’s no chance this is going to be null.
Comment 90 Jon Lee 2016-01-05 10:33:57 PST
ping?