43531 2010-08-04 19:08:55 -0700 Web Inspector: Support appcache status change for Chrome 2010-08-10 15:12:49 -0700 1 1 1 Unclassified WebKit Web Inspector (Deprecated) 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 0 kkanetkar webkit-unassigned abarth bweinstein commit-queue eric joepeck keishi michaeln pfeldman pmuellr rik timothy webkit.review.bot yurys oldest_to_newest 260329 0 kkanetkar 2010-08-04 19:08:55 -0700 Currently it's always UNCACHED. 260904 1 63647 kkanetkar 2010-08-05 15:33:27 -0700 Created attachment 63647 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! 261394 2 michaeln 2010-08-06 12:47:27 -0700 Assuming page->inspectorController()->inspectorFrontend() is NULL when the inspector is not open... this LGTM 261484 3 eric 2010-08-06 14:40:46 -0700 Why is this needed? Why is this only needed for Chrome? 261489 4 joepeck 2010-08-06 14:46:22 -0700 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()) { > ... > } > } 261495 5 joepeck 2010-08-06 14:47:18 -0700 (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. 261578 6 michaeln 2010-08-06 16:49:11 -0700 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. 261699 7 63647 pfeldman 2010-08-07 03:10:58 -0700 Comment on attachment 63647 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. 262343 8 63935 kkanetkar 2010-08-09 15:27:39 -0700 Created attachment 63935 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. 262443 9 63935 joepeck 2010-08-09 18:33:57 -0700 Comment on attachment 63935 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. 262448 10 joepeck 2010-08-09 18:40:32 -0700 > 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. 262484 11 63935 commit-queue 2010-08-09 21:08:59 -0700 Comment on attachment 63935 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 262851 12 63935 commit-queue 2010-08-10 14:35:33 -0700 Comment on attachment 63935 patch2 Clearing flags on attachment: 63935 Committed r65094: <http://trac.webkit.org/changeset/65094> 262852 13 commit-queue 2010-08-10 14:35:38 -0700 All reviewed patches have been landed. Closing bug. 262880 14 webkit.review.bot 2010-08-10 15:12:49 -0700 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 63647 2010-08-05 15:33:27 -0700 2010-08-09 15:27:39 -0700 proposed fix cached1.txt text/plain 1724 kkanetkar SW5kZXg6IFdlYktpdC9jaHJvbWl1bS9zcmMvQXBwbGljYXRpb25DYWNoZUhvc3QuY3BwCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFdlYktpdC9jaHJvbWl1bS9zcmMvQXBwbGljYXRpb25DYWNoZUhvc3QuY3BwCShy ZXZpc2lvbiA2NDc4OSkKKysrIFdlYktpdC9jaHJvbWl1bS9zcmMvQXBwbGljYXRpb25DYWNoZUhv c3QuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yMDIsNiArMjAyLDE1IEBACiAKIHZvaWQgQXBwbGlj YXRpb25DYWNoZUhvc3Q6Om5vdGlmeURPTUFwcGxpY2F0aW9uQ2FjaGUoRXZlbnRJRCBpZCwgaW50 IHRvdGFsLCBpbnQgZG9uZSkKIHsKKyNpZiBFTkFCTEUoSU5TUEVDVE9SKQorICAgIC8vIElmIGhv c3QncyBmcmFtZSBpcyBtYWluIGZyYW1lIGFuZCBpbnNwZWN0b3IgZnJvbnRlbmQgaXMgY29ubmVj dGVkLCB1cGRhdGUgYXBwY2FjaGUgc3RhdHVzLgorICAgIGlmIChpZCAhPSBQUk9HUkVTU19FVkVO VCAmJiBtX2RvY3VtZW50TG9hZGVyLT5mcmFtZSgpKSB7CisgICAgICAgIFBhZ2UqIHBhZ2UgPSBt X2RvY3VtZW50TG9hZGVyLT5mcmFtZSgpLT5wYWdlKCk7CisgICAgICAgIGlmIChwYWdlICYmIHBh Z2UtPmluc3BlY3RvckNvbnRyb2xsZXIoKS0+aW5zcGVjdG9yRnJvbnRlbmQoKSAmJiBwYWdlLT5t YWluRnJhbWUoKSA9PSBtX2RvY3VtZW50TG9hZGVyLT5mcmFtZSgpKQorICAgICAgICAgICAgcGFn ZS0+aW5zcGVjdG9yQ29udHJvbGxlcigpLT5hcHBsaWNhdGlvbkNhY2hlQWdlbnQoKS0+dXBkYXRl QXBwbGljYXRpb25DYWNoZVN0YXR1cyhzdGF0dXMoKSk7CisgICAgfQorI2VuZGlmCisKICAgICBp ZiAobV9kZWZlcnNFdmVudHMpIHsKICAgICAgICAgLy8gRXZlbnQgZGlzcGF0Y2hpbmcgaXMgZGVm ZXJyZWQgdW50aWwgZG9jdW1lbnQub25sb2FkIGhhcyBmaXJlZC4KICAgICAgICAgbV9kZWZlcnJl ZEV2ZW50cy5hcHBlbmQoRGVmZXJyZWRFdmVudChpZCwgdG90YWwsIGRvbmUpKTsKSW5kZXg6IFdl YktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViS2l0L2Nocm9taXVtL0No YW5nZUxvZwkocmV2aXNpb24gNjQ3ODkpCisrKyBXZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCSh3 b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTAtMDgtMDUgIEthdml0YSBLYW5ldGth ciAgPGtrYW5ldGthckBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogU3VwcG9ydCBhcHBjYWNoZSBzdGF0 dXMgY2hhbmdlIGZvciBDaHJvbWUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTQzNTMxCisKKyAgICAgICAgKiBzcmMvQXBwbGljYXRpb25DYWNoZUhvc3Qu Y3BwOiBVcGRhdGluZyBhcHBjYWNoZSBzdGF0dXMgZm9yIG5vbi1wcm9ncmVzcyBldmVudHMgdG8g YmUgcHVzaGVkIHRvIGluc3BlY3RvciBGRS4KKyAgICAgICAgKFdlYkNvcmU6OkFwcGxpY2F0aW9u Q2FjaGVIb3N0Ojpub3RpZnlET01BcHBsaWNhdGlvbkNhY2hlKToKKwogMjAxMC0wOC0wNCAgS2Vu bmV0aCBSdXNzZWxsICA8a2JyQGdvb2dsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRGlt aXRyaSBHbGF6a292Lgo= 63935 2010-08-09 15:27:39 -0700 2010-08-10 14:35:33 -0700 patch2 cached3.txt text/plain 1658 kkanetkar SW5kZXg6IFdlYktpdC9jaHJvbWl1bS9zcmMvQXBwbGljYXRpb25DYWNoZUhvc3QuY3BwCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFdlYktpdC9jaHJvbWl1bS9zcmMvQXBwbGljYXRpb25DYWNoZUhvc3QuY3BwCShy ZXZpc2lvbiA2NTAwNSkKKysrIFdlYktpdC9jaHJvbWl1bS9zcmMvQXBwbGljYXRpb25DYWNoZUhv c3QuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yMDIsNiArMjAyLDE1IEBACiAKIHZvaWQgQXBwbGlj YXRpb25DYWNoZUhvc3Q6Om5vdGlmeURPTUFwcGxpY2F0aW9uQ2FjaGUoRXZlbnRJRCBpZCwgaW50 IHRvdGFsLCBpbnQgZG9uZSkKIHsKKyNpZiBFTkFCTEUoSU5TUEVDVE9SKQorICAgIC8vIElmIGhv c3QncyBmcmFtZSBpcyBtYWluIGZyYW1lIGFuZCBpbnNwZWN0b3IgZnJvbnRlbmQgaXMgY29ubmVj dGVkLCB1cGRhdGUgYXBwY2FjaGUgc3RhdHVzLgorICAgIGlmIChpZCAhPSBQUk9HUkVTU19FVkVO VCAmJiBtX2RvY3VtZW50TG9hZGVyLT5mcmFtZSgpKSB7CisgICAgICAgIFBhZ2UqIHBhZ2UgPSBt X2RvY3VtZW50TG9hZGVyLT5mcmFtZSgpLT5wYWdlKCk7CisgICAgICAgIGlmIChwYWdlICYmIHBh Z2UtPmluc3BlY3RvckNvbnRyb2xsZXIoKS0+YXBwbGljYXRpb25DYWNoZUFnZW50KCkgJiYgcGFn ZS0+bWFpbkZyYW1lKCkgPT0gbV9kb2N1bWVudExvYWRlci0+ZnJhbWUoKSkKKyAgICAgICAgICAg IHBhZ2UtPmluc3BlY3RvckNvbnRyb2xsZXIoKS0+YXBwbGljYXRpb25DYWNoZUFnZW50KCktPnVw ZGF0ZUFwcGxpY2F0aW9uQ2FjaGVTdGF0dXMoc3RhdHVzKCkpOworICAgIH0KKyNlbmRpZgorCiAg ICAgaWYgKG1fZGVmZXJzRXZlbnRzKSB7CiAgICAgICAgIC8vIEV2ZW50IGRpc3BhdGNoaW5nIGlz IGRlZmVycmVkIHVudGlsIGRvY3VtZW50Lm9ubG9hZCBoYXMgZmlyZWQuCiAgICAgICAgIG1fZGVm ZXJyZWRFdmVudHMuYXBwZW5kKERlZmVycmVkRXZlbnQoaWQsIHRvdGFsLCBkb25lKSk7CkluZGV4 OiBXZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYktpdC9jaHJvbWl1 bS9DaGFuZ2VMb2cJKHJldmlzaW9uIDY1MDExKQorKysgV2ViS2l0L2Nocm9taXVtL0NoYW5nZUxv Zwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBACisyMDEwLTA4LTA5ICBLYXZpdGEgS2Fu ZXRrYXIgIDxra2FuZXRrYXJAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgIFdlYiBJbnNwZWN0b3I6IFN1cHBvcnQgYXBwY2FjaGUg c3RhdHVzIGNoYW5nZSBmb3IgQ2hyb21lCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD00MzUzMQorCisgICAgICAgICogc3JjL0FwcGxpY2F0aW9uQ2FjaGVI b3N0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkFwcGxpY2F0aW9uQ2FjaGVIb3N0Ojpub3RpZnlE T01BcHBsaWNhdGlvbkNhY2hlKToKKwogMjAxMC0wOC0wOSAgVmFuZ2VsaXMgS29ra2V2aXMgIDx2 YW5nZWxpc0BjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRGltaXRyaSBHbGF6 a292Lgo=