Bug 64097 – Web Inspector: Add support for disabling cache in web inspector.

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

[Vsevolod Vlasov]

Add support for disabling cache in web inspector.

[Vsevolod Vlasov]

Created attachment 100116 [details]
Patch

[WebKit Review Bot]

Comment on attachment 100116 [details]
Patch

Attachment 100116 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9001475

[Vsevolod Vlasov]

Created attachment 100126 [details]
Patch (mac fix)

[Joseph Pecoraro]

Comment on attachment 100126 [details]
Patch (mac fix)

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

> Source/WebCore/inspector/InspectorResourceAgent.cpp:440
> +    memoryCache()->setDisabled(false);

Technically this should restore the memory cache state to whatever the normal operating
state was when the agent was enabled. But I'm sure its safe to assume enabling the cache is fine.

> Source/WebCore/inspector/front-end/Settings.js:85
> +    this.installApplicationSetting("cacheDisabled", false);

What if the cache is already disabled? I think it would be cooler if this reflected
the actual state of the WebCore MemoryCache. In most cases, yes the cache
will be enabled, but that may not always be the case.

> Source/WebCore/inspector/front-end/SettingsScreen.js:49
> +    p.appendChild(this._createCheckboxSetting(WebInspector.UIString("Disable cache"), WebInspector.settings.cacheDisabled));

Was this a new localized string? It would need to be added to Source/WebCore/English.lproj/localizedStrings.js.

[Pavel Feldman]

Comment on attachment 100126 [details]
Patch (mac fix)

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

> Source/WebCore/inspector/InspectorResourceAgent.cpp:433
> +    memoryCache()->setDisabled(m_state->getBoolean(ResourceAgentState::cacheDisabled));

I'd rather introduce memoryCache()->setDisabledByInspector().

[Vsevolod Vlasov]

Created attachment 100288 [details]
Patch

[Vsevolod Vlasov]

Created attachment 100289 [details]
Patch

[Pavel Feldman]

Comment on attachment 100289 [details]
Patch

Looks good inspector-wise. Lets get blessing from ap on the MemoryCache change.

[WebKit Review Bot]

Comment on attachment 100289 [details]
Patch

Attachment 100289 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9011479

[Vsevolod Vlasov]

Created attachment 100300 [details]
Patch (mac build fix)

[Alexey Proskuryakov]

Comment on attachment 100300 [details]
Patch (mac build fix)

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

The WebCore code that was added seems unnecessary - we already have a way to disable memory cache via Safari Debug menu <http://macs.about.com/od/usingyourmac/qt/safaridebug.htm>. You should be able to use the same code as existing functionality does.

Finally, I'm really unsure if Web Inspector users ever need to disable WebCore memory cache. This is useful for WebKit development, and this is why the functionality is in Debug menu, but I don't think that it should be exposed via Web Inspector.

> Source/WebCore/loader/cache/MemoryCache.h:210
> +    bool m_disabledByInspector; // Whether or not the cache is disabled by inspector.

I'm very much unsure if there is a need to track how exactly the memory cache was disabled.

[Vsevolod Vlasov]

Created attachment 101317 [details]
Patch with comments addressed

[Vsevolod Vlasov]

Created attachment 101319 [details]
Patch with comments addressed (rebaselined)

[Vsevolod Vlasov]

As MemoryCahce::setDisabled is only called outside of WebCore currently, I moved this call out to chromium specific code.
This way, each port could take care of memoryCache state independently.

> Finally, I'm really unsure if Web Inspector users ever need to disable WebCore memory cache. This is useful for WebKit development, and this is why the functionality is in Debug menu, but I don't think that it should be exposed via Web Inspector.

I contacted one of the twitter developers, who claimed they need that. It appears they have different environment in production and development and while they evidently don't need to disable cache in production, the find it very useful to disable it while developing.

[Alexey Proskuryakov]

> I contacted one of the twitter developers, who claimed they need that.

What would be valuable is knowing _why_ they need it. For any feature, there will be many people claiming that they absolutely require it.

[Vsevolod Vlasov]

(In reply to comment #15)
> > I contacted one of the twitter developers, who claimed they need that.
> 
> What would be valuable is knowing _why_ they need it. For any feature, there will be many people claiming that they absolutely require it.

As I said, they have different development environment where they do not version their scripts - not something you want to do each time you change one character.
It's clearly possible to force web server to send no-cache headers, but that would require an extra effort from web developers.

[Pavel Feldman]

Comment on attachment 101319 [details]
Patch with comments addressed (rebaselined)

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

> Source/WebCore/inspector/InspectorResourceAgent.cpp:439
> +    m_client->setCacheDisabled(m_state->getBoolean(ResourceAgentState::cacheDisabled));

Nit: I'd suggest that you put this into ::restore instead.

[Vsevolod Vlasov]

Committed r91408: <http://trac.webkit.org/changeset/91408>

[Vsevolod Vlasov]

(In reply to comment #17)
> (From update of attachment 101319 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101319&action=review
> 
> > Source/WebCore/inspector/InspectorResourceAgent.cpp:439
> > +    m_client->setCacheDisabled(m_state->getBoolean(ResourceAgentState::cacheDisabled));
> 
> Nit: I'd suggest that you put this into ::restore instead.

Disabling cache requires setting specific headers in willSendRequest, so it requires inspectorResourceAgent to be enabled.