Bug 43531 - Web Inspector: Support appcache status change for Chrome
Summary: Web Inspector: Support appcache status change for Chrome
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-04 19:08 PDT by Kavita Kanetkar
Modified: 2010-08-10 15:12 PDT (History)
13 users (show)

See Also:


Attachments
proposed fix (1.68 KB, patch)
2010-08-05 15:33 PDT, Kavita Kanetkar
pfeldman: review-
Details | Formatted Diff | Diff
patch2 (1.62 KB, patch)
2010-08-09 15:27 PDT, Kavita Kanetkar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kavita Kanetkar 2010-08-04 19:08:55 PDT
Currently it's always UNCACHED.
Comment 1 Kavita Kanetkar 2010-08-05 15:33:27 PDT
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!
Comment 2 Michael Nordman 2010-08-06 12:47:27 PDT
Assuming page->inspectorController()->inspectorFrontend() is NULL when the inspector is not open... this LGTM
Comment 3 Eric Seidel (no email) 2010-08-06 14:40:46 PDT
Why is this needed?  Why is this only needed for Chrome?
Comment 4 Joseph Pecoraro 2010-08-06 14:46:22 PDT
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()) {
>            ...
>        }
>    }
Comment 5 Joseph Pecoraro 2010-08-06 14:47:18 PDT
(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.
Comment 6 Michael Nordman 2010-08-06 16:49:11 PDT
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.
Comment 7 Pavel Feldman 2010-08-07 03:10:58 PDT
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.
Comment 8 Kavita Kanetkar 2010-08-09 15:27:39 PDT
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.
Comment 9 Joseph Pecoraro 2010-08-09 18:33:57 PDT
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.
Comment 10 Joseph Pecoraro 2010-08-09 18:40:32 PDT
> 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.
Comment 11 WebKit Commit Bot 2010-08-09 21:08:59 PDT
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
Comment 12 WebKit Commit Bot 2010-08-10 14:35:33 PDT
Comment on attachment 63935 [details]
patch2

Clearing flags on attachment: 63935

Committed r65094: <http://trac.webkit.org/changeset/65094>
Comment 13 WebKit Commit Bot 2010-08-10 14:35:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 WebKit Review Bot 2010-08-10 15:12:49 PDT
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