Bug 19867 - Inspector should support $(id) in the command line
Summary: Inspector should support $(id) in the command line
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Keishi Hattori
URL:
Keywords: InRadar
Depends on:
Blocks: 14355
  Show dependency treegraph
 
Reported: 2008-07-03 06:33 PDT by Keishi Hattori
Modified: 2008-08-11 20:58 PDT (History)
4 users (show)

See Also:


Attachments
patch (2.15 KB, patch)
2008-07-07 06:03 PDT, Keishi Hattori
timothy: review-
Details | Formatted Diff | Diff
test if "this" changes (140.78 KB, image/png)
2008-08-07 07:25 PDT, Keishi Hattori
no flags Details
patch (2.65 KB, patch)
2008-08-11 07:07 PDT, Keishi Hattori
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2008-07-03 06:33:07 PDT
Inspector should support $(id) in the command line for Firebug parity.
Comment 1 Keishi Hattori 2008-07-04 03:03:58 PDT
Putting the code below into Console._evalInInspectedWindow kind of works. I'll investigate a better way though.

        expression = "(function () { \
            var $ = $ || function(id) { return document.getElementById(id); }; \
            var $$ = $$ || function(cl) { return document.getElementsByClassName(cl); }; \
            var $x = $x || function(xpath, context) { \
                var nodes = []; \
                try { \
                    var doc = context || document; \
                    var results = doc.evaluate(xpath, doc, null, XPathResult.ANY_TYPE, null); \
                    var node; \
                    while (node = results.iterateNext()) nodes.push(node); \
                } catch (e) {} \
                return nodes; \
            }; \
            var keys = keys || function(o) { var a = []; for (k in o) a.push(k); return a; }; \
            var values = values || function(o) { var a = []; for (k in o) a.push(o[k]); return a; }; \
            return (" + expression + ")})();";
Comment 2 Anthony Ricaud 2008-07-04 03:09:38 PDT
Just a comment about $$. It should be a wrapper for document.querySelectorAll instead of document.getElementsByClassName.
Comment 3 Keishi Hattori 2008-07-04 03:29:27 PDT
Sorry, thats a mistake.
Comment 4 Keishi Hattori 2008-07-04 07:15:08 PDT
I realized this is horribly wrong. I can't even do for loops.
Comment 5 Keishi Hattori 2008-07-07 06:03:35 PDT
Created attachment 22127 [details]
patch

I used "with" and it works fairly well.
Comment 6 Adam Roben (:aroben) 2008-07-10 08:12:22 PDT
Comment on attachment 22127 [details]
patch

+        expression = "with (testAPI) { " + expression + " }";

I think using with like this will change the meaning of "this" in the user's expression. It should be pretty easy to test this. We definitely don't want to change "this"!

It looks like Firebug uses window.__scope__, which I assume is a Mozilla-specific extension. Maybe Geoff Garen would have some ideas about how best to accomplish this.
Comment 7 Adam Roben (:aroben) 2008-07-11 14:06:30 PDT
<rdar://problem/6070209>
Comment 8 Adam Roben (:aroben) 2008-08-07 07:23:41 PDT
(In reply to comment #6)
> (From update of attachment 22127 [details] [edit])
> +        expression = "with (testAPI) { " + expression + " }";
> 
> I think using with like this will change the meaning of "this" in the user's
> expression. It should be pretty easy to test this. We definitely don't want to
> change "this"!

Keishi tells me that I was wrong and this approach does not modify the value of "this". That's great!

Geoff, would you mind commenting on the approach here?
Comment 9 Adam Roben (:aroben) 2008-08-07 07:24:07 PDT
Comment on attachment 22127 [details]
patch

Putting back to r? and marking Geoff as a potential reviewer.
Comment 10 Keishi Hattori 2008-08-07 07:25:22 PDT
Created attachment 22695 [details]
test if "this" changes

I ran these commands in the command line and this is what I got. I don't think "this" changes.

$("gbar");
$$("p");
$x("/html/body/div");
keys(console);
values(console);
window===this
this
Comment 11 Timothy Hatcher 2008-08-07 09:05:08 PDT
Comment on attachment 22127 [details]
patch

You should rename testAPI to be more unique. Like __inspectorConsoleAPI.

Also it might be better to explicitly assign __inspectorConsoleAPI t the window instead f using var.

So: window. __inspectorConsoleAPI = ...
Comment 12 Keishi Hattori 2008-08-11 07:07:21 PDT
Created attachment 22727 [details]
patch

I've added profile/profileEnd but they don't work quite well. It picks up the anonymous function that it's wrapped with. Also the result doesn't get updated. Maybe profile not getting updated should be a bug on it's own.
Comment 13 Timothy Hatcher 2008-08-11 20:40:10 PDT
Comment on attachment 22727 [details]
patch

+        if (!inspectedWindow._inspectorCommandLineAPI)

You should put braces around that if block, since it is multiple lines.

Also you should improve the ChangeLog to call out what functions you are changing and add more description before the Reviewed by line.

The < > and commas aren't needed for the bug list. Just put one per line.

Please file a bug about the profiler issues you mentioned.
Comment 14 Timothy Hatcher 2008-08-11 20:58:02 PDT
Landed in r35676.