Bug 42426 – Implement remaining Inspector support for chrome's appcache

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

[Kavita Kanetkar]

Related to Bug 24529

[Kavita Kanetkar]

Created attachment 61750 [details]
patch 

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

Thanks.

[Early Warning System Bot]

Attachment 61750 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3583032

[WebKit Review Bot]

Attachment 61750 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3395448

[Kavita Kanetkar]

Created attachment 61756 [details]
patch2

[Michael Nordman]

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?

[Joseph Pecoraro]

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

[Kavita Kanetkar]

Created attachment 61759 [details]
patch3

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

Thanks!

[Early Warning System Bot]

Attachment 61759 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3575044

[Kavita Kanetkar]

Created attachment 61760 [details]
correct patch3

[WebKit Review Bot]

Attachment 61759 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3573043

[WebKit Review Bot]

Attachment 61760 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3567047

[Michael Nordman]

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.

[Pavel Feldman]

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

[Pavel Feldman]

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?

[Joseph Pecoraro]

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

[Kavita Kanetkar]

Created attachment 62247 [details]
patch

Moved data structs to host from agent.

[Kavita Kanetkar]

Created attachment 62248 [details]
patch

Addressed comments from pfeldman.

[Joseph Pecoraro]

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.

[Michael Nordman]

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

[Kavita Kanetkar]

Created attachment 62253 [details]
patch

Thanks Joe and Michael for the review.

[Michael Nordman]

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.

[Joseph Pecoraro]

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

[Kavita Kanetkar]

Created attachment 62350 [details]
patch

Addressed comments.

[Michael Nordman]

lgtm

[Joseph Pecoraro]

Looks good to me as well. There is some trailing whitespace, but that
is not important.

[WebKit Commit Bot]

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

[Joseph Pecoraro]

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

[Eric Seidel (no email)]

Comment on attachment 62350 [details]
patch

Bug 42821.

[WebKit Commit Bot]

Comment on attachment 62350 [details]
patch

Clearing flags on attachment: 62350

Committed r64006: <http://trac.webkit.org/changeset/64006>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/64006 might have broken Chromium Mac Release

[Pavel Feldman]

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