Bug 76706 - Web Inspector: Make "Copy as HTML" use the same copy functions as other copy methods.
Summary: Web Inspector: Make "Copy as HTML" use the same copy functions as other copy ...
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: 2012-01-20 08:28 PST by Konrad Piascik
Modified: 2012-01-23 18:39 PST (History)
12 users (show)

See Also:


Attachments
patch (6.78 KB, patch)
2012-01-20 09:06 PST, Konrad Piascik
pfeldman: review-
Details | Formatted Diff | Diff
Updated patch (5.00 KB, patch)
2012-01-23 08:39 PST, Konrad Piascik
no flags Details | Formatted Diff | Diff
patch (4.85 KB, patch)
2012-01-23 08:43 PST, Konrad Piascik
pfeldman: review-
Details | Formatted Diff | Diff
patch (4.86 KB, patch)
2012-01-23 09:19 PST, Konrad Piascik
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.