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.
Created attachment 123334 [details] patch
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 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.
(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.
> 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.
Created attachment 123563 [details] Updated patch
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);
Created attachment 123565 [details] patch Forgot to update ChangeLog
Comment on attachment 123565 [details] patch As per comments above.
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);
> copy not callback right? > DOMAgent.getOuterHTML(this.id, copy); Right.
Created attachment 123567 [details] patch
Comment on attachment 123567 [details] patch Clearing flags on attachment: 123567 Committed r105679: <http://trac.webkit.org/changeset/105679>
All reviewed patches have been landed. Closing bug.