Bug 43531 – Web Inspector: Support appcache status change for Chrome

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Kavita Kanetkar]

Currently it's always UNCACHED.

[Kavita Kanetkar]

Created attachment 63647 [details]
proposed fix

Here, the appcache status update piggybacks off ApplicationCacheHost::notifyDOMApplicationCache()
and pushes the status to inspector frontend via an *expensive* call status().

So is the check for:
page->inspectorController()->inspectorFrontend()

sufficient to guarantee that inspector is launched and only for that page? 
I did look through the code and looks like disconnectFrontend() is called if there's no frontend open.
Again, if frontend is connected for other pages,
page->inspectorController()->inspectorFrontend() 
will still be NULL if this particular appcache host's page does not have inspector open. Am I correct?

Thanks!

[Michael Nordman]

Assuming page->inspectorController()->inspectorFrontend() is NULL when the inspector is not open... this LGTM

[Eric Seidel (no email)]

Why is this needed?  Why is this only needed for Chrome?

[Joseph Pecoraro]

The InspectorApplicationCache agent has the same lifetime as the InspectorFrontend.
It is considered an "Inspector Frontend Lifetime Agent". It only exists while there is
a frontend. For reference you can see this in InspectorController:

  - in InspectorController::connectFrontend, an InspectorFrontend is created and
     soon after an InspectorApplicationCache agent is created and passed that frontend.

  - in InspectorController::releaseFrontendLifetimeAgents it is cleared. Note this is
     called via disconnectFrontend.

That way, the InspectorApplicationAgent should only exist if there is a frontend, and
the frontend it has (m_frontend) should always exist.

I think you only need to check if the InspectorApplicationAgent exists or not,
for instance "page->inspectorController()->applicationCacheAgent()". You shouldn't
need to check for the frontend itself. Note, this is the approach that WebKit took:

>    if (Page *page = frame->page()) {
>        if (InspectorApplicationCacheAgent* applicationCacheAgent = page->inspectorController()->applicationCacheAgent()) {
>            ...
>        }
>    }

[Joseph Pecoraro]

(In reply to comment #3)
> Why is this needed?  Why is this only needed for Chrome?

Hi Eric. Chrome has its own, port specific implementation of
Application Caches. It shares only the ApplicationCacheHost.h
interface with WebKit's implementation.

[Michael Nordman]

I think this same technique (siphoning off of the event stream) could also work for webcore's impl. With some modest refactoring we could probably share the class responsible (not just the technique) for the event dispatching.

See https://bugs.webkit.org/show_bug.cgi?id=41908 for the type of refactoring that may help.

I'm not saying we should do that refactoring in this patch, it would be good to get it functioning with the current code base.

[Pavel Feldman]

Comment on attachment 63647 [details]
proposed fix

WebKit/chromium/src/ApplicationCacheHost.cpp:209
 +          if (page && page->inspectorController()->inspectorFrontend() && page->mainFrame() == m_documentLoader->frame())
You should not make ApplicationCacheHost too much involved into the InspectorController's business. inspectorFrontend() should not be used as indication of the frontend availability + it will actually go away this week. You should notify inspector controller on any change and let it decide when to trigger the action.


WebKit/chromium/src/ApplicationCacheHost.cpp:210
 +              page->inspectorController()->applicationCacheAgent()->updateApplicationCacheStatus(status());
So you will simply check applicationCacheAgent for being 0, and if not, notify it.

[Kavita Kanetkar]

Created attachment 63935 [details]
patch2

Thanks for the review. I understand that host shouldn't tell controller when to push things to FE.
But in this case I couldn't keep pushing ACH's status() because it was relatively expensive.

Now checking if the agent exists. If so, update the status. Please take a look.

[Joseph Pecoraro]

Comment on attachment 63935 [details]
patch2

> 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.

[Joseph Pecoraro]

> 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.

Filed bug 43764.

[WebKit Commit Bot]

Comment on attachment 63935 [details]
patch2

Rejecting patch 63935 from commit-queue.

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
fatal: Unable to create '/Users/eseidel/Projects/CommitQueue/.git/svn/refs/remotes/trunk/index.lock': File exists.

If no other git process is currently running, this probably means a
git process crashed in this repository earlier. Make sure no other git
process is running and remove the file manually to continue.
write-tree: command returned error: 128

Died at WebKitTools/Scripts/update-webkit line 129.

Full output: http://queues.webkit.org/results/3756009

[WebKit Commit Bot]

Comment on attachment 63935 [details]
patch2

Clearing flags on attachment: 63935

Committed r65094: <http://trac.webkit.org/changeset/65094>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/65094 might have broken SnowLeopard Intel Release (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/65094
http://trac.webkit.org/changeset/65095