[webkit-reviews] review denied: [Bug 169865] Web Inspector: Add "Disable Caches" option that only applies to the inspected page while Web Inspector is open : [Attachment 305398] Proposed Fix v1.1 (pending possible new icon)

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Brian Burg <bburg at apple.com>'s
request for review:
Bug 169865: Web Inspector: Add "Disable Caches" option that only applies to the
inspected page while Web Inspector is open
https://bugs.webkit.org/show_bug.cgi?id=169865

Attachment 305398: Proposed Fix v1.1 (pending possible new icon)

https://bugs.webkit.org/attachment.cgi?id=305398&action=review




--- Comment #14 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 305398
  --> https://bugs.webkit.org/attachment.cgi?id=305398
Proposed Fix v1.1 (pending possible new icon)

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

r- for two reasons:

    1. Need tests for all new protocol commands.
    2. I think we need to more appropriately address setting/restoring this
state

> Source/WebInspectorUI/ChangeLog:33
> +	   * Versions/Inspector-iOS-9.3.json:
> +	   Remove the old command, it was not used and didn't do the right
thing anyway.

You need to run
Source/WebInspector/Scripts/update-LegacyInspectorBackendCommands.rb to update
the associated BackendCommands for these backends.

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:652
> +    m_pageAgent->page().setResourceCachingDisabled(false);

There are two approaches for toggling WebCore state from Web Inspector.

-- Approach A: Inspector Only Override

This approach makes sense for setting that are only active when Web Inspector
is open and goes away when Web Inspector closes.

The way we were doing this for other Inspector overrides is to essentially have
two settings:

    1. The normal setting which all other clients may be toggling.
    2. The inspector override of the setting not changing what clients are
toggling.

And change the normal setting getter to check both (1) and (2) [or just (2)].
This would mean something like:

    Page::setResourceCacheDisabled(bool x) { m_resourceCachingDisabled = x; }
// (1)
    Page::setResourceCacheDisabledInspectorOverride(bool x) {
m_resourceCachingDisabledInspectorOverride = x; } // (2).
    Page::isResourceCacheDisabled() { return m_resourceCachingDisabled ||
m_resourceCachingDisabledInspectorOverride; }

In this case Web Inspector forcibly overrides a built-in setting where others
can modify that built-in setting we need a pattern like this to avoid the
inspector suddenly breaking the other client of the normal setting.


-- Approach B: Toggle the Normal Setting

This makes sense for settings that persist when Web Inspector is not open and
Web Inspector checks for their initial values when it opens and toggles the
values the same way other clients toggle it (affecting all clients).

If this is the case we would detect the initial Page::isResourceCacheDisabled
value when Web Inspector opens and reflect that.

------

I think this falls into category A. Because we only want to disable the
resource cache while Web Inspector is open, and return back to normal settings
when Web Inspector closes. The browser may be disabling cache, but Web
Inspector doesn't need to reflect that, it has its own force disable cache
button that does not affect when Web Inspector is not open.