Bug 43847 - Web Inspector: brush up object proxies, introduce remote object model.
Summary: Web Inspector: brush up object proxies, introduce remote object model.
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: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-11 05:56 PDT by Pavel Feldman
Modified: 2010-08-12 12:51 PDT (History)
11 users (show)

See Also:


Attachments
[PATCH] Proposed change. (47.80 KB, patch)
2010-08-11 08:11 PDT, Pavel Feldman
yurys: review-
Details | Formatted Diff | Diff
[PATCH] Review comments addressed. (58.51 KB, patch)
2010-08-12 05:33 PDT, Pavel Feldman
yurys: review-
Details | Formatted Diff | Diff
[PATCH] Review comments addressed. (59.76 KB, patch)
2010-08-12 06:23 PDT, Pavel Feldman
yurys: 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 2010-08-11 05:56:55 PDT
Patch to follow.
Comment 1 Pavel Feldman 2010-08-11 08:11:51 PDT
Created attachment 64116 [details]
[PATCH] Proposed change.
Comment 2 Yury Semikhatsky 2010-08-11 23:58:13 PDT
Comment on attachment 64116 [details]
[PATCH] Proposed change.

WebCore/inspector/front-end/RemoteObject.js:2
 +   * Copyright (C) 2009 Google Inc. All rights reserved.
2009->2010

WebCore/inspector/front-end/RemoteObject.js:55
 +      message._type = "error";
Just pass it as argument to the constructor.

WebCore/inspector/front-end/RemoteObject.js:64
 +  WebInspector.RemoteObject.fromPayload = function(payload)
Maybe fromProtocolMessage or something else more descriptive than just payload?

WebCore/inspector/front-end/RemoteObject.js:68
 +      // FIXME: make sure we only get here with real payloads in the new DebuggerAgent.js.
There is no DebuggerAgent.js, please change the comment.

WebCore/inspector/front-end/RemoteObject.js:117
 +                      result[propertiesPayload[i].name] = propertiesPayload[i].value.description;
It should return WebInspector.RemoteObjects as values. Otherwise rename the method to getPropertyValueDescriptions. r- for this.

WebCore/inspector/front-end/PropertiesSidebarPane.js:61
 +          InjectedScriptAccess.get(-node.id).getPrototypes(node.id, callback);
Please encapsulate getting injected script id by node id into a method.

WebCore/inspector/front-end/InjectedScript.js:543
 +          if (typeof obj.length == "number")
== -> ===

WebCore/inspector/front-end/InjectedScript.js:412
 +      var description = InjectedScript._describe(object, abbreviate);
This call was wrapped in try/catch, why did you decide to remove it?
Comment 3 Pavel Feldman 2010-08-12 05:33:28 PDT
Created attachment 64213 [details]
[PATCH] Review comments addressed.
Comment 4 Yury Semikhatsky 2010-08-12 06:20:50 PDT
Comment on attachment 64213 [details]
[PATCH] Review comments addressed.

WebCore/inspector/front-end/ConsoleView.js:593
 +      },
please revert InjectedScriptAccess.get change as discussed offline

WebCore/inspector/front-end/EventListenersSidebarPane.js:197
 +                  value = WebInspector.RemoveObject.fromNode(value.id);
Please fix this line. r- for that
Comment 5 Pavel Feldman 2010-08-12 06:23:11 PDT
Created attachment 64216 [details]
[PATCH] Review comments addressed.
Comment 6 Pavel Feldman 2010-08-12 06:32:55 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	D	WebCore/inspector/front-end/ObjectProxy.js
	M	LayoutTests/ChangeLog
	M	LayoutTests/inspector/console-dir-expected.txt
	M	LayoutTests/inspector/console-dir-global.html
	M	LayoutTests/inspector/console-dir.html
	M	LayoutTests/inspector/console-format-collections-expected.txt
	M	LayoutTests/inspector/console-format-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/inspector/front-end/AuditRules.js
	M	WebCore/inspector/front-end/ConsoleView.js
	M	WebCore/inspector/front-end/DOMAgent.js
	M	WebCore/inspector/front-end/ElementsPanel.js
	M	WebCore/inspector/front-end/ElementsTreeOutline.js
	M	WebCore/inspector/front-end/EventListenersSidebarPane.js
	M	WebCore/inspector/front-end/InjectedScript.js
	M	WebCore/inspector/front-end/ObjectPropertiesSection.js
	M	WebCore/inspector/front-end/PropertiesSidebarPane.js
	M	WebCore/inspector/front-end/ScopeChainSidebarPane.js
	M	WebCore/inspector/front-end/ScriptsPanel.js
	M	WebCore/inspector/front-end/SourceFrame.js
	M	WebCore/inspector/front-end/WatchExpressionsSidebarPane.js
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/front-end/utilities.js
Committed r65241
Comment 7 WebKit Review Bot 2010-08-12 06:37:14 PDT
http://trac.webkit.org/changeset/65241 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release
Comment 8 Pavel Feldman 2010-08-12 08:06:08 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	D	WebCore/inspector/front-end/RemoteObject.js
	M	LayoutTests/ChangeLog
	M	LayoutTests/inspector/console-dir-expected.txt
	M	LayoutTests/inspector/console-dir-global.html
	M	LayoutTests/inspector/console-dir.html
	M	LayoutTests/inspector/console-format-collections-expected.txt
	M	LayoutTests/inspector/console-format-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/inspector/front-end/AuditRules.js
	M	WebCore/inspector/front-end/ConsoleView.js
	M	WebCore/inspector/front-end/DOMAgent.js
	M	WebCore/inspector/front-end/ElementsPanel.js
	M	WebCore/inspector/front-end/ElementsTreeOutline.js
	M	WebCore/inspector/front-end/EventListenersSidebarPane.js
	M	WebCore/inspector/front-end/InjectedScript.js
	M	WebCore/inspector/front-end/ObjectPropertiesSection.js
	A	WebCore/inspector/front-end/ObjectProxy.js
	M	WebCore/inspector/front-end/PropertiesSidebarPane.js
	M	WebCore/inspector/front-end/ScopeChainSidebarPane.js
	M	WebCore/inspector/front-end/ScriptsPanel.js
	M	WebCore/inspector/front-end/SourceFrame.js
	M	WebCore/inspector/front-end/WatchExpressionsSidebarPane.js
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/front-end/utilities.js
Committed r65245
Comment 9 Eric Seidel (no email) 2010-08-12 11:28:42 PDT
If you wish to land manually, webkit-patch mark-bug-fixed will do all the bug updating logic for you. I tthink most people just use webkit-patch land these days though. Are there webkit-patch land issues you've run into?
Comment 10 Pavel Feldman 2010-08-12 12:51:22 PDT
(In reply to comment #9)
> If you wish to land manually, webkit-patch mark-bug-fixed will do all the bug updating logic for you. I tthink most people just use webkit-patch land these days though. Are there webkit-patch land issues you've run into?

It is mostly speed. I am using git and "git svn rebase" + "git svn dcommit" just works faster for me.
Comment 11 Pavel Feldman 2010-08-12 12:51:49 PDT
Landed as r65248