Bug 42426 - Implement remaining Inspector support for chrome's appcache
Summary: Implement remaining Inspector support for chrome's appcache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-15 17:12 PDT by Kavita Kanetkar
Modified: 2010-07-24 08:09 PDT (History)
11 users (show)

See Also:


Attachments
patch (10.80 KB, patch)
2010-07-15 17:25 PDT, Kavita Kanetkar
no flags Details | Formatted Diff | Diff
patch2 (10.45 KB, patch)
2010-07-15 18:47 PDT, Kavita Kanetkar
no flags Details | Formatted Diff | Diff
patch3 (10.80 KB, patch)
2010-07-15 20:20 PDT, Kavita Kanetkar
no flags Details | Formatted Diff | Diff
correct patch3 (14.43 KB, patch)
2010-07-15 20:33 PDT, Kavita Kanetkar
pfeldman: review-
Details | Formatted Diff | Diff
patch (17.48 KB, patch)
2010-07-21 16:38 PDT, Kavita Kanetkar
no flags Details | Formatted Diff | Diff
patch (16.95 KB, patch)
2010-07-21 17:00 PDT, Kavita Kanetkar
joepeck: review-
Details | Formatted Diff | Diff
patch (16.92 KB, patch)
2010-07-21 18:50 PDT, Kavita Kanetkar
joepeck: review-
Details | Formatted Diff | Diff
patch (16.95 KB, patch)
2010-07-22 15:31 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-07-15 17:12:25 PDT
Related to Bug 24529
Comment 1 Kavita Kanetkar 2010-07-15 17:25:38 PDT
Created attachment 61750 [details]
patch 

Final pieces to make things work with chrome's appcache.

Thanks.
Comment 2 Early Warning System Bot 2010-07-15 17:32:21 PDT
Attachment 61750 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3583032
Comment 3 WebKit Review Bot 2010-07-15 17:49:53 PDT
Attachment 61750 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3395448
Comment 4 Kavita Kanetkar 2010-07-15 18:47:51 PDT
Created attachment 61756 [details]
patch2
Comment 5 Michael Nordman 2010-07-15 19:20:36 PDT
Comment on attachment 61756 [details]
patch2

WebCore/inspector/InspectorController.cpp:529
 +      
looks like some stray whitespace got introduced here

WebCore/inspector/InspectorApplicationCacheAgent.h:87
 +      void updateApplicationCacheStatus(int status);
makes me sad that we can't find a way to use the more semantically rich data type here :(

WebCore/loader/appcache/ApplicationCacheHost.h:119
 +          InspectorApplicationCacheAgent::ApplicationCacheInfo applicationCacheInfo();
Would associatedCacheInfo() be more descriptive? Maybe not?


What about WebKit\chromium\src\ApplicationCacheHost.cpp?
Comment 6 Joseph Pecoraro 2010-07-15 19:38:45 PDT
> WebCore/loader/appcache/ApplicationCacheHost.h:119
>  +          InspectorApplicationCacheAgent::ApplicationCacheInfo applicationCacheInfo();
> Would associatedCacheInfo() be more descriptive? Maybe not?

I think if we go with "obsoleteAssociatedCache" (or delete/destory) for the delete button
semantics, then "associatedCacheInfo" absolutely makes the most sense.
Comment 7 Kavita Kanetkar 2010-07-15 20:20:55 PDT
Created attachment 61759 [details]
patch3

I thought I'd add it to WebKit API bug. Added it here now.

Thanks!
Comment 8 Early Warning System Bot 2010-07-15 20:28:54 PDT
Attachment 61759 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3575044
Comment 9 Kavita Kanetkar 2010-07-15 20:33:57 PDT
Created attachment 61760 [details]
correct patch3
Comment 10 WebKit Review Bot 2010-07-15 20:35:36 PDT
Attachment 61759 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3573043
Comment 11 WebKit Review Bot 2010-07-15 21:02:33 PDT
Attachment 61760 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3567047
Comment 12 Michael Nordman 2010-07-16 12:26:33 PDT
Comment on attachment 61760 [details]
correct patch3



WebKit/chromium/src/ApplicationCacheHost.cpp:213
 +  InspectorApplicationCacheAgent::ApplicationCacheInfo ApplicationCacheHost::applicationCacheInfo()
this outgoing call needs a check for m_internal

WebKit/chromium/src/ApplicationCacheHost.cpp:217
 +      // Convert to webcore.
i think the comment about converting here (and below) state the obvious and aren't really needed

WebKit/chromium/src/ApplicationCacheHost.cpp:229
 +              KURL(webResources[i].url), webResources[i].isMaster, webResources[i].isManifest, webResources[i].isFallback, webResources[i].isForeign,
indent is off here

class WebURL has a KURL() operator so i don't think we need to explicitly invoke the KURL ctor

WebKit/chromium/src/ApplicationCacheHostInternal.h:58
 +          // FIXME: Prod the inspector to update it's notion of what cache the page is using.
compile bustage... duplicate method


WebKit/chromium/src/ApplicationCacheHostInternal.h: 
 +  
any reason to remove this blank line?

WebCore/inspector/InspectorApplicationCacheAgent.cpp:55
 +  void InspectorApplicationCacheAgent::updateApplicationCacheStatus(int status)
If it's technically impossible to use the enum, maybe a comment to make it clear what values are expected here.
 // Status is of type ApplicationCacheHost::Status

Although for the life of me I can't see where its technically impossible to use the more descriptive type. I don't see what the issue is with defining the ResourceInfo and CacheInfo structs in the ApplicationCacheHost.h file. They are part of the interface (not the implementation) and just belong there. 

Here's how this same thing looks on the class that represents ApplicationCacheHost in chromium's WebKitAPI.
http://trac.webkit.org/browser/trunk/WebKit/chromium/public/WebApplicationCacheHost.h

95	    // Structures and methods to support inspecting Application Caches.
96	    struct CacheInfo {
97	        WebURL manifestURL; // Empty if there is no associated cache.
98	        double creationTime;
99	        double updateTime;
100	        long long totalSize;
101	        CacheInfo() : creationTime(0), updateTime(0), totalSize(0) { }
102	    };
103	    struct ResourceInfo {
104	        WebURL url;
105	        long long size;
106	        bool isMaster;
107	        bool isManifest;
108	        bool isExplicit;
109	        bool isForeign;
110	        bool isFallback;
111	        ResourceInfo() : size(0), isMaster(false), isManifest(false), isExplicit(false), isForeign(false), isFallback(false) { }
112	    };
113	    virtual void getAssociatedCacheInfo(CacheInfo*) { }
114	    virtual void getResourceList(WebVector<ResourceInfo>*) { }
115	    virtual void deleteAssociatedCacheGroup() { }

I'd like to see us that the corresponding thing in webcore/loader/appcache/ApplicationCacheHost.h. Then cyclic .h dependencies just go away.
Comment 13 Pavel Feldman 2010-07-18 12:27:32 PDT
Comment on attachment 61760 [details]
correct patch3

Do you have a reviewer for this? Joe are you open to reviewing this? (Note that it did not compile with chromium).
Comment 14 Pavel Feldman 2010-07-18 12:43:48 PDT
Comment on attachment 61760 [details]
correct patch3

Looks promising. r- for build failure.

WebKit/chromium/src/ApplicationCacheHost.cpp:223
 +      if (m_internal) {
Nit: I prefer guards to conditions in such cases:
if (!m_internal)
    return;
...

WebKit/chromium/src/ApplicationCacheHost.cpp:229
 +              KURL(webResources[i].url), webResources[i].isMaster, webResources[i].isManifest, webResources[i].isFallback, webResources[i].isForeign,
poor indentation.

WebCore/ChangeLog:5
 +          Implement remaining Inspector support for chrome's appcache
You might want to add a note relavant to WebCore here (such as "Move fillResourceList into the ApplicationCacheHost")

WebCore/inspector/InspectorApplicationCacheAgent.h:87
 +      void updateApplicationCacheStatus(int status);
Somewhere we need to require that this int is in sync with the frontend.

WebCore/loader/appcache/ApplicationCacheHost.cpp:257
 +      if (!cache || !cache->isComplete())
ASSERT was replaced with the guard. Which is right? Or does it depend on the host browser?
Comment 15 Joseph Pecoraro 2010-07-18 15:27:07 PDT
> Although for the life of me I can't see where its technically impossible to use the more
> descriptive type. I don't see what the issue is with defining the ResourceInfo and
> CacheInfo structs in the ApplicationCacheHost.h file. They are part of the interface
> (not the implementation) and just belong there. 

That sounds fine to me.

(In reply to comment #13)
> Do you have a reviewer for this? Joe are you open to reviewing this?
> (Note that it did not compile with chromium).

The WebCore parts looked good. I thought Michael Nordman already
reviewed the Chromium parts (which were new). I'm open to reviewing
the WebCore parts.
Comment 16 Kavita Kanetkar 2010-07-21 16:38:13 PDT
Created attachment 62247 [details]
patch

Moved data structs to host from agent.
Comment 17 Kavita Kanetkar 2010-07-21 17:00:30 PDT
Created attachment 62248 [details]
patch

Addressed comments from pfeldman.
Comment 18 Joseph Pecoraro 2010-07-21 17:30:07 PDT
Comment on attachment 62248 [details]
patch

> +++ WebKit/chromium/src/ApplicationCacheHost.cpp	(working copy)
> +    for (int i = 0; i < webResources.size(); ++i) {
> +        resources->append(ResourceInfo(
> +        KURL(webResources[i].url), webResources[i].isMaster, webResources[i].isManifest, webResources[i].isFallback, webResources[i].isForeign, webResources[i].isExplicit, webResources[i].size));
> +        }

Whitespace issue.


> +++ WebCore/inspector/InspectorApplicationCacheAgent.h	(working copy)
>  #include "ApplicationCacheHost.h"
> -#include "KURL.h"
>  #include "PlatformString.h"
>  #include <wtf/Noncopyable.h>

Can PlatformString be removed here as well? I don't see other String usage.


>      // Backend to Frontend
>      void didReceiveManifestResponse(unsigned long identifier, const ResourceResponse&);
> +
>      void updateApplicationCacheStatus(ApplicationCacheHost::Status);
> +
>      void updateNetworkState(bool isNowOnline);

Why the extra newlines?


> +++ WebCore/loader/appcache/ApplicationCacheHost.h	(working copy)
> +#include "KURL.h"
>      ...
>      class KURL;

Looks like the forward declaration can be removed.


> +        struct CacheInfo {
> +            CacheInfo(const KURL& manifest, double creationTime, double updateTime, long long size)
> +                : m_manifest(manifest)
> +                , m_creationTime(creationTime)
> +                , m_updateTime(updateTime)
> +                , m_size(size)
> +        {
> +        }
> +
> +        KURL m_manifest;
> +        double m_creationTime;
> +        double m_updateTime;
> +        long long m_size;
> +    };

Whitespace issues again on the "struct" and constructor lines.

You did change the creation and update times from String to double.
I like that change. I think it would be nice to clarify this in the
ChangeLog. You didn't just "move it" you also changed it.


> +        struct CacheInfo {
> +    struct ResourceInfo {
> +    typedef Vector<ResourceInfo> ResourceInfoList;

I think these should be moved into the ENABLE(INSPECTOR) guard.

I don't think any of these are r- worthy, but you don't have committer
rights yet. The Chromium side still needs a chromium reviewer, but it looks
straightforward.
Comment 19 Michael Nordman 2010-07-21 18:19:32 PDT
Comment on attachment 62248 [details]
patch

WebKit/chromium/src/ApplicationCacheHost.cpp:216
 +        return CacheInfo(WebURL(), 0, 0, 0);
ApplicationCacheHost::CacheInfo's ctor takes a KURL, not a WebURL().

WebKit/chromium/src/ApplicationCacheHost.cpp:232
 +          KURL(webResources[i].url), webResources[i].isMaster, webResources[i].isManifest, webResources[i].isFallback, webResources[i].isForeign, webResources[i].isExplicit, webResources[i].size));
indent is off here/line wrap is odd with such a long line
class WebURL has a KURL() operator so i don't think we need to explicitly invoke the KURL ctor
Comment 20 Kavita Kanetkar 2010-07-21 18:50:45 PDT
Created attachment 62253 [details]
patch

Thanks Joe and Michael for the review.
Comment 21 Michael Nordman 2010-07-21 21:59:26 PDT
Comment on attachment 62253 [details]
patch

WebKit/chromium/src/ApplicationCacheHost.cpp:213
 +  ApplicationCacheHost::CacheInfo ApplicationCacheHost::applicationCacheInfo()
Sorry i didn't notice this before. These two new methods should in an #if ENABLE(INSPECTOR) block to match the .h file.

WebKit/chromium/src/ApplicationCacheHost.cpp:232
 +          KURL(webResources[i].url), webResources[i].isMaster, webResources[i].isManifest, webResources[i].isFallback,
Having this line indent to the same level as the preceding line catches my eye. 

230     for (int i = 0; i < webResources.size(); ++i) {
231         resources->append(ResourceInfo(
232              webResources[i].url, webResources[i].isMaster, webResources[i].isManifest, webResources[i].isFallback,
233              webResources[i].isForeign, webResources[i].isExplicit, webResources[i].size));
234     }

Also, class WebURL has a KURL() operator so i don't think we need to explicitly invoke the KURL ctor. WebURL intentionally has a cast operator just for this purpose, so the resulting code is easier to read with less conversion code obscuring things.
Comment 22 Joseph Pecoraro 2010-07-22 11:40:35 PDT
Comment on attachment 62253 [details]
patch

> +            CacheInfo(const KURL& manifest, double creationTime, double updateTime, long long size)
> +                : m_manifest(manifest)
> +                , m_creationTime(creationTime)
> +                , m_updateTime(updateTime)
> +                , m_size(size) {   }

> +            ResourceInfo(const KURL& resource, bool isMaster, bool isManifest, bool isFallback, bool isForeign, bool isExplicit, long long size)
> +                : m_resource(resource)
> +                , m_isMaster(isMaster)
> +                , m_isManifest(isManifest)
> +                , m_isFallback(isFallback)
> +                , m_isForeign(isForeign)
> +                , m_isExplicit(isExplicit)
> +                , m_size(size) {  }

You changed the format here. I don't know the correct style for an empty
constructor like this. I put the braces on individual lines before, I
have also seen braces with a single space in between "{ }". This change
is neither of these. I'd suggest looking around at some other structs
and matching the style of those for empty constructors.

Since there are comments that need to be addressed by Michael and you will
need to put up another patch anyways I'll set r-.
Comment 23 Kavita Kanetkar 2010-07-22 15:31:54 PDT
Created attachment 62350 [details]
patch

Addressed comments.
Comment 24 Michael Nordman 2010-07-22 15:36:18 PDT
lgtm
Comment 25 Joseph Pecoraro 2010-07-22 16:51:21 PDT
Looks good to me as well. There is some trailing whitespace, but that
is not important.
Comment 26 WebKit Commit Bot 2010-07-23 16:24:58 PDT
Comment on attachment 62350 [details]
patch

Rejecting patch 62350 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20715 test cases.
animations/play-state.html -> failed

Exiting early after 1 failures. 123 tests run.
16.28s total testing time

122 test cases (99%) succeeded
1 test case (<1%) had incorrect layout

Full output: http://queues.webkit.org/results/3573361
Comment 27 Joseph Pecoraro 2010-07-23 17:18:57 PDT
> animations/play-state.html -> failed

I don't think this is your fault. Feel free to re-set the cq+ flag again
when you think it is safe to do so.
Comment 28 Eric Seidel (no email) 2010-07-23 20:09:41 PDT
Comment on attachment 62350 [details]
patch

Bug 42821.
Comment 29 WebKit Commit Bot 2010-07-23 20:35:37 PDT
Comment on attachment 62350 [details]
patch

Clearing flags on attachment: 62350

Committed r64006: <http://trac.webkit.org/changeset/64006>
Comment 30 WebKit Commit Bot 2010-07-23 20:35:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 WebKit Review Bot 2010-07-23 21:09:17 PDT
http://trac.webkit.org/changeset/64006 might have broken Chromium Mac Release
Comment 32 Pavel Feldman 2010-07-24 08:09:51 PDT
This broke Chromium Mac. I never use cq for WebKit/chromium commits. I land manually and watch canaries.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/src/ApplicationCacheHost.cpp
Committed r64009