[webkit-reviews] review granted: [Bug 72123] Web Inspector: Application cache status should be updated on swap. : [Attachment 115016] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has granted Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 72123: Web Inspector: Application cache status should be updated on swap.
https://bugs.webkit.org/show_bug.cgi?id=72123

Attachment 115016: Patch
https://bugs.webkit.org/attachment.cgi?id=115016&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=115016&action=review


r+ given the macros are removed prior to landing.

> Source/WebCore/ChangeLog:3
> +	   Web Inspector: Application cache status should be updated on swap.

What is "swap" could you please add more details into the changelog?

> Source/WebCore/inspector/front-end/ApplicationCacheItemsView.js:130
> +	   if (this.isShowing() && this._status === applicationCache.IDLE &&
(oldStatus === applicationCache.UPDATEREADY || !this._resources))

I noticed that you are doing unconditional update upon wasShown. You could
instead set a dirty flag here and only update in case of dirty.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:244
> +#if ENABLE(INSPECTOR)

You don't need this macro here. Instrumentation should take care of overhead,
while frame getter and != check are not expensive.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:459
> +#if ENABLE(INSPECTOR)

ditto.

> LayoutTests/http/tests/inspector/resource-tree/appcache-test.js:3
>  function createAndNavigateIFrame(url)

Drive-by: why is this file under the resource-tree folder?