[webkit-reviews] review granted: [Bug 43531] Web Inspector: Support appcache status change for Chrome : [Attachment 63935] patch2

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Kavita Kanetkar
<kkanetkar at chromium.org>'s request for review:
Bug 43531: Web Inspector: Support appcache status change for Chrome
https://bugs.webkit.org/show_bug.cgi?id=43531

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

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
> Index: WebKit/chromium/src/ApplicationCacheHost.cpp
> +	   if (page && page->inspectorController()->applicationCacheAgent() &&
page->mainFrame() == m_documentLoader->frame())

Patch looks good to me. Two points I wanted to comment on, which you may
have been aware of.

The "page->mainFrame()" check makes sense for now. I don't think we handle
this correctly in WebKit. I think once we support multiple frames / manifests
(see bug 41636) we will probably end up passing the frame into the calls to
applicationCacheAgent.

Another point, it makes sense to ignore the progress event here, because
our UI doesn't do anything for progress events for now. But in the future
we might show some kind of progress, maybe a progress bar or #/# indicator.
I don't think there is a bug on that. I will file one now.