Bug 27939 – WebInspector: Extract style editing into a separate file that is going to be loaded in page context.

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

[Pavel Feldman]

WebInspector: Extract style editing into a separate file that is going to be loaded in page context.

How this new file should be called? It is going to live along with the InspectorController, will belong to the page context and will operate raw (non-quarantined) JS objects.

[Pavel Feldman]

Created attachment 33973 [details]
patch

[Timothy Hatcher]

But it isn't injected yet?

[Pavel Feldman]

(In reply to comment #2)
> But it isn't injected yet?

No, and that is something I need help with. What I need is to open this js file (and only this js file) in a page's context in a way that its objects are not visible to the page's object. It is very similar to the way frontend is being opened today. Any hints / pointers?

The plan is to come up with that context, get rid of wrappers for all calls to this injected script and when all the code that needs to be injected is migrated, remove universal access from the frontend. That is a long road. Makes sense?

[Timothy Hatcher]

Sam might have some thoughts on this.

[Eric Seidel (no email)]

Comment on attachment 33973 [details]
patch

are we going to have different rules for the InjectedScript.js file?  Will that be the only way to get script into the page's context?  I think it's good to divide things out like this so that it's easier to make sure the Inspector is "secure" long term.  My line of questioning mostly leads to the question of if this is going to be the only injected script file, and if InjectedScript.js is the right name if it's not.

[Pavel Feldman]

(In reply to comment #5)
> (From update of attachment 33973 [details])
> are we going to have different rules for the InjectedScript.js file?  Will that
> be the only way to get script into the page's context?  I think it's good to
> divide things out like this so that it's easier to make sure the Inspector is
> "secure" long term.  My line of questioning mostly leads to the question of if
> this is going to be the only injected script file, and if InjectedScript.js is
> the right name if it's not.

InjectedScript is going to be the only file containing the code that has access to the running page. All data is going to be serialized on the way to/from this injected script. It should not be too large (~1.5KLOC). I am not sure if that is a good name for it anyway, so I am open to suggestions.

While I am looking for the advice on how to organize this new context:
a) Continue extracting properties and search-related requests there
b) Create a separate html file referencing this script only and open it similarly to the front-end itself
c) Wait and see what Sam suggests
d) Something else?

[Joseph Pecoraro]

Just a heads up, due to bug 27928 landing this will need a small merge in order to apply to ToT.

[Pavel Feldman]

Created attachment 34134 [details]
patch

I updated the change to do all the edits for styles and metrics in a serialized way. I've also merged with the changes panel. I'd like to submit it if possible in order to avoid painful merging. I will not close the bug though unless this new file runs in a separate context. What do you think?

[Pavel Feldman]

Created attachment 34136 [details]
Same, but access to injected script is done via InspectorController

[Pavel Feldman]

Thanks for review!

Please do not land this since I would like to synchronize this with the Chromium build cycle.

[Adam Barth]

Comment on attachment 34136 [details]
Same, but access to injected script is done via InspectorController

Marking to avoid landing because of comment #11.

[Adam Barth]

Or rather Commit #10...  (Silly self-referential bug comments.)

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/DOMAgent.js
	A	WebCore/inspector/front-end/InjectedScript.js
	M	WebCore/inspector/front-end/MetricsSidebarPane.js
	M	WebCore/inspector/front-end/StylesSidebarPane.js
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/inspector/front-end/inspector.html
Committed r46841

Please do not close this bug - pending more patches.

[Pavel Feldman]

Created attachment 34210 [details]
move style-related utilities into InjectedScript

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/DOMAgent.js
	M	WebCore/inspector/front-end/InjectedScript.js
	M	WebCore/inspector/front-end/StylesSidebarPane.js
	M	WebCore/inspector/front-end/utilities.js
Committed r46848

Please do not close this bug - pending more patches.

[Adam Barth]

Comment on attachment 34210 [details]
move style-related utilities into InjectedScript

Please either close the bug or clear the review flag when you land a patch.  Otherwise, it show up in the query for things in need of a commit.  Our standard process is to use one bug per patch.  Bug numbers are cheap.  Don't be afraid of making a new one!