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
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.
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
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
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
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
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 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.
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
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 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.
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
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
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.
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
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
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
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
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
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
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
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 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 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 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);
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)
(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!
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
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
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
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
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
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 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.
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
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
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
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
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
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
2014-07-01 04:26 PDT, Yoav Weiss
2014-07-01 05:40 PDT, Build Bot
2014-07-01 06:20 PDT, Build Bot
2014-07-01 06:32 PDT, Build Bot
2014-10-01 06:32 PDT, Yoav Weiss
2014-10-01 13:23 PDT, Yoav Weiss
2014-10-01 15:34 PDT, Build Bot
2014-10-01 15:56 PDT, Build Bot
2014-10-06 03:06 PDT, Yoav Weiss
2014-10-06 03:57 PDT, Yoav Weiss
2014-10-06 04:09 PDT, Yoav Weiss
2014-10-06 05:21 PDT, Build Bot
2014-10-06 06:02 PDT, Build Bot
2015-03-08 19:24 PDT, Yoav Weiss
2015-03-08 20:05 PDT, Build Bot
2015-03-08 20:08 PDT, Build Bot
2015-03-25 03:47 PDT, Yoav Weiss
2015-03-25 06:26 PDT, Yoav Weiss
2015-03-25 07:08 PDT, Build Bot
2015-03-25 07:12 PDT, Build Bot
2015-03-27 01:53 PDT, Yoav Weiss
2015-03-27 02:42 PDT, Build Bot
2015-03-27 02:49 PDT, Build Bot
2015-03-30 01:20 PDT, Yoav Weiss
2015-03-30 02:04 PDT, Build Bot
2015-03-30 02:22 PDT, Build Bot
2015-03-30 05:21 PDT, Yoav Weiss
2015-03-30 05:30 PDT, Yoav Weiss
2015-03-30 06:08 PDT, Build Bot
2015-03-30 06:11 PDT, Build Bot
2015-03-30 12:25 PDT, Yoav Weiss
2015-04-01 00:56 PDT, Yoav Weiss
2015-12-04 00:18 PST, Yoav Weiss
2015-12-04 00:50 PST, Build Bot
2015-12-04 00:52 PST, Build Bot
2015-12-04 00:54 PST, Build Bot
2015-12-04 02:56 PST, Yoav Weiss
2015-12-04 03:42 PST, Build Bot
2015-12-04 03:45 PST, Build Bot
2015-12-04 04:02 PST, Build Bot
2019-08-21 00:14 PDT, Rob Buis
2019-08-21 02:29 PDT, EWS Watchlist
2019-08-21 05:26 PDT, Rob Buis
2019-08-21 07:25 PDT, EWS Watchlist
2019-08-21 11:04 PDT, Rob Buis
2019-08-21 13:22 PDT, EWS Watchlist
2019-08-21 23:52 PDT, Rob Buis
2019-08-22 01:26 PDT, EWS Watchlist
2019-08-22 11:08 PDT, Rob Buis
2019-08-22 13:01 PDT, Rob Buis
2019-08-22 16:03 PDT, EWS Watchlist
2019-08-23 01:01 PDT, Rob Buis
2019-08-23 02:20 PDT, EWS Watchlist