[webkit-reviews] review granted: [Bug 169865] Web Inspector: Add "Disable Caches" option that only applies to the inspected page while Web Inspector is open : [Attachment 305631] Patch v2

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted 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 305631: Patch v2

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




--- Comment #16 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 305631
  --> https://bugs.webkit.org/attachment.cgi?id=305631
Patch v2

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

r=me

> Source/WebInspectorUI/ChangeLog:18
> +2017-03-25  Brian Burg  <bburg at apple.com>

Double ChangeLog. Just merge this top part into the bottom one.

> Source/WebInspectorUI/UserInterface/Base/Main.js:195
> +    // COMPATIBILITY (iOS 10): Network.setDisableResourceCaching did not
exist.

I've been doing "10.3" because that is really the last Versioned protocol.

> Source/WebInspectorUI/UserInterface/Base/Main.js:198
> +	  
NetworkAgent.setResourceCachingDisabled(this.resourceCachingDisabledSetting.val
ue);

We could cut down on protocol traffic by only doing this if the value is true.
I think that is a safe assumption to make.

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:131
> +	   // COMPATIBILITY (iOS 10): Network.setDisableResourceCaching did not
exist.

I've been doing "10.3" because that is really the last Versioned protocol.

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:135
> +	       this._disableResourceCacheNavigationItem = new
WebInspector.ActivateButtonNavigationItem("disable-resource-cache",
toolTipForDisableResourceCache, activatedToolTipForDisableResourceCache,
"Images/StepOver.svg", 16, 16);

This probably needs a new icon. I get the idea with this one is that we are
bypassing the cache so it looks like a "step over the cache". Not sure if you
want to have a new icon before or after landing this.

>
LayoutTests/http/tests/inspector/network/set-resource-caching-disabled-disk-cac
he.html:25
> +    let suite =
InspectorTest.createAsyncSuite("Network.SetResourceCachingDisabled.DiskCache");

The other is named "SetResourceCachingDisabled.MemoryCache". I rather like
these with the "Network." prefix.

>
LayoutTests/http/tests/inspector/network/set-resource-caching-disabled-disk-cac
he.html:63
> +    addTestCase({
> +	   name: "SetResourceCachingDisabled.DiskCache",
> +	   description: "Fail to load a resource from the disk cache when
resource caching is disabled",
> +	   expression: `triggerDiskCacheLoad()`,
> +	   responseSource: WebInspector.Resource.ResponseSource.Network,
> +	   statusCode: 200,
> +    });

This test could just have a:

    setup(result) { NetworkAgent.setResourceCachingDisabled(true); resolve() }

Instead of putting it inside "addTestCase" above which affects both the
PossibleNetworkLoad and the Attempted DiskCache Load.