Bug 134488 – Async loading of image resources

• Status: REOPENED
• Component: New Bugs

[Yoav Weiss]

Async loading of image resources

[Yoav Weiss]

Created attachment 234157 [details]
Patch

[Yoav Weiss]

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

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Yoav Weiss]

Created attachment 239023 [details]
Patch

[Yoav Weiss]

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.

[Yoav Weiss]

Created attachment 239044 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Yoav Weiss]

Created attachment 239322 [details]
Patch

[Yoav Weiss]

Created attachment 239325 [details]
Patch

[Yoav Weiss]

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.

[Yoav Weiss]

Created attachment 239327 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Yoav Weiss]

Created attachment 248223 [details]
Patch

[Darin Adler]

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.

[Yoav Weiss]

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.

[Darin Adler]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Yoav Weiss]

Created attachment 249397 [details]
Patch

[Yoav Weiss]

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.

[Yoav Weiss]

Created attachment 249398 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Yoav Weiss]

Created attachment 249558 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Yoav Weiss]

Created attachment 249716 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Yoav Weiss]

Created attachment 249722 [details]
Patch

[Yoav Weiss]

Created attachment 249723 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Yoav Weiss]

Created attachment 249755 [details]
Patch

[Yoav Weiss]

Finally all green! A review would be appreciated :)

[Dean Jackson]

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"

[Chris Dumez]

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.

[Chris Dumez]

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);

[Yoav Weiss]

Created attachment 249902 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 249902 [details]
Patch

Clearing flags on attachment: 249902

Committed r182247: <http://trac.webkit.org/changeset/182247>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dean Jackson]

This has broken the following test:
fast/events/mouse-cursor-image-set.html

[Dean Jackson]

I've filed  https://bugs.webkit.org/show_bug.cgi?id=143323

[Alexey Proskuryakov]

There are more tests broken:

https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fsecurity%2Flocal-image-from-remote.html

and possibly this one, although it's flaky:

https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fsecurity%2Fframe-loading-via-document-write.html

[Dean Jackson]

I think we'll have to roll it out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 143326

[Alexey Proskuryakov]

Also these CSP tests were broken on Windows:

https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2Freport-uri-from-javascript.html

https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2Freport-uri-from-inline-javascript.html

[Jon Lee]

Can we revive this patch? Is there an issue that we should be blocking this bug on?

[Yoav Weiss]

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)

[Jon Lee]

(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!

[Yoav Weiss]

Cool! I'll rebase this patch to see where we're at

[Radar WebKit Bug Importer]

<rdar://problem/23754810>

[Yoav Weiss]

Created attachment 266607 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Yoav Weiss]

Created attachment 266613 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Jon Lee]

ping?

[Rob Buis]

Created attachment 376850 [details]
Patch

[EWS Watchlist]

Comment on attachment 376850 [details]
Patch

Attachment 376850 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12950660

New failing tests:
fast/images/decode-animated-image.html
fast/dom/MutationObserver/parser-mutations.html
fast/dom/Node/textContent-mutationEvents.html
http/tests/images/decode-slow-load-static-image.html
fast/images/decode-decoding-attribute-async-large-image.html
js/dom/Promise-static-resolve.html
http/tests/images/loading-image-no-border.html
fast/shadow-dom/signal-slot-list-retains-js-wrappers.html
fast/dom/MutationObserver/takeRecords.html
fast/dom/HTMLImageElement/sizes/image-sizes-js-change-reverse.html
fast/events/popup-blocked-from-untrusted-mouse-click.html

[EWS Watchlist]

Created attachment 376855 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Rob Buis]

Created attachment 376864 [details]
Patch

[EWS Watchlist]

Comment on attachment 376864 [details]
Patch

Attachment 376864 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12951479

New failing tests:
fast/images/decode-animated-image.html
http/tests/images/decode-slow-load-static-image.html
fast/images/decode-decoding-attribute-async-large-image.html
http/tests/security/isolatedWorld/image-load-should-not-bypass-main-world-csp.html
http/tests/images/loading-image-no-border.html
fast/dom/HTMLImageElement/sizes/image-sizes-js-change-reverse.html
fast/events/popup-blocked-from-untrusted-mouse-click.html

[EWS Watchlist]

Created attachment 376875 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Rob Buis]

Created attachment 376896 [details]
Patch

[EWS Watchlist]

Comment on attachment 376896 [details]
Patch

Attachment 376896 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12952662

New failing tests:
fast/dom/HTMLImageElement/sizes/image-sizes-js-change-reverse.html
http/tests/images/loading-image-no-border.html
fast/events/popup-blocked-from-untrusted-mouse-click.html
http/tests/security/isolatedWorld/image-load-should-not-bypass-main-world-csp.html

[EWS Watchlist]

Created attachment 376910 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Rob Buis]

Created attachment 376993 [details]
Patch

[EWS Watchlist]

Comment on attachment 376993 [details]
Patch

Attachment 376993 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12955667

New failing tests:
fast/dom/HTMLImageElement/sizes/image-sizes-js-change-reverse.html
http/tests/images/loading-image-no-border.html

[EWS Watchlist]

Created attachment 376995 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Rob Buis]

Created attachment 377027 [details]
Patch

[Rob Buis]

Created attachment 377036 [details]
Patch

[EWS Watchlist]

Comment on attachment 377036 [details]
Patch

Attachment 377036 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12958158

New failing tests:
fast/dom/HTMLImageElement/sizes/image-sizes-js-change-reverse.html
http/tests/images/loading-image-no-border.html

[EWS Watchlist]

Created attachment 377064 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Rob Buis]

Created attachment 377110 [details]
Patch

[EWS Watchlist]

Comment on attachment 377110 [details]
Patch

Attachment 377110 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12960141

New failing tests:
fast/dom/HTMLImageElement/sizes/image-sizes-js-change-reverse.html
http/tests/images/loading-image-no-border.html

[EWS Watchlist]

Created attachment 377117 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit