Bug 62894 - Web Inspector: backend needs to provide system-unique object ids, so these remain unique across navigation
Summary: Web Inspector: backend needs to provide system-unique object ids, so these re...
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:
 
Reported: 2011-06-17 11:38 PDT by Andrey Kosyakov
Modified: 2011-07-01 05:25 PDT (History)
12 users (show)

See Also:


Attachments
patch (8.05 KB, patch)
2011-06-17 11:40 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
patch (dropped uninformative parameter names) (8.02 KB, patch)
2011-06-17 11:45 PDT, Andrey Kosyakov
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.35 MB, application/zip)
2011-06-17 14:06 PDT, WebKit Review Bot
no flags Details
patch (23.77 KB, patch)
2011-06-28 06:05 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
patch (8.54 KB, patch)
2011-06-29 05:28 PDT, Andrey Kosyakov
pfeldman: review-
Details | Formatted Diff | Diff
patch (8.66 KB, patch)
2011-06-29 07:23 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 Andrey Kosyakov 2011-06-17 11:38:09 PDT
For multi-process browsers, inspector front-end may communicate with agents residing in various processes, e.g. in case of navigation. To avoid necessity to clean up all object maps upon navigation and simplify client implementation, it is suggested that multi-process browsers start providing system-unique object identifiers.

This patch is sort of proof of concept, adding composite identifiers for frames only. This needs eventually be done for other objects (perhaps on the generated protocol stubs level).
Comment 1 Andrey Kosyakov 2011-06-17 11:40:59 PDT
Created attachment 97627 [details]
patch
Comment 2 WebKit Review Bot 2011-06-17 11:44:52 PDT
Attachment 97627 [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/InspectorPageAgent.h:108:  The parameter name "identifier" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Andrey Kosyakov 2011-06-17 11:45:04 PDT
Created attachment 97628 [details]
patch (dropped uninformative parameter names)
Comment 4 WebKit Review Bot 2011-06-17 14:06:03 PDT
Comment on attachment 97628 [details]
patch (dropped uninformative parameter names)

Attachment 97628 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8877720

New failing tests:
inspector/debugger/open-close-open.html
inspector/styles/styles-history.html
Comment 5 WebKit Review Bot 2011-06-17 14:06:08 PDT
Created attachment 97650 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 6 Andrey Kosyakov 2011-06-20 08:38:33 PDT
Comment on attachment 97628 [details]
patch (dropped uninformative parameter names)

As discussed offline, will make the id generation/parsing part of the generated protocol stubs/proxies.
Comment 7 Andrey Kosyakov 2011-06-28 06:05:23 PDT
Created attachment 98904 [details]
patch
Comment 8 Andrey Kosyakov 2011-06-29 05:28:45 PDT
Created attachment 99077 [details]
patch

As discussed offline: we don't want composite ids to be processed on the protocol level, as some objects are built on the application layer (by the back-end)
This is a modification of the first approach -- we just maintain composite string ids for the frames on the back-end as well.
Comment 9 Pavel Feldman 2011-06-29 05:32:07 PDT
Comment on attachment 99077 [details]
patch

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

A couple of nits, otherwise looks ok.

> Source/WebCore/inspector/InspectorController.cpp:352
> +void InspectorController::setAgentProcessIdentifier(unsigned int identifier)

WebCore should not operate 'processes'. Please come up with a neutral name (AgentUniqueIdentifier?). Also it should probably be a string.
Comment 10 Andrey Kosyakov 2011-06-29 07:23:22 PDT
Created attachment 99088 [details]
patch

Renamed AgentProcessIdentifier to AgentIdentifierPrefix, changed it to string.
Comment 11 WebKit Review Bot 2011-06-29 07:26:11 PDT
Attachment 99088 [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/InspectorController.h:96:  The parameter name "prefix" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Andrey Kosyakov 2011-07-01 05:25:31 PDT
Manually committed r90017: http://trac.webkit.org/changeset/90017