Bug 61192

Summary: Web Inspector: do not issue frontendReused for reload or navigate.
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Andrey Kosyakov <caseq>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, commit-queue, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 61036    
Attachments:
Description Flags
Patch
none
Patch
pfeldman: review+, commit-queue: commit-queue-
Archive of layout-test-results from cr-jail-8
none
patch
none
patch (untabified; sorry)
pfeldman: review-
patch pfeldman: review+

Description Pavel Feldman 2011-05-20 09:05:46 PDT
We should only issue it while connecting to the page that is loaded prior to attach only.
Comment 1 Pavel Feldman 2011-05-20 09:09:47 PDT
Created attachment 94232 [details]
Patch
Comment 2 Yury Semikhatsky 2011-05-20 09:35:46 PDT
Comment on attachment 94232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94232&action=review

> Source/WebCore/inspector/front-end/ResourcesPanel.js:248
> +        if (!event.data.frame.parentId) {

Please add a comment explaining this logic.
Comment 3 Pavel Feldman 2011-05-20 09:46:19 PDT
Created attachment 94235 [details]
Patch
Comment 4 WebKit Commit Bot 2011-05-20 12:01:30 PDT
Comment on attachment 94235 [details]
Patch

Rejecting attachment 94235 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2

Last 500 characters of output:
tests/xmlhttprequest ................................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers ...........
http/tests/xmlviewer .
http/tests/xmlviewer/dumpAsText ...........
767.94s total testing time

23585 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
17 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8718550
Comment 5 WebKit Commit Bot 2011-05-20 12:01:35 PDT
Created attachment 94255 [details]
Archive of layout-test-results from cr-jail-8

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: cr-jail-8  Port: Mac  Platform: Mac OS X 10.6.7
Comment 6 Andrey Kosyakov 2011-05-23 02:30:02 PDT
Created attachment 94389 [details]
patch

The way we remove old frames and preserve the main frame upon navigation is somewhat unfortunate, so let's at least try to keep it on the model level, rather than have similar logic proliferate to the higher-level ResourcePanel. We already have notifications going from model to panel upon resource changes, so it shouldn't be necessary to have a separate logic to reset it upon navigation (unless for performance reasons, which, I think, are not really an issue here). Here's my shot at fixing this -- mostly similar, but remove obsolete frames one-by-one via "usual" means (_frameDetached).

To Pavel: I couldn't reproduce the bug you were observing with id collision of old and new main frames in different renderers with my approach. OTOH, I don't see why there should be such a bug, given that we seem to properly replace old frames with new ones in case of id collision. May be I'm missing something... :)
Comment 7 WebKit Review Bot 2011-05-23 02:32:04 PDT
Attachment 94389 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/front-end/ResourceTreeModel.js:128:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/inspector/front-end/ResourceTreeModel.js:160:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/inspector/front-end/ResourceTreeModel.js:161:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Andrey Kosyakov 2011-05-23 02:44:04 PDT
Created attachment 94391 [details]
patch (untabified; sorry)
Comment 9 Pavel Feldman 2011-05-24 10:32:32 PDT
Comment on attachment 94391 [details]
patch (untabified; sorry)

This will regress HTML5 resources panel cleanup that was performed from resourcetreemodel's frontendReused callback.
Comment 10 Andrey Kosyakov 2011-05-25 09:11:46 PDT
Created attachment 94790 [details]
patch

- fixed HTML5 storage problem (exposed StoragePanel.reset())
- changed order of notifications from backend upon didCommitLoad (first do all resets, then notify page agent on navigation)
- now reset m_DidCommitLoadFired when detaching front-end
- create main resource early, to get resource names in resource tree abbreviated consistently
Comment 11 Pavel Feldman 2011-05-25 11:10:19 PDT
Comment on attachment 94790 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94790&action=review

> Source/WebCore/inspector/InspectorInstrumentation.cpp:608
> +        pageAgent->frameNavigated(loader);

I am positive it regresses :) Did you run the tests?
Comment 12 Andrey Kosyakov 2011-05-26 05:04:51 PDT
(In reply to comment #11)
> (From update of attachment 94790 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94790&action=review
> 
> > Source/WebCore/inspector/InspectorInstrumentation.cpp:608
> > +        pageAgent->frameNavigated(loader);
> 
> I am positive it regresses :) Did you run the tests?

All tests are passing, manual tests look sane too. Can you please be a bit more specific?
Comment 13 Pavel Feldman 2011-05-27 12:23:31 PDT
Comment on attachment 94790 [details]
patch

Ok, it looks sane. Lets see how it goes.
Comment 14 Andrey Kosyakov 2011-05-30 02:22:51 PDT
Manually committed r87681: http://trac.webkit.org/changeset/87681