Bug 158466 – Add event support for link preload.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yoav Weiss]

Add event support for link preload.

[Yoav Weiss]

Created attachment 280686 [details]
Patch

[Yoav Weiss]

Created attachment 280688 [details]
Patch

[Yoav Weiss]

Created attachment 280690 [details]
Patch

[Yoav Weiss]

Created attachment 280692 [details]
Patch

[Build Bot]

Comment on attachment 280692 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[Build Bot]

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

[Build Bot]

Comment on attachment 280692 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[Build Bot]

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

[Build Bot]

Comment on attachment 280692 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[Build Bot]

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

[Build Bot]

Comment on attachment 280692 [details]
Patch

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

New failing tests:
media/video-controls-show-on-kb-or-ax-event.html
http/tests/preload/onload_event.html

[Build Bot]

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

[Yoav Weiss]

Created attachment 280703 [details]
Patch

[Build Bot]

Comment on attachment 280703 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[Build Bot]

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

[Build Bot]

Comment on attachment 280703 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[Build Bot]

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

[Build Bot]

Comment on attachment 280703 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[Build Bot]

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

[Build Bot]

Comment on attachment 280703 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[Build Bot]

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

[Yoav Weiss]

Created attachment 280788 [details]
Patch

[Build Bot]

Comment on attachment 280788 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[Build Bot]

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

[Build Bot]

Comment on attachment 280788 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[Build Bot]

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

[Build Bot]

Comment on attachment 280788 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[Build Bot]

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

[Build Bot]

Comment on attachment 280788 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[Build Bot]

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

[Yoav Weiss]

Created attachment 280795 [details]
Patch

[Build Bot]

Comment on attachment 280795 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[Build Bot]

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

[Build Bot]

Comment on attachment 280795 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[Build Bot]

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

[Yoav Weiss]

Created attachment 280799 [details]
Patch

[Yoav Weiss]

Comment on attachment 280799 [details]
Patch

All green and ready for review.

[Alex Christensen]

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.

[Yoav Weiss]

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.

[Yoav Weiss]

Created attachment 280903 [details]
Patch

[Yoav Weiss]

Addressed the review comments. Alex, can you take another look?

[Alex Christensen]

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.

[Yoav Weiss]

Created attachment 282268 [details]
Patch

[Yoav Weiss]

Created attachment 282273 [details]
Patch

[Yoav Weiss]

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

[Yoav Weiss]

Created attachment 282275 [details]
Patch

[Yoav Weiss]

Created attachment 282322 [details]
Patch

[Yoav Weiss]

Created attachment 282324 [details]
Patch

[Yoav Weiss]

Addressed all the comments and made sure the bots are happy. Alex - PTAL?

[Alex Christensen]

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?

[Yoav Weiss]

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

[Yoav Weiss]

Created attachment 282734 [details]
Patch

[Yoav Weiss]

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.

[Yoav Weiss]

Friendly ping :)

[Yoav Weiss]

Created attachment 287330 [details]
Patch

[Yoav Weiss]

Created attachment 287373 [details]
Patch

[Yoav Weiss]

Created attachment 287374 [details]
Patch

[Alex Christensen]

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)

:(

[Yoav Weiss]

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

[Yoav Weiss]

Created attachment 287549 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 287549 [details]
Patch

Clearing flags on attachment: 287549

Committed r205269: <http://trac.webkit.org/changeset/205269>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

fixed in https://trac.webkit.org/changeset/205287