RESOLVED INVALID71017
Web Inspector: introduce script origin and push it into the inspector. 
https://bugs.webkit.org/show_bug.cgi?id=71017
Summary Web Inspector: introduce script origin and push it into the inspector.
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
DetailsFormatted DiffDiff
[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-
DetailsFormatted DiffDiff
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.