Bug 31238 - Web Inspector: Inspector should support copy() in the command line
Summary: Web Inspector: Inspector should support copy() 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 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-08 02:20 PST by Keishi Hattori
Modified: 2009-11-08 22:39 PST (History)
8 users (show)

See Also:


Attachments
proposed patch (3.44 KB, patch)
2009-11-08 02:26 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff
proposed patch 2 (10.48 KB, patch)
2009-11-08 02:37 PST, Keishi Hattori
pfeldman: review-
Details | Formatted Diff | Diff
proposed patch 3 (10.90 KB, patch)
2009-11-08 21:14 PST, Keishi Hattori
pfeldman: review-
Details | Formatted Diff | Diff
proposed patch 4 (10.90 KB, patch)
2009-11-08 22:16 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2009-11-08 02:20:21 PST
I found an undocumented but useful Firebug Command Line API, copy(). It copies the argument to the pasteboard.
Comment 1 Keishi Hattori 2009-11-08 02:26:00 PST
Created attachment 42711 [details]
proposed patch

I had to use a horrific hack to get it to work. Is there a better way?
Comment 2 Keishi Hattori 2009-11-08 02:37:31 PST
Created attachment 42713 [details]
proposed patch 2

added comment and added line breaks to the eval string.
Comment 3 Pavel Feldman 2009-11-08 03:59:00 PST
Comment on attachment 42713 [details]
proposed patch 2

What you should do instead is:
- Introduce copyText in InspectorBackend.(idl, h, cpp). Just mimic copyNode there and pass text instead of node id.
- Implement it as Pasteboard::generalPasteboard()->writePlainText(text); (see copyNode again)
- Make a simple call to it from your new copy API method.

We could be even more cleaver here and copy markup in case selected object is node (make an instanceof check under copy and choose between copyText and copyNode to call). [object HTMLElement] is not too informative otherwise...
Comment 4 Keishi Hattori 2009-11-08 20:32:16 PST
(In reply to comment #3)
> (From update of attachment 42713 [details])
> What you should do instead is:
> - Introduce copyText in InspectorBackend.(idl, h, cpp). Just mimic copyNode
> there and pass text instead of node id.
> - Implement it as Pasteboard::generalPasteboard()->writePlainText(text); (see
> copyNode again)
> - Make a simple call to it from your new copy API method.
> 
> We could be even more cleaver here and copy markup in case selected object is
> node (make an instanceof check under copy and choose between copyText and
> copyNode to call). [object HTMLElement] is not too informative otherwise…

I was able to get the copyText method working. I wanted to use copyNode for nodes but I couldn't figure out how to get the node id from a node?
Comment 5 Pavel Feldman 2009-11-08 20:55:14 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 42713 [details] [details])
> > What you should do instead is:
> > - Introduce copyText in InspectorBackend.(idl, h, cpp). Just mimic copyNode
> > there and pass text instead of node id.
> > - Implement it as Pasteboard::generalPasteboard()->writePlainText(text); (see
> > copyNode again)
> > - Make a simple call to it from your new copy API method.
> > 
> > We could be even more cleaver here and copy markup in case selected object is
> > node (make an instanceof check under copy and choose between copyText and
> > copyNode to call). [object HTMLElement] is not too informative otherwise…
> 
> I was able to get the copyText method working. I wanted to use copyNode for
> nodes but I couldn't figure out how to get the node id from a node?

Oh, sorry about the confusion.

Actually, given that copy is executed in the injectedscript already, there is no need to go from id to node in order to copy it. Your argument is already a real node, no need to resolve it by id. But there is no copyNode(Node) method in InspectorBackend because it would be [Custom] and there were actually no use cases for it...

I think you should either forget my suggestion about calling copyNode or do

var nodeId = InspectorController.pushNodePathToFrontend(node, false /* selectInUI */);

It would give you id for a node, but as a side affect it will also push it into the frontend in case it was not there. Which is actually fine.
Comment 6 Keishi Hattori 2009-11-08 21:14:59 PST
Created attachment 42730 [details]
proposed patch 3

Thanks I get it now. So pushNodePathToFrontend does the mapping.
Comment 7 Pavel Feldman 2009-11-08 21:43:53 PST
Comment on attachment 42730 [details]
proposed patch 3

> +    void copyText(const String& text);
> +
>      // Generic code called from custom implementations.
>      void highlight(long nodeId);

Please move copyText into the "Generic code called from custom implementations." (under that comment).


> +    var inspectorCommandLineAPI = evalFunction.call(evalObject, "window.console._inspectorCommandLineAPI = { \n\
> +        $: function() { return document.getElementById.apply(document, arguments) }, \n\
> +        $$: function() { return document.querySelectorAll.apply(document, arguments) }, \n\
> +        $x: function(xpath, context) \n\
> +        { \n\

I assume that the only change here is \n in the end (please confirm)?

r- for the copyText outside the injectedscript section, otherwise r+!
Comment 8 Pavel Feldman 2009-11-08 21:51:24 PST
Sorry, I meant to move copyText into the 

// Called from InjectedScript.
// TODO: extract into a separate IDL.

section in IDL, not custom in header... (Too early here, too much copypate).
Comment 9 Keishi Hattori 2009-11-08 22:16:03 PST
Created attachment 42734 [details]
proposed patch 4

(In reply to comment #7)
> I assume that the only change here is \n in the end (please confirm)?

Changes to the eval string is \n and K&R style indentation.
Comment 10 WebKit Commit Bot 2009-11-08 22:39:41 PST
Comment on attachment 42734 [details]
proposed patch 4

Clearing flags on attachment: 42734

Committed r50639: <http://trac.webkit.org/changeset/50639>
Comment 11 WebKit Commit Bot 2009-11-08 22:39:46 PST
All reviewed patches have been landed.  Closing bug.