Bug 61192 – Web Inspector: do not issue frontendReused for reload or navigate.

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

[Pavel Feldman]

We should only issue it while connecting to the page that is loaded prior to attach only.

[Pavel Feldman]

Created attachment 94232 [details]
Patch

[Yury Semikhatsky]

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.

[Pavel Feldman]

Created attachment 94235 [details]
Patch

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Andrey Kosyakov]

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

[WebKit Review Bot]

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.

[Andrey Kosyakov]

Created attachment 94391 [details]
patch (untabified; sorry)

[Pavel Feldman]

Comment on attachment 94391 [details]
patch (untabified; sorry)

This will regress HTML5 resources panel cleanup that was performed from resourcetreemodel's frontendReused callback.

[Andrey Kosyakov]

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

[Pavel Feldman]

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?

[Andrey Kosyakov]

(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?

[Pavel Feldman]

Comment on attachment 94790 [details]
patch

Ok, it looks sane. Lets see how it goes.

[Andrey Kosyakov]

Manually committed r87681: http://trac.webkit.org/changeset/87681