Bug 26046 – Limit the number of resources and console messages cached by the InspectorController

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

When InspectorController is enabled, it is caching resources metadata (HTTP headers), XmlHttp response contents and console messages with no obvious limits. This affects browser performance, especially when browser is pointing to some XMLHttpRequest-intensive site over night.

I would like to change InspectorController in a following manner:

1. Do not cache XMLHttpRequests response string when Web Inspector window is not opened.
2. Limit the number of cached console messages to 200.
3. Limit the number of cached resource HTTP headers to 200, expire them after.

Timothy, are you open to such a change? If yes, I will send out a patch shortly!

[Timothy Hatcher]

I am fine adding limits, but I think "Do not cache XMLHttpRequests response string when Web Inspector window is not opened" will lead to confusion and bug reports.

"Why isn't my XHR data there? Screw this! I'm going back to Firebug."

We do this for HTML errors, only report them when the Inspector is open during parse time, and it is confusing. I would like to change that in the future.

[Timothy Hatcher]

Maybe we need UI to indicate that things are missing. So when headers and console messages are purged there is some UI in the Console as the first message saying "123 messages have been discarded since they exceded the 200 message limit."  Or something better worded. And something similar where HTTP headers are normally saying something like "headers were discarded, reload the page to see them". 

[Pavel Feldman]

Capturing our discussion with Timothy over IRC:

Users don't like things disappearing from their resources panel / console. What we could do instead is to introduce the 'Enable resource inspection' splash screen for the Resources tab. However, users might be annoyed with the need of enabling it all the time. 

To mitigate this, each splash (resources, scripts, profiles) is going to get two options on how it is being enabled: 'for this session only' or 'always'.

Unchecking the 'enabled' checkbox in the bottom of each page will reset the 'always' state for the option.

[Timothy Hatcher]

Created attachment 30717 [details]
Mock-up

[Alexey Proskuryakov]

See also: bug 17534.

Introducing a persistent state doesn't sound great, because it's likely that developers with inspection enabled will get worse performance and potentially even different behavior than normal users (and it's easy to forget about changing the state in the past). Who wants to tell pundits that they need to change Inspector options to confirm others' benchmark results?

[Pavel Feldman]

There are couple of consideration here:
1) InspectorController cab only be explicitly enabled via Advanced preferences tab.
2) We are also considering chrome client callback that will visually identify the fact that page is in 'slow' mode.

Don't get me wrong, I am open to the suggestions / options. Let me iterate over the thinking once again here to give you a broader context.

When Develop menu is enabled, InspectorController starts caching different things (console messages, xml http request results). It is hardly surviving 24 hours of gmail. We see two ways of fixing this:
1) expire cache if WebInspector window is not opened 
2) Explicitly enable caching when Resources tab is activated (just as it is with Scripts and Profiler).

Since #1 has disadvantages that Timothy is pointing out, we are choosing #2. Now #2 leads to a usability issue where user needs to explicitly enable the tab over and over. We mitigate it with sticky global options that obviously suffer from the cons that you are bringing up. We are trying to mitigate this with chrome UI sign and with stressing that this option is persistent when user is first enabling it.

Thoughts?

[Alexey Proskuryakov]

This sounds like a good topic for webkit-dev mailing list.

> We are trying to mitigate this with chrome UI sign and with 
> stressing that this option is persistent when user is
> first enabling it.

I think that such measures are inefficient in general, but that's not a universally supported view, of course. Thinking of it, I never remember whether I have Firebug enabled in Firefox!

[Pavel Feldman]

Created attachment 30750 [details]
patch preview

This is a patch preview to get some early comments. Things that are missing:

1) Splash image for resources tab
2) Radio button style for 'checked' state
3) Automatic page refresh upon Resources tab activation.

[Timothy Hatcher]

Comment on attachment 30750 [details]
patch preview

Looking good!

> -    ASSERT(m_frontend);
> +    ASSERT(m_frontend.get());

You shoudln't need that change.

>      if (!m_frontend.get())
>          return;

No need to call .get().

> +    if (m_frontend.get())
> +        m_frontend->resourceTrackingWasEnabled();

No need to call .get().

> +    if (m_frontend.get())
> +        m_frontend->resourceTrackingWasDisabled();

No need to call .get().


> +    var enableOption = function(text, checked) {

I prefer function enableOption() instead.

> +        var caption = document.createElement("text");
> +        caption.textContent = text;
> +        caption.addEventListener("mousedown", function() { option.checked = true; }, false);
> +        li.appendChild(caption);

You should do: <label><input type="radio"> text</label>, then you wont need the event listener.

> +    alwaysWasChosen: function() {
> +        return this.enabledAlways.checked;
>      }

Maybe a getter for this? get alwaysEnabled()?

> +    _toggleProfiling: function(opt_always)
> +    _toggleResourceTracking: function(opt_always)
> +    _toggleDebugging: function(opt_always)

These variables should not have an underscore and be camelcase, and no abbrivation.


> +    text-align: center;
> +    margin-left: 20px;

What was this change for?


> +.panel-enabler-view text {

An element named text isn't good, just use a span. But this wont be needed if you use by label suggestion.

> +	color: rgb(6, 6, 6);

Tab here.

> +.panel-enabler-view.resources img {
> +    content: url(Images/scriptsSilhouette.png);
> +}

I'll make a new resource silhouette soon, but we could land like this.

I'll attach the radio button dot soon. You can use multiple background show the dot.

[Timothy Hatcher]

Created attachment 30761 [details]
Radio Button Dot

[Pavel Feldman]

(In reply to comment #9)
> (From update of attachment 30750 [details] [review])
> Looking good!
> 
> > -    ASSERT(m_frontend);
> > +    ASSERT(m_frontend.get());
> 
> You shoudln't need that change.
>

Done
 
> >      if (!m_frontend.get())
> >          return;
> 
> No need to call .get().
> 
> > +    if (m_frontend.get())
> > +        m_frontend->resourceTrackingWasEnabled();
> 
> No need to call .get().
> 
> > +    if (m_frontend.get())
> > +        m_frontend->resourceTrackingWasDisabled();
> 
> No need to call .get().
>

Done all
 
> 
> > +    var enableOption = function(text, checked) {
> 
> I prefer function enableOption() instead.
>

Done
 
> > +        var caption = document.createElement("text");
> > +        caption.textContent = text;
> > +        caption.addEventListener("mousedown", function() { option.checked = true; }, false);
> > +        li.appendChild(caption);
> 
> You should do: <label><input type="radio"> text</label>, then you wont need the
> event listener.
> 

Done

> > +    alwaysWasChosen: function() {
> > +        return this.enabledAlways.checked;
> >      }
> 
> Maybe a getter for this? get alwaysEnabled()?
>


Done
 
> > +    _toggleProfiling: function(opt_always)
> > +    _toggleResourceTracking: function(opt_always)
> > +    _toggleDebugging: function(opt_always)
> 
> These variables should not have an underscore and be camelcase, and no
> abbrivation.
>

Done
 
> 
> > +    text-align: center;
> > +    margin-left: 20px;
> 
> What was this change for?
> 
>

This was to align disclaimer with the radio buttons. 'text-align' was not required, removed.
 
> > +.panel-enabler-view text {
> 
> An element named text isn't good, just use a span. But this wont be needed if
> you use by label suggestion.
> 

Done

> > +	color: rgb(6, 6, 6);
> 
> Tab here.
> 
> > +.panel-enabler-view.resources img {
> > +    content: url(Images/scriptsSilhouette.png);
> > +}
> 
> I'll make a new resource silhouette soon, but we could land like this.
> 
> I'll attach the radio button dot soon. You can use multiple background show the
> dot.
>

Thanks!

[Pavel Feldman]

Created attachment 30787 [details]
patch (needs radioDot.png to be added manually).

This one contains fixes for all the comments. Note that radioDot.png needs to be manually added into the patch when landing.

The only thing that is missing is when enabling resources panel it does not do auto-reload to populate itself. I'd like to ask Timothy's advice on this and maybe submit auto-reload as a separate patch.

[Pavel Feldman]

(In reply to comment #7)
> This sounds like a good topic for webkit-dev mailing list.
> 
> > We are trying to mitigate this with chrome UI sign and with 
> > stressing that this option is persistent when user is
> > first enabling it.
> 
> I think that such measures are inefficient in general, but that's not a
> universally supported view, of course. Thinking of it, I never remember whether
> I have Firebug enabled in Firefox!
> 

I think I mitigated this via following: In case of 'Enabled always' for Debugger and Profiler, they are only being enabled when user is opening Web Inspector UI. 'Enable always' for Resources will make InspectorController cache data and produce overhead prior to opening of WebInspector, but that is what is happening today anyway, without user knowing about it. So it seems that the change is only for good!

[Pavel Feldman]

Created attachment 30790 [details]
screenshot

[Pavel Feldman]

Created attachment 30806 [details]
patch

This addresses your comments:
- introduces refresh on resources tab enabling
- hides status bar buttons when resources panel is not enabled
- fixes the issue with tabs being disabled on WebInspector closing
- is binary, so should have png image in it

[Timothy Hatcher]

Comment on attachment 30806 [details]
patch

Looks great! Just one minor issue and a question.

> -    margin-left: 50px;
> -    margin-bottom: 6px;
> -    line-height: 18px;
> -    word-break: break-word;
> +   margin: 0 0 5px 20px;

There is a tab here. Also why remove word-break?

r+ if you fix the tab.

[Pavel Feldman]

Created attachment 30809 [details]
patch

Indent fixed (was not a tab). word-wrap restored. 

it is a style for '.panel-enabler-view label' that actually was not used in the component, was cleaning it up when introducing the usage.

[Timothy Hatcher]

Comment on attachment 30809 [details]
patch

Nice job!

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/44322.