801605 - Scratchpad does not need NetUtils.asyncCopy

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P3)

Description

[David Teller [:Yoric] - still alive but not very active]

This does not seem to be on a critical path.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

If anybody wishes to pick this bug, the culprit is method |exportToFile| in browser/devtools/scratchpad/scratchpad.js

Comment 2

[Abhishek Potnis [:avp]]

I am currently busy with my exams. Will be working on this bug from December 17 2012.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Abhishek, are you working on it? Otherwise, there's another volunteer to pick it up.

Comment 4

[Abhishek Potnis [:avp]]

I am working on it. Will upload the patch by tomorrow.

Comment 5

[Abhishek Potnis [:avp]]

Attachment: Edited |exportToFile| function

I have partially edited the |exportToFile| function. I am still not clear on the working of promises, though I read the documentation.

    promise.then(function success(value) {

    }, function failure(reason) {

    });

I have understood that the first argument in the |then| function is the fulfilledHandler and is executed on Success i.e fulfilment of the promise but how do I implement it ? I mean in the above code snippet what must the |value| be ? Same for the second argument which is the errorHandler.

In the earlier code, on failure, this line was being executed : window.alert(self.strings.GetStringFromName("saveFile.failed"));
So should I place this in the function (failure) {....} ?

Also I did not understand these lines:
if (aCallback) {
        aCallback.call(self, aStatus);
      }

Please help me here.

Thanks.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

> I have understood that the first argument in the |then| function is the fulfilledHandler and is executed on Success i.e fulfilment of the promise but how do I implement it ? I mean in the above code snippet what must the |value| be ? Same for the second argument which is the errorHandler.

Well, |value| and |reason| are given to you by |promise|. Not sure I understand the rest of the question.

> In the earlier code, on failure, this line was being executed : window.alert(self.strings.GetStringFromName("saveFile.failed"));
So should I place this in the function (failure) {....} ?
Yes, I believe that the alert should go into |function failure(...) { ... }|.

> Also I did not understand these lines:
Maybe you should read the documentation of |call|: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Function/call

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 704914 [details] [diff] [review]
Edited |exportToFile| function

Review of attachment 704914 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/scratchpad/scratchpad.js
@@ +610,5 @@
>  
> +    let encoder = new TextEncoder();
> +    let array = encoder.encode(this.getText());
> +    let promise = OS.File.writeAtomic(aFile, array,
> +    {tmpPath: aFile+".tmp"});

Nit: add two whitespaces before '{' and a whitespace on each side of '+'

Comment 8

[Abhishek Potnis [:avp]]

Attachment: Edited |exportToFile| function

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 708101 [details] [diff] [review]
Edited |exportToFile| function

Review of attachment 708101 [details] [diff] [review]:
-----------------------------------------------------------------

Good for me, if it passes Try and with the following changes.

Also, the title of this patch should be clearer, e.g. "Bug 801605 - Getting rid of NetUtils.asyncCopy in Scratchpad".

We'll need review from one of the owners of devtools.

::: browser/devtools/scratchpad/scratchpad.js
@@ +643,5 @@
>        return;
>      }
>  
> +    let encoder = new TextEncoder();
> +    let array = encoder.encode(this.getText());

Nit: |buffer| sounds better than |array|.

@@ +658,5 @@
> +         }
> +         if (aCallback) {
> +          aCallback.call(self, Components.results.NS_ERROR_UNEXPECTED);
> +         }
> +    });

Nit: Could you fix indentation? Two whitespaces per level.

Comment 10

[Mihai Sucan [:msucan]]

Comment on attachment 708101 [details] [diff] [review]
Edited |exportToFile| function

Review of attachment 708101 [details] [diff] [review]:
-----------------------------------------------------------------

Please address David's comments. A couple of nits below.

r+ with all of the comments addressed.

Thank you for the contribution!

::: browser/devtools/scratchpad/scratchpad.js
@@ +645,5 @@
>  
> +    let encoder = new TextEncoder();
> +    let array = encoder.encode(this.getText());
> +    let promise = OS.File.writeAtomic(aFile, array,
> +     {tmpPath: aFile + ".tmp"});

Two spaces for indentation here.

@@ +658,5 @@
> +         }
> +         if (aCallback) {
> +          aCallback.call(self, Components.results.NS_ERROR_UNEXPECTED);
> +         }
> +    });

Please use bind() for the functions instead of |self|.

Comment 11

[Abhishek Potnis [:avp]]

Attachment: Getting rid of NetUtils.asyncCopy in Scratchpad

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 710178 [details] [diff] [review]
Getting rid of NetUtils.asyncCopy in Scratchpad

Redirecting to msucan.

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 710178 [details] [diff] [review]
Getting rid of NetUtils.asyncCopy in Scratchpad

Thanks for the updates!

I'm going to land this patch tomorrow, while addressing some of my minor nits.

Comment 14

[Mihai Sucan [:msucan]]

Comment on attachment 710178 [details] [diff] [review]
Getting rid of NetUtils.asyncCopy in Scratchpad

Review of attachment 710178 [details] [diff] [review]:
-----------------------------------------------------------------

While the patch looks good, I am getting errors on my system:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug_651942_recent_files.js | uncaught exception - TypeError: Value [xpconnect wrapped nsILocalFile] cannot be converted to a pointer at resource://gre/modules/osfile/osfile_shared_allthreads.jsm:298
Stack trace:
    JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 1067
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-INFO | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug_651942_recent_files.js | Console message: [JavaScript Error: "TypeError: Value [xpconnect wrapped nsILocalFile] cannot be converted to a pointer" {file: "resource://gre/modules/osfile/osfile_shared_allthreads.jsm" line: 298}]
TEST-INFO | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug_651942_recent_files.js | Console message: [JavaScript Error: "[Exception... "Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIInputStream.available]"  nsresult: "0x80470002 (NS_BASE_STREAM_CLOSED)"  location: "JS frame :: chrome://browser/content/scratchpad.js :: SP_importFromFile/< :: line 693"  data: no]" {file: "chrome://browser/content/scratchpad.js" line: 693}]


I wanted to land the patch now, with very small code style changes, but I cannot land it due to these failures. Did you test the patch? Please look into them.

Below you have some coding style nits and some questions/concerns about the patch.

::: browser/devtools/scratchpad/scratchpad.js
@@ +645,5 @@
>  
> +    let encoder = new TextEncoder();
> +    let buffer = encoder.encode(this.getText());
> +    let promise = OS.File.writeAtomic(aFile, buffer,
> +      {tmpPath: aFile + ".tmp"});

Nit: does this need to be on a new line?

Does aFile + ".tmp" do what you expect here? Does it stringify correctly?

@@ +650,5 @@
> +    promise.then((function success(value) {
> +         if (aCallback) {
> +          aCallback(Components.results.NS_OK);
> +         }
> +    }).bind(this), (function failure(reason) {

nit 1: please only indent the function bodies with two spaces.
nit 2: you do not need to wrap the function in parentheses. You can do: function() { }.bind(this).

@@ +655,5 @@
> +         if (!aSilentError) {
> +           window.alert(self.strings.GetStringFromName("saveFile.failed"));
> +         }
> +         if (aCallback) {
> +           aCallback(Components.results.NS_ERROR_UNEXPECTED);

You are changing the way callbacks work. aCallback was invoked with a specific |this|. Please change this line to aCallcallback.call(this, ....). Same goes for the other lines where you invoke aCallback.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 710178 [details] [diff] [review]
Getting rid of NetUtils.asyncCopy in Scratchpad

Review of attachment 710178 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/scratchpad/scratchpad.js
@@ +645,5 @@
>  
> +    let encoder = new TextEncoder();
> +    let buffer = encoder.encode(this.getText());
> +    let promise = OS.File.writeAtomic(aFile, buffer,
> +      {tmpPath: aFile + ".tmp"});

I'm almost certain that this should be aFile.path + ".tmp".

Comment 16

[Marco Castelluccio [:marco]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #15)
> Comment on attachment 710178 [details] [diff] [review]
> Getting rid of NetUtils.asyncCopy in Scratchpad
> 
> Review of attachment 710178 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/scratchpad/scratchpad.js
> @@ +645,5 @@
> >  
> > +    let encoder = new TextEncoder();
> > +    let buffer = encoder.encode(this.getText());
> > +    let promise = OS.File.writeAtomic(aFile, buffer,
> > +      {tmpPath: aFile + ".tmp"});
> 
> I'm almost certain that this should be aFile.path + ".tmp".

And also OS.File.writeAtomic(aFile.path

Comment 17

[Abhishek Potnis [:avp]]

Attachment: Getting rid of NetUtils.asyncCopy in Scratchpad

Green on try ! 

https://tbpl.mozilla.org/?tree=Try&rev=30ad3b4d7ab7
https://tbpl.mozilla.org/?tree=Try&rev=40bf4407d659

Comment 18

[Mihai Sucan [:msucan]]

Comment on attachment 717505 [details] [diff] [review]
Getting rid of NetUtils.asyncCopy in Scratchpad

Review of attachment 717505 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the update. One last problem, see below.

::: browser/devtools/scratchpad/scratchpad.js
@@ +655,1 @@
>          window.alert(self.strings.GetStringFromName("saveFile.failed"));

s/self/this/

(self is always undefined)

Comment 19

[Abhishek Potnis [:avp]]

Attachment: Getting rid of NetUtils.asyncCopy in Scratchpad

Comment 20

[Mihai Sucan [:msucan]]

Comment on attachment 718186 [details] [diff] [review]
Getting rid of NetUtils.asyncCopy in Scratchpad

Thanks for the update.

Comment 21

[Mihai Sucan [:msucan]]

Landed:
https://hg.mozilla.org/integration/fx-team/rev/72b1355623c7

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/72b1355623c7