Bug 44028 - Web Inspector: Make InjectedScript proto-based.
Summary: Web Inspector: Make InjectedScript proto-based.
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
Depends on: 44055
  Show dependency treegraph
Reported: 2010-08-15 05:39 PDT by Pavel Feldman
Modified: 2010-08-16 09:34 PDT (History)
13 users (show)

See Also:

[PATCH] Proposed change. (38.28 KB, patch)
2010-08-15 05:45 PDT, Pavel Feldman
no flags 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-15 05:39:48 PDT
As a part of a 'better protocol' initiative, I am re-thinking the InjectedScript exposure. My present thinking is that InspectorDebugAgent (or new InspectorRuntimeAgent) will have 'installFunction' and 'evaluateInWindow' methods. Front-ends will install whatever functions they want into the injected script (like ours will install "getCompletions" and "getPrototypes"). The only functionality that will be 'pre-installed' in injected script is going to be wrapping and unwrapping of objects. Here is the first step towards that goal.
Comment 1 Pavel Feldman 2010-08-15 05:45:58 PDT
Created attachment 64445 [details]
[PATCH] Proposed change.
Comment 2 Yury Semikhatsky 2010-08-16 01:09:28 PDT
Comment on attachment 64445 [details]
[PATCH] Proposed change.

 +          result.type = typeof object;
Please construct the result using a factory method on InjectedScript.RemoteObject like you do in other places.

 +          https://bugs.webkit.org/show_bug.cgi?id=44028
Please add meaningful description for semantic changes in this patch, they are very hard to track among code formatting changes.

 +          Web Inspector: Make InjectedScript proto-based.
How does this change relate to the final goal of reducing number of predefined methods on the InjectedScript and introducing installFunction? We may well have installFunction defined on the InjectedScript instance as there is anyway the only one.
Comment 3 Pavel Feldman 2010-08-16 05:20:14 PDT
Comment on attachment 64445 [details]
[PATCH] Proposed change.

Clearing flags on attachment: 64445

Committed r65414: <http://trac.webkit.org/changeset/65414>
Comment 4 Pavel Feldman 2010-08-16 05:20:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 WebKit Review Bot 2010-08-16 06:03:18 PDT
http://trac.webkit.org/changeset/65414 might have broken Qt Linux Release
The following changes are on the blame list:
Comment 6 Csaba Osztrogonác 2010-08-16 07:03:58 PDT
Reopen, because it was rolled out by http://trac.webkit.org/changeset/65423
Comment 7 Pavel Feldman 2010-08-16 09:34:38 PDT
This has been rolled out and re-landed with the console test fixed (test had a race condition):

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	D	LayoutTests/http/tests/inspector-enabled/resources/console-log-before-frame-navigation.js
	M	LayoutTests/ChangeLog
	M	LayoutTests/http/tests/inspector-enabled/console-log-before-frame-navigation-expected.txt
	M	LayoutTests/http/tests/inspector-enabled/console-log-before-frame-navigation.html
	M	LayoutTests/http/tests/inspector-enabled/resources/console-log-frame-before-navigation.html
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/InjectedScript.js
Committed r65434