Created attachment 280696[details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 280697[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 280699[details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Created attachment 280700[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 280704[details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 280705[details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 280706[details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Created attachment 280707[details]
Archive of layout-test-results from ews117 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 280790[details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 280791[details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 280792[details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Created attachment 280793[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 280797[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 280798[details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 280799[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=280799&action=review> Source/WebCore/loader/LinkPreloadResourceClients.cpp:2
> + * Copyright 2016 The Chromium Authors. All rights reserved.
Did the chromium authors write this? Is there a corresponding crbug?
> Source/WebCore/loader/LinkPreloadResourceClients.h:28
> +#ifndef LinkPreloadResourceClients_h
> +#define LinkPreloadResourceClients_h
#pragma once
> Source/WebCore/loader/LinkPreloadResourceClients.h:81
> + return new LinkPreloadScriptResourceClient(loader, resource);
This is a memory leak. There's no corresponding delete. Instead of Optional, I think we should just use std::unique_ptr
> Source/WebCore/loader/cache/CachedResourceLoader.cpp:1035
> #if !PLATFORM(IOS)
I think we should consider making iOS loading work more like everything else. Probably not in this patch, though.
> Source/WebCore/platform/network/ResourceRequestBase.h:152
> + bool ignoreForRequestCounting() const { return m_ignoreForRequestCounting; }
> + void setIgnoreForRequestCounting(bool ignoreForRequestCounting) { m_ignoreForRequestCounting = ignoreForRequestCounting; }
shouldIgnoreForeRequestCounting and setShouldIgnoreForRequestCounting might be better names. Or maybe ignoreForRequestCount to match CachedResource.
Comment on attachment 280799[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=280799&action=review>> Source/WebCore/loader/LinkPreloadResourceClients.cpp:2
>> + * Copyright 2016 The Chromium Authors. All rights reserved.
>
> Did the chromium authors write this? Is there a corresponding crbug?
The crbug is https://bugs.chromium.org/p/chromium/issues/detail?id=552289
I added these files to the Chromium project a few months back. I consulted other Chromium developers for the design.
>> Source/WebCore/loader/LinkPreloadResourceClients.h:28
>> +#define LinkPreloadResourceClients_h
>
> #pragma once
will change
>> Source/WebCore/loader/LinkPreloadResourceClients.h:81
>> + return new LinkPreloadScriptResourceClient(loader, resource);
>
> This is a memory leak. There's no corresponding delete. Instead of Optional, I think we should just use std::unique_ptr
Thanks for catching this. I'll change it to use unique_ptr, and add a LinkLoader member that keeps that reference around.
>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:1035
>> #if !PLATFORM(IOS)
>
> I think we should consider making iOS loading work more like everything else. Probably not in this patch, though.
I agree. I think we're using the OS here as a signal for network conditions, where we could most probably address those directly.
>> Source/WebCore/platform/network/ResourceRequestBase.h:152
>> + void setIgnoreForRequestCounting(bool ignoreForRequestCounting) { m_ignoreForRequestCounting = ignoreForRequestCounting; }
>
> shouldIgnoreForeRequestCounting and setShouldIgnoreForRequestCounting might be better names. Or maybe ignoreForRequestCount to match CachedResource.
will change to ignoreForRequestCount for better consistency.
Comment on attachment 280903[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=280903&action=review> Source/WebCore/loader/LinkLoader.cpp:108
> + ASSERT(loader);
make loader a LinkLoader& and call it with *this.
> Source/WebCore/loader/LinkLoader.cpp:110
> + if (!resource)
> + return std::unique_ptr<LinkPreloadResourceClient>(nullptr);
I think resource should be a CachedResource&. As it stands right now, this can never be hit because we return early if resource is null in the one callsite.
> Source/WebCore/loader/LinkLoader.cpp:126
> + default:
I think it would be better to list all the CachedResource types so we don't forget to update this if we change the enum, and so we can verify that we're doing the right thing in all cases.
> Source/WebCore/loader/LinkLoader.cpp:127
> + ASSERT_NOT_REACHED();
It would be nice to have a comment explaining why this cannot be reached, possibly with a link to the relevant part of the spec. I glance at this and wonder why there are only 7 of the 14 cases handled. Is this from section 2.2.1 of the spec? I see as=foobarxmlthing in the test, but what about as=somethingvalidbutunexpected?
> Source/WebCore/loader/LinkLoader.cpp:128
> + return std::unique_ptr<LinkPreloadResourceClient>(nullptr);
ditto. Just nullptr.
> Source/WebCore/loader/LinkLoader.h:67
> + std::unique_ptr<LinkPreloadResourceClient> m_preloadResourceClient;
So this is just a CachedResourceClient that doesn't do anything, right? It just puts the resource in the cache and that's it. Makes sense. Will the result always be cached?
> Source/WebCore/loader/LinkPreloadResourceClients.h:51
> + virtual void clear() = 0;
Why is this purely virtual? All the implementations do the exact same thing. This feels excessive unless you plan to do something different in any of the clients.
(In reply to comment #42)
> Comment on attachment 280903[details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280903&action=review
>
> > Source/WebCore/loader/LinkLoader.cpp:108
> > + ASSERT(loader);
>
> make loader a LinkLoader& and call it with *this.
done
>
> > Source/WebCore/loader/LinkLoader.cpp:110
> > + if (!resource)
> > + return std::unique_ptr<LinkPreloadResourceClient>(nullptr);
>
> I think resource should be a CachedResource&. As it stands right now, this
> can never be hit because we return early if resource is null in the one
> callsite.
done
>
> > Source/WebCore/loader/LinkLoader.cpp:126
> > + default:
>
> I think it would be better to list all the CachedResource types so we don't
> forget to update this if we change the enum, and so we can verify that we're
> doing the right thing in all cases.
done
>
> > Source/WebCore/loader/LinkLoader.cpp:127
> > + ASSERT_NOT_REACHED();
>
> It would be nice to have a comment explaining why this cannot be reached,
> possibly with a link to the relevant part of the spec. I glance at this and
> wonder why there are only 7 of the 14 cases handled. Is this from section
> 2.2.1 of the spec? I see as=foobarxmlthing in the test, but what about
> as=somethingvalidbutunexpected?
I'll add one
>
> > Source/WebCore/loader/LinkLoader.cpp:128
> > + return std::unique_ptr<LinkPreloadResourceClient>(nullptr);
>
> ditto. Just nullptr.
done
>
> > Source/WebCore/loader/LinkLoader.h:67
> > + std::unique_ptr<LinkPreloadResourceClient> m_preloadResourceClient;
>
> So this is just a CachedResourceClient that doesn't do anything, right? It
> just puts the resource in the cache and that's it. Makes sense. Will the
> result always be cached?
Yeah, that's just here to keep the preloadResourceClient alive, in order to get onload/onerror events. The Resource will always go in the memcache.
>
> > Source/WebCore/loader/LinkPreloadResourceClients.h:51
> > + virtual void clear() = 0;
>
> Why is this purely virtual? All the implementations do the exact same
> thing. This feels excessive unless you plan to do something different in
> any of the clients.
That's a pure virtual because clearResource() is only defined for descendants of CachedResourceClient, not for LinkPreloadResourceClient. So, I didn't find a way to centralize the implementation of that function.
(In reply to comment #50)
> Comment on attachment 282324[details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282324&action=review
>
> > Source/WebCore/loader/LinkLoader.cpp:70
> > + m_client.linkLoadingErrored();
>
> We called these on a 0-delay timer before, but now they are called
> synchronously. Why the change?
So, I thought that we're calling two separate timers here (mistaking dispatchEvent() calls in HTMLLinkElement to EventSender::dispatchEventSoon()), and thought I'd remove one. However, that's not the case, and sync calling the load event here is the wrong behavior.
The reason I stumbled upon this to begin with is the failure of delaying_onload_link_preload_after_discovery_image.html
Digging further it seems that the reason for the failure is that DOMWindow's load event is called synchronously while resource onload events are called in an async way. However, I believe <img onload> is called asynchronously, yet is called before window.onload (test case: http://test.weissyoav.com/tests/onload.html)
I need to analyze this some more and figure out why is the <link> behavior here different than <img>. Then I need to make sure <link onload> is called after window.onload, while keeping its async nature.
Moving HTMLLinkElement's events to use EventSender rather than simply trigger the events using timers made sure the event is triggered before the window's onload.
Comment on attachment 287374[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=287374&action=review> Source/WebCore/loader/LinkPreloadResourceClients.h:83
> + virtual String debugName() const { return "LinkPreloadScript"; }
ASCIILiteral for all of these. Are they useful? Maybe they could be removed.
> Source/WebCore/loader/cache/CachedResourceLoader.cpp:1065
> #if !PLATFORM(IOS)
:(
(In reply to comment #58)
> Comment on attachment 287374[details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287374&action=review
>
> > Source/WebCore/loader/LinkPreloadResourceClients.h:83
> > + virtual String debugName() const { return "LinkPreloadScript"; }
>
> ASCIILiteral for all of these. Are they useful? Maybe they could be
> removed.
Yeah, I'll just remove them
>
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:1065
> > #if !PLATFORM(IOS)
>
> :(
I share the :( regarding this heuristic being platform dependent rather than network dependent.
(In reply to comment #61)
> Comment on attachment 287549[details]
> Patch
>
> Clearing flags on attachment: 287549
>
> Committed r205269: <http://trac.webkit.org/changeset/205269>
It broke the !ENABLE(SVG_FONTS) and !ENABLE(XSLT) build too:
../../Source/WebCore/loader/LinkLoader.cpp:125:10: error: 'SVGFontResource' is not a member of 'WebCore::CachedResource'
--> SVGFontResource is guarded with ENABLE(SVG_FONTS) in CachedResource.h
../../Source/WebCore/loader/LinkLoader.cpp:127:10: error: 'XSLStyleSheet' is not a member of 'WebCore::CachedResource'
--> XSLStyleSheet is guarded with ENABLE(XSLT) in CachedResource.h
2016-06-07 02:57 PDT, Yoav Weiss
2016-06-07 03:35 PDT, Yoav Weiss
2016-06-07 03:55 PDT, Yoav Weiss
2016-06-07 04:12 PDT, Yoav Weiss
2016-06-07 05:10 PDT, Build Bot
2016-06-07 05:16 PDT, Build Bot
2016-06-07 05:19 PDT, Build Bot
2016-06-07 05:26 PDT, Build Bot
2016-06-07 06:22 PDT, Yoav Weiss
2016-06-07 07:21 PDT, Build Bot
2016-06-07 07:22 PDT, Build Bot
2016-06-07 07:31 PDT, Build Bot
2016-06-07 07:33 PDT, Build Bot
2016-06-08 01:36 PDT, Yoav Weiss
2016-06-08 02:37 PDT, Build Bot
2016-06-08 02:38 PDT, Build Bot
2016-06-08 02:45 PDT, Build Bot
2016-06-08 02:52 PDT, Build Bot
2016-06-08 04:30 PDT, Yoav Weiss
2016-06-08 05:29 PDT, Build Bot
2016-06-08 05:32 PDT, Build Bot
2016-06-08 05:37 PDT, Yoav Weiss
2016-06-09 02:14 PDT, Yoav Weiss
2016-06-28 12:43 PDT, Yoav Weiss
2016-06-28 12:56 PDT, Yoav Weiss
2016-06-28 13:18 PDT, Yoav Weiss
2016-06-28 22:34 PDT, Yoav Weiss
2016-06-28 23:41 PDT, Yoav Weiss
2016-07-04 14:29 PDT, Yoav Weiss
2016-08-29 14:31 PDT, Yoav Weiss
2016-08-29 23:34 PDT, Yoav Weiss
2016-08-29 23:54 PDT, Yoav Weiss
2016-08-31 15:11 PDT, Yoav Weiss