Created attachment 296686[details]
Archive of layout-test-results from ews115 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 296690[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 296694[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.6
Created attachment 296696[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
Attachment 296844[details] did not pass style-queue:
ERROR: LayoutTests/ChangeLog:8: Line contains tab character. [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:9: Line contains tab character. [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:11: Line contains tab character. [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:12: Line contains tab character. [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:13: Line contains tab character. [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:14: Line contains tab character. [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:15: Line contains tab character. [whitespace/tab] [5]
Total errors found: 7 in 21 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 296849[details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 296850[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 296851[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
Created attachment 296852[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.6
Comment on attachment 304495[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=304495&action=review> Source/WebCore/loader/cache/CachedResourceClient.h:48
> + virtual bool markAsReferenced() const { return true; }
I'm not sure this is named correctly. shouldMarkAsReferenced? Isn't there already a way to tell if a load is a preload?
> LayoutTests/TestExpectations:1260
> +http/tests/preload/single_download_preload.html [ DumpJSConsoleLogInStdErr ]
Does this make these tests not accidentally print out a warning message? Is there no way to do this from within the test?
(In reply to comment #23)
> Comment on attachment 304495[details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304495&action=review
>
> > Source/WebCore/loader/cache/CachedResourceClient.h:48
> > + virtual bool markAsReferenced() const { return true; }
>
> I'm not sure this is named correctly. shouldMarkAsReferenced?
Yeah, I'll rename
> Isn't there already a way to tell if a load is a preload?
There is already a way to tell if a certain request is a preload request or a preloaded resource.
However, a preloaded resource can be referenced (e.g. `<link rel=preload href=foo as=script><script src=foo>`).
Current tracking if a resource is referenced is based on the addition of CachedResourceClients to it. So I figured it'd be cleanest to add that "is this referencing" info to the resource client.
>
> > LayoutTests/TestExpectations:1260
> > +http/tests/preload/single_download_preload.html [ DumpJSConsoleLogInStdErr ]
>
> Does this make these tests not accidentally print out a warning message?
Yeah
> Is there no way to do this from within the test?
Don't know. If there is, I'd be happy to switch to that method
Created attachment 304559[details]
Archive of layout-test-results from ews116 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(In reply to comment #24)
> (In reply to comment #23)
> > Comment on attachment 304495[details]
> > Patch
> >
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=304495&action=review
> >
> > > Source/WebCore/loader/cache/CachedResourceClient.h:48
> > > + virtual bool markAsReferenced() const { return true; }
> >
> > I'm not sure this is named correctly. shouldMarkAsReferenced?
>
> Yeah, I'll rename
>
> > Isn't there already a way to tell if a load is a preload?
>
> There is already a way to tell if a certain request is a preload request or
> a preloaded resource.
> However, a preloaded resource can be referenced (e.g. `<link rel=preload
> href=foo as=script><script src=foo>`).
> Current tracking if a resource is referenced is based on the addition of
> CachedResourceClients to it. So I figured it'd be cleanest to add that "is
> this referencing" info to the resource client.
>
> >
> > > LayoutTests/TestExpectations:1260
> > > +http/tests/preload/single_download_preload.html [ DumpJSConsoleLogInStdErr ]
> >
> > Does this make these tests not accidentally print out a warning message?
>
> Yeah
>
> > Is there no way to do this from within the test?
>
> Don't know. If there is, I'd be happy to switch to that method
Hmm, the debug bot failure seems to indicate that it's not taking this directive into account. Is that a known issue? Is there a way to get around that?
I don't know much about DumpJSConsoleLogInStdErr, but I think it would be cleaner to add something to TestRunner.idl or Internals.idl to disable the logs from within the test rather than having to know about this magic value in TestExpectations to figure out what is going on.
(In reply to Alex Christensen from comment #29)
> I don't know much about DumpJSConsoleLogInStdErr, but I think it would be
> cleaner to add something to TestRunner.idl or Internals.idl to disable the
> logs from within the test rather than having to know about this magic value
> in TestExpectations to figure out what is going on.
DumpJSConsoleLogInStdErr is mostly for imported W3C tests.
In most cases, if you can tweak the test, you can fix the flakiness issues, which makes TestRunner method not necessary, although we can add it for convenience.
If we were able to inject WebKit specific scripts on imported W3C tests without modifying them (like testXX.webkit.js file next to textXX.js), we could probably get rid of DumpJSConsoleLogInStdErr and use TestRunner.
> Hmm, the debug bot failure seems to indicate that it's not taking this
> directive into account. Is that a known issue? Is there a way to get around
> that?
It is not known, can you reproduce it locally?
(In reply to youenn fablet from comment #31)
> > Hmm, the debug bot failure seems to indicate that it's not taking this
> > directive into account. Is that a known issue? Is there a way to get around
> > that?
>
> It is not known, can you reproduce it locally?
I am not seeing DumpJSConsoleLogInStdErr in the patch or in TestExpectations though.
I removed DumpJSConsoleLogInStdErr and modified the tests to be more resilient, as well as stopped the timer when the document is detached.
Unfortunately, I'm currently failing to upload a patch... I'll try to upload it again tomorrow.
Created attachment 305271[details]
Archive of layout-test-results from ews102 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 305272[details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 305273[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=305273&action=review> Source/WebCore/page/DOMWindow.h:390
> + Timer m_unusedPreloadsTimer;
Why not having this timer in CachedResourceLoader and letting DOMWindow/Document notify the cached resource loader when to start/stop its timer?
(I still think the preload management logic should be somehow isolated to ease understanding and future evolution of this mechanism)
(In reply to youenn fablet from comment #41)
> Comment on attachment 305273[details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305273&action=review
>
> > Source/WebCore/page/DOMWindow.h:390
> > + Timer m_unusedPreloadsTimer;
>
> Why not having this timer in CachedResourceLoader and letting
> DOMWindow/Document notify the cached resource loader when to start/stop its
> timer?
I'll move it if you think it's cleaner.
(In reply to Yoav Weiss from comment #42)
> (In reply to youenn fablet from comment #41)
> > Comment on attachment 305273[details]
> > Patch
> >
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=305273&action=review
> >
> > > Source/WebCore/page/DOMWindow.h:390
> > > + Timer m_unusedPreloadsTimer;
> >
> > Why not having this timer in CachedResourceLoader and letting
> > DOMWindow/Document notify the cached resource loader when to start/stop its
> > timer?
>
> I'll move it if you think it's cleaner.
I think it will avoid leaking some CachedResourceLoader logic in DOMWindow.
Please let me know if there is something worse when moving the logic.
(In reply to youenn fablet from comment #44)
> (In reply to Yoav Weiss from comment #42)
> > (In reply to youenn fablet from comment #41)
> > > Comment on attachment 305273[details]
> > > Patch
> > >
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=305273&action=review
> > >
> > > > Source/WebCore/page/DOMWindow.h:390
> > > > + Timer m_unusedPreloadsTimer;
> > >
> > > Why not having this timer in CachedResourceLoader and letting
> > > DOMWindow/Document notify the cached resource loader when to start/stop its
> > > timer?
> >
> > I'll move it if you think it's cleaner.
>
> I think it will avoid leaking some CachedResourceLoader logic in DOMWindow.
> Please let me know if there is something worse when moving the logic.
Much cleaner now with the timer moved. Good call! :)
Comment on attachment 305286[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=305286&action=review>> Source/WebCore/dom/Document.cpp:2263
>> + m_cachedResourceLoader->stopUnusedPreloadsTimer();
>
> Youenn - can you just confirm that I can assume m_cachedResourceLoader here can never be nullptr? I'm almost sure that's the case...
This is fine.
Created attachment 305295[details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Uploaded a new version of the patch with improved tests, and which is also clearing the ResourceTiming buffer when the document is detached in order to reduce test flakiness.
PTAL?
Comment on attachment 305493[details]
Patch
Rejecting attachment 305493[details] from commit-queue.
Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 305493, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit
Last 500 characters of output:
21bf338bedb2f38ccec2141744769e2e7b 8d9ae72ccc5ab5bf039c78da119722bab7f1ddb0 M Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.
Total errors found: 0 in 1 files
Full output: http://webkit-queues.webkit.org/results/3425805
2016-12-09 12:35 PST, Yoav Weiss
2016-12-09 13:28 PST, Build Bot
2016-12-09 13:44 PST, Build Bot
2016-12-09 13:58 PST, Build Bot
2016-12-09 14:03 PST, Build Bot
2016-12-10 20:19 PST, Yoav Weiss
2016-12-10 20:35 PST, Yoav Weiss
2016-12-10 21:30 PST, Build Bot
2016-12-10 21:42 PST, Build Bot
2016-12-10 21:48 PST, Build Bot
2016-12-10 21:59 PST, Build Bot
2017-03-15 05:36 PDT, Yoav Weiss
2017-03-15 05:53 PDT, Yoav Weiss
2017-03-15 14:07 PDT, Yoav Weiss
2017-03-15 15:19 PDT, Build Bot
2017-03-23 23:26 PDT, Yoav Weiss
2017-03-24 01:12 PDT, Build Bot
2017-03-24 01:18 PDT, Build Bot
2017-03-24 02:48 PDT, Yoav Weiss
2017-03-24 09:16 PDT, Yoav Weiss
2017-03-24 10:35 PDT, Build Bot
2017-03-27 12:07 PDT, Yoav Weiss
2017-03-28 02:28 PDT, Yoav Weiss