Bug 31238 – Web Inspector: Inspector should support copy() in the command line

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Keishi Hattori]

I found an undocumented but useful Firebug Command Line API, copy(). It copies the argument to the pasteboard.

[Keishi Hattori]

Created attachment 42711 [details]
proposed patch

I had to use a horrific hack to get it to work. Is there a better way?

[Keishi Hattori]

Created attachment 42713 [details]
proposed patch 2

added comment and added line breaks to the eval string.

[Pavel Feldman]

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...

[Keishi Hattori]

(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?

[Pavel Feldman]

(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.

[Keishi Hattori]

Created attachment 42730 [details]
proposed patch 3

Thanks I get it now. So pushNodePathToFrontend does the mapping.

[Pavel Feldman]

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+!

[Pavel Feldman]

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).

[Keishi Hattori]

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.

[WebKit Commit Bot]

Comment on attachment 42734 [details]
proposed patch 4

Clearing flags on attachment: 42734

Committed r50639: <http://trac.webkit.org/changeset/50639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.