Summary: | Implement remaining Inspector support for chrome's appcache | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kavita Kanetkar <kkanetkar> | ||||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, dglazkov, eric, gustavo, joepeck, michaeln, pfeldman, webkit-ews, webkit.review.bot, xan.lopez | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||
Attachments: |
|
Description
Kavita Kanetkar
2010-07-15 17:12:25 PDT
Created attachment 61750 [details]
patch
Final pieces to make things work with chrome's appcache.
Thanks.
Attachment 61750 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3583032 Attachment 61750 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3395448 Created attachment 61756 [details]
patch2
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?
> 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.
Created attachment 61759 [details]
patch3
I thought I'd add it to WebKit API bug. Added it here now.
Thanks!
Attachment 61759 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3575044 Created attachment 61760 [details]
correct patch3
Attachment 61759 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3573043 Attachment 61760 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3567047 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 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 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?
> 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. Created attachment 62247 [details]
patch
Moved data structs to host from agent.
Created attachment 62248 [details]
patch
Addressed comments from pfeldman.
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 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
Created attachment 62253 [details]
patch
Thanks Joe and Michael for the review.
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 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-. Created attachment 62350 [details]
patch
Addressed comments.
lgtm Looks good to me as well. There is some trailing whitespace, but that is not important. 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 > 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 on attachment 62350 [details] patch Bug 42821. Comment on attachment 62350 [details] patch Clearing flags on attachment: 62350 Committed r64006: <http://trac.webkit.org/changeset/64006> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/64006 might have broken Chromium Mac Release 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 |