Bug 61192 - Web Inspector: do not issue frontendReused for reload or navigate.
Summary: Web Inspector: do not issue frontendReused for reload or navigate.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Kosyakov
URL:
Keywords:
Depends on:
Blocks: 61036
  Show dependency treegraph
 
Reported: 2011-05-20 09:05 PDT by Pavel Feldman
Modified: 2011-05-30 02:37 PDT (History)
12 users (show)

See Also:


Attachments
Patch (8.12 KB, patch)
2011-05-20 09:09 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (8.24 KB, patch)
2011-05-20 09:46 PDT, Pavel Feldman
pfeldman: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from cr-jail-8 (207.50 KB, application/zip)
2011-05-20 12:01 PDT, WebKit Commit Bot
no flags Details
patch (5.75 KB, patch)
2011-05-23 02:30 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
patch (untabified; sorry) (5.78 KB, patch)
2011-05-23 02:44 PDT, Andrey Kosyakov
pfeldman: review-
Details | Formatted Diff | Diff
patch (14.14 KB, patch)
2011-05-25 09:11 PDT, Andrey Kosyakov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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