Bug 158466 - Add event support for link preload.
Summary: Add event support for link preload.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 158720
  Show dependency treegraph
 
Reported: 2016-06-07 01:11 PDT by Yoav Weiss
Modified: 2016-09-01 03:40 PDT (History)
10 users (show)

See Also:


Attachments
Patch (57.69 KB, patch)
2016-06-07 02:57 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (57.73 KB, patch)
2016-06-07 03:35 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (58.30 KB, patch)
2016-06-07 03:55 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (58.34 KB, patch)
2016-06-07 04:12 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (955.16 KB, application/zip)
2016-06-07 05:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (793.96 KB, application/zip)
2016-06-07 05:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (666.66 KB, application/zip)
2016-06-07 05:19 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.51 MB, application/zip)
2016-06-07 05:26 PDT, Build Bot
no flags Details
Patch (58.64 KB, patch)
2016-06-07 06:22 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (804.46 KB, application/zip)
2016-06-07 07:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (918.83 KB, application/zip)
2016-06-07 07:22 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (650.27 KB, application/zip)
2016-06-07 07:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.42 MB, application/zip)
2016-06-07 07:33 PDT, Build Bot
no flags Details
Patch (58.67 KB, patch)
2016-06-08 01:36 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (809.15 KB, application/zip)
2016-06-08 02:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (858.69 KB, application/zip)
2016-06-08 02:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (669.50 KB, application/zip)
2016-06-08 02:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.43 MB, application/zip)
2016-06-08 02:52 PDT, Build Bot
no flags Details
Patch (58.81 KB, patch)
2016-06-08 04:30 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (806.64 KB, application/zip)
2016-06-08 05:29 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (816.49 KB, application/zip)
2016-06-08 05:32 PDT, Build Bot
no flags Details
Patch (59.00 KB, patch)
2016-06-08 05:37 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (59.30 KB, patch)
2016-06-09 02:14 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (59.17 KB, patch)
2016-06-28 12:43 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (59.24 KB, patch)
2016-06-28 12:56 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (59.63 KB, patch)
2016-06-28 13:18 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (59.75 KB, patch)
2016-06-28 22:34 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (59.74 KB, patch)
2016-06-28 23:41 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (63.71 KB, patch)
2016-07-04 14:29 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (63.34 KB, patch)
2016-08-29 14:31 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (69.44 KB, patch)
2016-08-29 23:34 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (64.40 KB, patch)
2016-08-29 23:54 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (64.07 KB, patch)
2016-08-31 15:11 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yoav Weiss 2016-06-07 01:11:15 PDT
Add event support for link preload.
Comment 1 Yoav Weiss 2016-06-07 02:57:59 PDT
Created attachment 280686 [details]
Patch
Comment 2 Yoav Weiss 2016-06-07 03:35:39 PDT
Created attachment 280688 [details]
Patch
Comment 3 Yoav Weiss 2016-06-07 03:55:50 PDT
Created attachment 280690 [details]
Patch
Comment 4 Yoav Weiss 2016-06-07 04:12:18 PDT
Created attachment 280692 [details]
Patch
Comment 5 Build Bot 2016-06-07 05:10:28 PDT
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
Comment 6 Build Bot 2016-06-07 05:10:33 PDT
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
Comment 7 Build Bot 2016-06-07 05:15:58 PDT
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
Comment 8 Build Bot 2016-06-07 05:16:02 PDT
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
Comment 9 Build Bot 2016-06-07 05:19:10 PDT
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
Comment 10 Build Bot 2016-06-07 05:19:14 PDT
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
Comment 11 Build Bot 2016-06-07 05:26:50 PDT
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
Comment 12 Build Bot 2016-06-07 05:26:54 PDT
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
Comment 13 Yoav Weiss 2016-06-07 06:22:04 PDT
Created attachment 280703 [details]
Patch
Comment 14 Build Bot 2016-06-07 07:21:34 PDT
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
Comment 15 Build Bot 2016-06-07 07:21:38 PDT
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
Comment 16 Build Bot 2016-06-07 07:21:57 PDT
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
Comment 17 Build Bot 2016-06-07 07:22:01 PDT
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
Comment 18 Build Bot 2016-06-07 07:31:05 PDT
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
Comment 19 Build Bot 2016-06-07 07:31:09 PDT
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
Comment 20 Build Bot 2016-06-07 07:33:34 PDT
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
Comment 21 Build Bot 2016-06-07 07:33:37 PDT
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
Comment 22 Yoav Weiss 2016-06-08 01:36:59 PDT
Created attachment 280788 [details]
Patch
Comment 23 Build Bot 2016-06-08 02:37:07 PDT
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
Comment 24 Build Bot 2016-06-08 02:37:13 PDT
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
Comment 25 Build Bot 2016-06-08 02:38:47 PDT
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
Comment 26 Build Bot 2016-06-08 02:38:53 PDT
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
Comment 27 Build Bot 2016-06-08 02:44:59 PDT
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
Comment 28 Build Bot 2016-06-08 02:45:03 PDT
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
Comment 29 Build Bot 2016-06-08 02:52:54 PDT
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
Comment 30 Build Bot 2016-06-08 02:52:59 PDT
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
Comment 31 Yoav Weiss 2016-06-08 04:30:27 PDT
Created attachment 280795 [details]
Patch
Comment 32 Build Bot 2016-06-08 05:29:41 PDT
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
Comment 33 Build Bot 2016-06-08 05:29:45 PDT
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
Comment 34 Build Bot 2016-06-08 05:32:34 PDT
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
Comment 35 Build Bot 2016-06-08 05:32:38 PDT
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 36 Yoav Weiss 2016-06-08 05:37:08 PDT
Created attachment 280799 [details]
Patch
Comment 37 Yoav Weiss 2016-06-08 06:56:59 PDT
Comment on attachment 280799 [details]
Patch

All green and ready for review.
Comment 38 Alex Christensen 2016-06-08 16:07:36 PDT
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 39 Yoav Weiss 2016-06-09 02:12:54 PDT
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 40 Yoav Weiss 2016-06-09 02:14:32 PDT
Created attachment 280903 [details]
Patch
Comment 41 Yoav Weiss 2016-06-09 22:33:47 PDT
Addressed the review comments. Alex, can you take another look?
Comment 42 Alex Christensen 2016-06-21 23:03:59 PDT
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.
Comment 43 Yoav Weiss 2016-06-28 12:43:49 PDT
Created attachment 282268 [details]
Patch
Comment 44 Yoav Weiss 2016-06-28 12:56:51 PDT
Created attachment 282273 [details]
Patch
Comment 45 Yoav Weiss 2016-06-28 13:03:32 PDT
(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.
Comment 46 Yoav Weiss 2016-06-28 13:18:42 PDT
Created attachment 282275 [details]
Patch
Comment 47 Yoav Weiss 2016-06-28 22:34:40 PDT
Created attachment 282322 [details]
Patch
Comment 48 Yoav Weiss 2016-06-28 23:41:38 PDT
Created attachment 282324 [details]
Patch
Comment 49 Yoav Weiss 2016-06-29 02:47:33 PDT
Addressed all the comments and made sure the bots are happy. Alex - PTAL?
Comment 50 Alex Christensen 2016-06-29 09:46:08 PDT
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?
Comment 51 Yoav Weiss 2016-07-01 06:21:04 PDT
(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.
Comment 52 Yoav Weiss 2016-07-04 14:29:03 PDT
Created attachment 282734 [details]
Patch
Comment 53 Yoav Weiss 2016-07-04 14:31:21 PDT
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 54 Yoav Weiss 2016-07-11 14:53:17 PDT
Friendly ping :)
Comment 55 Yoav Weiss 2016-08-29 14:31:21 PDT
Created attachment 287330 [details]
Patch
Comment 56 Yoav Weiss 2016-08-29 23:34:49 PDT
Created attachment 287373 [details]
Patch
Comment 57 Yoav Weiss 2016-08-29 23:54:17 PDT
Created attachment 287374 [details]
Patch
Comment 58 Alex Christensen 2016-08-31 14:44:20 PDT
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)

:(
Comment 59 Yoav Weiss 2016-08-31 15:08:35 PDT
(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.
Comment 60 Yoav Weiss 2016-08-31 15:11:14 PDT
Created attachment 287549 [details]
Patch
Comment 61 WebKit Commit Bot 2016-08-31 15:43:37 PDT
Comment on attachment 287549 [details]
Patch

Clearing flags on attachment: 287549

Committed r205269: <http://trac.webkit.org/changeset/205269>
Comment 62 WebKit Commit Bot 2016-08-31 15:43:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 63 Csaba Osztrogonác 2016-09-01 03:23:57 PDT
(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
Comment 64 Csaba Osztrogonác 2016-09-01 03:40:40 PDT
fixed in https://trac.webkit.org/changeset/205287