We should detect delayed client-side redirects. This involves setting a timestamp in WebPageProxy::didFinishDocumentLoadForFrame() and using that timestamp to calculate the delay and pass it on in WebPageProxy::logFrameNavigation().
<rdar://problem/58125759>
Created attachment 386277 [details] Patch
Created attachment 386892 [details] Patch
The Windows test failures look unrelated. http/wpt/css/css-highlight-api/ failures and one more CSS failure.
Comment on attachment 386892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386892&action=review > Source/WebKit/UIProcess/WebPageProxy.h:2656 > + MonotonicTime m_didFinishDocumentLoadForMainFrameTimestamp; I wish we did not have to add a WebPageProxy data member for this. Could this maybe be stored somewhere else, in ITP-specific code? > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:238 > + terminationObserver = [[NSNotificationCenter defaultCenter] addObserverForName:notificationName object:nil queue:nil usingBlock:^(NSNotification *note) { }]; What's the point of this termination observer that seems to do nothing? > LayoutTests/ChangeLog:9 > + Results updated with additional data now that delayed redirects are captured. Shouldn't we add a test specifically for what you're fixing though?
(In reply to Chris Dumez from comment #5) > Comment on attachment 386892 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=386892&action=review > > > Source/WebKit/UIProcess/WebPageProxy.h:2656 > > + MonotonicTime m_didFinishDocumentLoadForMainFrameTimestamp; > > I wish we did not have to add a WebPageProxy data member for this. Could > this maybe be stored somewhere else, in ITP-specific code? The timestamp is tied to the webpage and this is in the UI process where we don't really have any ITP code except for piping and API surface. Any idea of where it could go? > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:238 > > + terminationObserver = [[NSNotificationCenter defaultCenter] addObserverForName:notificationName object:nil queue:nil usingBlock:^(NSNotification *note) { }]; > > What's the point of this termination observer that seems to do nothing? Looks like it can be removed. Maybe it was only ever there for the purposes of ITP. > > LayoutTests/ChangeLog:9 > > + Results updated with additional data now that delayed redirects are captured. > > Shouldn't we add a test specifically for what you're fixing though? Sure, I can do that.
Created attachment 387118 [details] Patch
Comment on attachment 387118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387118&action=review > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:225 > + Seconds minDelayAfterMainFrameDocumentLoadToNotBeARedirect { 5_s }; This seems really long to me.
(In reply to Chris Dumez from comment #8) > Comment on attachment 387118 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=387118&action=review > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:225 > > + Seconds minDelayAfterMainFrameDocumentLoadToNotBeARedirect { 5_s }; > > This seems really long to me. I would agree if it wasn't for real measurement (on a fast machine). Here are three samples I still had in my logs: 1.920775 s 2.165201 s 2.231257 s Thanks for the review!
Comment on attachment 387118 [details] Patch Clearing flags on attachment: 387118 Committed r254296: <https://trac.webkit.org/changeset/254296>
All reviewed patches have been landed. Closing bug.