Add support for disabling cache in web inspector.
Created attachment 100116 [details] Patch
Comment on attachment 100116 [details] Patch Attachment 100116 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9001475
Created attachment 100126 [details] Patch (mac fix)
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.
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().
Created attachment 100288 [details] Patch
Created attachment 100289 [details] Patch
Comment on attachment 100289 [details] Patch Looks good inspector-wise. Lets get blessing from ap on the MemoryCache change.
Comment on attachment 100289 [details] Patch Attachment 100289 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9011479
Created attachment 100300 [details] Patch (mac build fix)
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.
Created attachment 101317 [details] Patch with comments addressed
Created attachment 101319 [details] Patch with comments addressed (rebaselined)
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.
> 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.
(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.
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.
Committed r91408: <http://trac.webkit.org/changeset/91408>
(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.