Bug 76706

Summary: Web Inspector: Make "Copy as HTML" use the same copy functions as other copy methods.
Product: WebKit Reporter: Konrad Piascik <kpiascik>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, efidler, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
pfeldman: review-
Updated patch
none
patch
pfeldman: review-
patch none

Description Konrad Piascik 2012-01-20 08:28:28 PST
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.
Comment 1 Konrad Piascik 2012-01-20 09:06:40 PST
Created attachment 123334 [details]
patch
Comment 2 Pavel Feldman 2012-01-23 01:05:43 PST
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.
Comment 3 Timothy Hatcher 2012-01-23 06:36:05 PST
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.
Comment 4 Konrad Piascik 2012-01-23 06:48:44 PST
(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.
Comment 5 Pavel Feldman 2012-01-23 06:53:50 PST
> 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.
Comment 6 Konrad Piascik 2012-01-23 08:39:28 PST
Created attachment 123563 [details]
Updated patch
Comment 7 Pavel Feldman 2012-01-23 08:42:42 PST
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);
Comment 8 Konrad Piascik 2012-01-23 08:43:52 PST
Created attachment 123565 [details]
patch

Forgot to update ChangeLog
Comment 9 Pavel Feldman 2012-01-23 08:47:04 PST
Comment on attachment 123565 [details]
patch

As per comments above.
Comment 10 Konrad Piascik 2012-01-23 08:47:05 PST
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);
Comment 11 Pavel Feldman 2012-01-23 08:48:42 PST
> copy not callback right?
> DOMAgent.getOuterHTML(this.id, copy);

Right.
Comment 12 Konrad Piascik 2012-01-23 09:19:47 PST
Created attachment 123567 [details]
patch
Comment 13 WebKit Review Bot 2012-01-23 18:39:15 PST
Comment on attachment 123567 [details]
patch

Clearing flags on attachment: 123567

Committed r105679: <http://trac.webkit.org/changeset/105679>
Comment 14 WebKit Review Bot 2012-01-23 18:39:20 PST
All reviewed patches have been landed.  Closing bug.