Bug 179359 - Web Inspector: Add some fast returns in cases where we only call through to a NetworkAgent
Summary: Web Inspector: Add some fast returns in cases where we only call through to a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-06 19:36 PST by Joseph Pecoraro
Modified: 2017-11-15 12:10 PST (History)
4 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (5.83 KB, patch)
2017-11-06 19:37 PST, Joseph Pecoraro
drousso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-11-06 19:36:39 PST
Add some fast returns in cases where we only call through to a NetworkAgent

The NetworkAgent is only enabled when a frontend connects, so we don't need to do work for these unless a frontend is connected.
Comment 1 Joseph Pecoraro 2017-11-06 19:37:56 PST
Created attachment 326184 [details]
[PATCH] Proposed Fix
Comment 2 Devin Rousso 2017-11-07 01:52:18 PST
Comment on attachment 326184 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=326184&action=review

r=me.

Should you also add this to these instrumentation points as well?
 - didReceiveThreadableLoaderResponse
 - didReceiveData

It seems like the logic we had could've theoretically supported saving network information when there was no frontend, but it would've required some changes to InspectorNetworkAgent::enable.  Is the main reason that we don't want to do this performance, or am I missing something?

> Source/WebCore/ChangeLog:14
> +        (WebCore::InspectorInstrumentation::continueAfterXFrameOptionsDenied):
> +        (WebCore::InspectorInstrumentation::continueWithPolicyDownload):

These are not actually modified.  Should they be?  They call into InspectorInstrumentation::didReceiveResourceResponseImpl, which does call to InspectorConsoleAgent.  I am unfamiliar with how that agent functions, so I can't say as to whether it relies on data from before a frontend exists or not.
Comment 3 Joseph Pecoraro 2017-11-07 11:06:57 PST
(In reply to Devin Rousso from comment #2)
> Comment on attachment 326184 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326184&action=review
> 
> r=me.
> 
> Should you also add this to these instrumentation points as well?
>  - didReceiveThreadableLoaderResponse
>  - didReceiveData

Yes! Thanks for catching these, I had multiple changes in my tree and forgot to git-add these.

> It seems like the logic we had could've theoretically supported saving
> network information when there was no frontend, but it would've required
> some changes to InspectorNetworkAgent::enable.  Is the main reason that we
> don't want to do this performance, or am I missing something?

Yes, performance, since this code is inlined it should make instrumentation hooks more lightweight.

> > Source/WebCore/ChangeLog:14
> > +        (WebCore::InspectorInstrumentation::continueAfterXFrameOptionsDenied):
> > +        (WebCore::InspectorInstrumentation::continueWithPolicyDownload):
> 
> These are not actually modified.  Should they be?  They call into
> InspectorInstrumentation::didReceiveResourceResponseImpl, which does call to
> InspectorConsoleAgent.  I am unfamiliar with how that agent functions, so I
> can't say as to whether it relies on data from before a frontend exists or
> not.

Again, multiple changes in my tree probably.
Comment 4 Joseph Pecoraro 2017-11-07 12:01:43 PST
<https://trac.webkit.org/r224540>
Comment 5 Radar WebKit Bug Importer 2017-11-15 12:10:36 PST
<rdar://problem/35567075>