Bug 27939

Summary: WebInspector: Extract style editing into a separate file that is going to be loaded in page context.
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Pavel Feldman <pfeldman>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, fishd, joepeck, sam, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
none
patch
none
Same, but access to injected script is done via InspectorController
abarth: commit-queue-
move style-related utilities into InjectedScript none

Description Pavel Feldman 2009-08-03 07:05:45 PDT
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.
Comment 1 Pavel Feldman 2009-08-03 07:07:07 PDT
Created attachment 33973 [details]
patch
Comment 2 Timothy Hatcher 2009-08-03 08:35:00 PDT
But it isn't injected yet?
Comment 3 Pavel Feldman 2009-08-03 08:56:21 PDT
(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?
Comment 4 Timothy Hatcher 2009-08-03 09:00:31 PDT
Sam might have some thoughts on this.
Comment 5 Eric Seidel (no email) 2009-08-03 09:29:57 PDT
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.
Comment 6 Pavel Feldman 2009-08-03 10:02:57 PDT
(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?
Comment 7 Joseph Pecoraro 2009-08-03 14:56:23 PDT
Just a heads up, due to bug 27928 landing this will need a small merge in order to apply to ToT.
Comment 8 Pavel Feldman 2009-08-05 07:02:08 PDT
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?
Comment 9 Pavel Feldman 2009-08-05 08:35:01 PDT
Created attachment 34136 [details]
Same, but access to injected script is done via InspectorController
Comment 10 Pavel Feldman 2009-08-05 09:17:19 PDT
Thanks for review!

Please do not land this since I would like to synchronize this with the Chromium build cycle.
Comment 11 Adam Barth 2009-08-05 12:00:41 PDT
Comment on attachment 34136 [details]
Same, but access to injected script is done via InspectorController

Marking to avoid landing because of comment #11.
Comment 12 Adam Barth 2009-08-05 12:01:11 PDT
Or rather Commit #10...  (Silly self-referential bug comments.)
Comment 13 Pavel Feldman 2009-08-06 02:54:30 PDT
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.
Comment 14 Pavel Feldman 2009-08-06 08:09:15 PDT
Created attachment 34210 [details]
move style-related utilities into InjectedScript
Comment 15 Pavel Feldman 2009-08-06 09:40:07 PDT
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.
Comment 16 Adam Barth 2009-08-07 01:04:59 PDT
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!