Bug 76706 – Web Inspector: Make "Copy as HTML" use the same copy functions as other copy methods.

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

[Konrad Piascik]

Copy as HTML currently performs its copying inside of webcore and doesn't copy any contents to the front-end if remote web inspector is attached.

[Konrad Piascik]

Created attachment 123334 [details]
patch

[Pavel Feldman]

Comment on attachment 123334 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123334&action=review

> Source/WebCore/inspector/front-end/ElementsPanel.js:937
> +        var copy = function(id, text) {

As I mentioned in the previous review:

- we only use named functions
- { should be on the next line
- this is not "id", this is "error" string

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:-1687
> -        this.representedObject.copyNode();

It sounds like you could leave implementation of copyNode in DOMAgent given that there are multiple call sites.

[Timothy Hatcher]

Comment on attachment 123334 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123334&action=review

>> Source/WebCore/inspector/front-end/ElementsPanel.js:937
>> +        var copy = function(id, text) {
> 
> As I mentioned in the previous review:
> 
> - we only use named functions
> - { should be on the next line
> - this is not "id", this is "error" string

And all he means by named is this:

function copy(error, text)
{
   // …
}

It will be the variable "copy" still.

[Konrad Piascik]

(In reply to comment #2)
> (From update of attachment 123334 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123334&action=review
> 
> > Source/WebCore/inspector/front-end/ElementsPanel.js:937
> > +        var copy = function(id, text) {
> 
> As I mentioned in the previous review:
> 
> - we only use named functions
> - { should be on the next line
> - this is not "id", this is "error" string
> 
> > Source/WebCore/inspector/front-end/ElementsTreeOutline.js:-1687
> > -        this.representedObject.copyNode();
> 
> It sounds like you could leave implementation of copyNode in DOMAgent given that there are multiple call sites.

Pavel,

2 things: 
1) I want to change the signature of DOMAgent.copyNode() then to not take a callback function since I'll be overwriting it with my own. Objections?

2) Should I be checking for an error inside the callback function copy?  If so what do you think would be the best course of action?  Console.log/error?

I'll put together a new patch with my own thoughts on how to deal with this in the meantime.

[Pavel Feldman]

> 1) I want to change the signature of DOMAgent.copyNode() then to not take a callback function since I'll be overwriting it with my own. Objections?
> 

Sounds good.

> 2) Should I be checking for an error inside the callback function copy?  If so what do you think would be the best course of action?  Console.log/error?
> 

This kind of error is expected due to asynchronous nature of this call (imagine that this node has just been deleted on the backend). So feel free to ignore it.

[Konrad Piascik]

Created attachment 123563 [details]
Updated patch

[Pavel Feldman]

Comment on attachment 123563 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123563&action=review

> Source/WebCore/inspector/front-end/DOMAgent.js:274
> +        var copy = function (errorString, text)

As I mentioned earlier, we only use named functions. It should be:

function copy(error, text)
{
    if (!error)
        InspectorFrontendHost.copyText(text);
}
DOMAgent.getOuterHTML(this.id, callback);

[Konrad Piascik]

Created attachment 123565 [details]
patch

Forgot to update ChangeLog

[Pavel Feldman]

Comment on attachment 123565 [details]
patch

As per comments above.

[Konrad Piascik]

Comment on attachment 123563 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123563&action=review

>> Source/WebCore/inspector/front-end/DOMAgent.js:274
>> +        var copy = function (errorString, text)
> 
> As I mentioned earlier, we only use named functions. It should be:
> 
> function copy(error, text)
> {
>     if (!error)
>         InspectorFrontendHost.copyText(text);
> }
> DOMAgent.getOuterHTML(this.id, callback);

copy not callback right?
DOMAgent.getOuterHTML(this.id, copy);

[Pavel Feldman]

> copy not callback right?
> DOMAgent.getOuterHTML(this.id, copy);

Right.

[Konrad Piascik]

Created attachment 123567 [details]
patch

[WebKit Review Bot]

Comment on attachment 123567 [details]
patch

Clearing flags on attachment: 123567

Committed r105679: <http://trac.webkit.org/changeset/105679>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.