Bug 71017

Summary:

Web Inspector: introduce script origin and push it into the inspector.

Product:

WebKit

Reporter:

Pavel Feldman <pfeldman>

Component:

Web Inspector (Deprecated)

Assignee:

Pavel Feldman <pfeldman>

Status:

RESOLVED INVALID

Severity:

Normal

CC:

abarth, apavlov, bweinstein, japhet, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
[Patch] Straw man implementation.	none
[Patch] Straw man with jsc bots happy.	abarth: review-, webkit-ews: commit-queue-

Pavel Feldman

Reported 2011-10-27 07:42:26 PDT

We need to distinguish scripts generated with document.write from the ones generated by the parser of the original resource. Otherwise we bind them to the resource and hence lose them from the debugging UI. Consider the following example: 1: <html> 2: <script>function aa() {}</script> 3: <script>function bb() {}</script> 4: <script>document.write("<scrip" + "t>\n\n\n\n\nfunction cc() {}</sc" + "ript>");</script> 5: <script>function dd() {}</script> 6: </html> As a result, following scripts are reported to be parsed (all sharing the main resource url): line 2-2 line 3-3 line 4-10 line 5-5 As a result of this patch, scripts will be annotated: line 2-2 (parser) line 3-3 (parser) line 4-10 (document.write) line 5-5 (parser) As a drive by, I annotate more script origins in this patch. Note that it will need additional support from the JSC to work properly.

Attachments
[Patch] Straw man implementation. (41.10 KB, patch) 2011-10-27 07:46 PDT, Pavel Feldman	no flags	Details Formatted Diff Diff
[Patch] Straw man with jsc bots happy. (41.99 KB, patch) 2011-10-27 09:00 PDT, Pavel Feldman	abarth: review- webkit-ews: commit-queue-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Pavel Feldman

Comment 1 2011-10-27 07:46:07 PDT

Created attachment 112680 [details] [Patch] Straw man implementation.

Gyuyoung Kim

Comment 2 2011-10-27 08:03:45 PDT

Comment on attachment 112680 [details] [Patch] Straw man implementation. Attachment 112680 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10227559

Early Warning System Bot

Comment 3 2011-10-27 08:10:57 PDT

Comment on attachment 112680 [details] [Patch] Straw man implementation. Attachment 112680 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10228541

Pavel Feldman

Comment 4 2011-10-27 09:00:37 PDT

Created attachment 112691 [details] [Patch] Straw man with jsc bots happy.

Early Warning System Bot

Comment 5 2011-10-27 09:13:53 PDT

Comment on attachment 112691 [details] [Patch] Straw man with jsc bots happy. Attachment 112691 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10224797

Yury Semikhatsky

Comment 6 2011-10-27 09:59:11 PDT

Comment on attachment 112691 [details] [Patch] Straw man with jsc bots happy. View in context: https://bugs.webkit.org/attachment.cgi?id=112691&action=review Overall looks good, but I don't think we need that many origin types if we don't discriminate between them anyway. > Source/WebKit/chromium/ChangeLog:3 > + clean Remove this. > Source/WebCore/inspector/front-end/Script.js:47 > + Remove this.

Adam Barth

Comment 7 2011-10-27 10:45:39 PDT

Comment on attachment 112691 [details] [Patch] Straw man with jsc bots happy. View in context: https://bugs.webkit.org/attachment.cgi?id=112691&action=review This patch does not contained any tests, as far as I can tell. > Source/WebCore/bindings/js/ScriptSourceCode.h:67 > + ScriptOrigin origin() const { return m_origin; } This is a really confusing name. I would have expected this to be the security origin of the script.

Adam Barth

Comment 8 2011-10-27 10:51:46 PDT

I'm not super excited about this patch because it imposes a lot of cost on WebCore. Now everywhere that constructs ScriptSourceCode needs to choose from this confusing enum menu. (Also, the name "origin" is likely to cause confusing w.r.t. SecurityOrigin.) Another approach that you might want to investigate is adding a setter for a property on ScriptSourceCode that you can call in the document.write case. That way not every who constructs a ScriptSourceCode needs to worry about this menu. (I'd also trim down the menu as much as possible---ideally just two or three options.) Another approach to consider is to view this information in terms of line numbers. In some sense, we shouldn't be reporting that this script executes on lines 4-10. That's really where the lie is. Instead, we should say that it's executing on some "additional" lines created at run-time (somehow). I'm not entirely sure where this approach leads, but it seems like more a property of the line numbers than of the script itself. Also, it's important to have tests for everything you change in a patch. I see that the patch is marked as a "straw man", so I assume you'll add tests in later iterations of the patch.

Pavel Feldman

Comment 9 2011-10-27 12:57:02 PDT

(In reply to comment #8) > I'm not super excited about this patch because it imposes a lot of cost on WebCore. Now everywhere that constructs ScriptSourceCode needs to choose from this confusing enum menu. (Also, the name "origin" is likely to cause confusing w.r.t. SecurityOrigin.) > > Another approach that you might want to investigate is adding a setter for a property on ScriptSourceCode that you can call in the document.write case. That way not every who constructs a ScriptSourceCode needs to worry about this menu. (I'd also trim down the menu as much as possible---ideally just two or three options.) I could do that. We have a dozen of ScriptSourceCode instantiations in WebKit though, so I don't think that making the new caller think about the ways his script ends up in the inspector is necessarily a bad thing. > Another approach to consider is to view this information in terms of line numbers. In some sense, we shouldn't be reporting that this script executes on lines 4-10. That's really where the lie is. Instead, we should say that it's executing on some "additional" lines created at run-time (somehow). I'm not entirely sure where this approach leads, but it seems like more a property of the line numbers than of the script itself. I actually started with this, but then I thought that it will change the web-facing API a bit (new Error().stack. window.onerror will change resulting values). I am actually in favor of this approach (i.e. in case of .write nesting level, reset start position provided into ScriptSourceCode to 0). > > Also, it's important to have tests for everything you change in a patch. I see that the patch is marked as a "straw man", so I assume you'll add tests in later iterations of the patch. I will surely provide coverage for whatever I intend to land.

Adam Barth

Comment 10 2011-10-27 13:28:35 PDT

> I actually started with this, but then I thought that it will change the web-facing API a bit (new Error().stack. window.onerror will change resulting values). I am actually in favor of this approach (i.e. in case of .write nesting level, reset start position provided into ScriptSourceCode to 0). That's probably ok. These APIs are all engine-specific anyway, really.

Pavel Feldman

Comment 11 2011-10-27 13:32:49 PDT

> That's probably ok. These APIs are all engine-specific anyway, really. I will proceed this way then. Thanks for your help.

Note You need to log in before you can comment on or make changes to this bug.