DevTools: Promisify WebInspector.TempFile

DevTools: Promisify WebInspector.TempFile

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185201

[loislo, 2014-11-06 15:42:07]

loislo@chromium.org changed reviewers:
+ pfeldman@chromium.org, vsevik@chromium.org

[loislo, 2014-11-07 10:23:25]

loislo@chromium.org changed reviewers:
+ yurys@chromium.org

[loislo, 2014-11-07 10:23:26]

ptal

[pfeldman, 2014-11-07 10:41:10]

Why is this necessary? Does it fix the remote debugging bug?

[apavlov, 2014-11-07 11:06:02]

apavlov@chromium.org changed reviewers:
+ apavlov@chromium.org

[apavlov, 2014-11-07 11:06:03]

https://codereview.chromium.org/705063004/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/bindings/TempFile.js (right):

https://codereview.chromium.org/705063004/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:372: var worker =
Runtime.startSharedWorker("temp_storage_shared_worker", "TempStorage");
I'm about to promisify the worker startup (see
https://chromiumcodereview.appspot.com/685203003/)

https://codereview.chromium.org/705063004/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:374: function handleError(event)
Please annotate things while you are here

[loislo, 2014-11-10 08:56:30]

It is plain promisification and it doesn't relate to the bug with remote
debugging.

https://codereview.chromium.org/705063004/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/bindings/TempFile.js (right):

https://codereview.chromium.org/705063004/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:374: function handleError(event)
On 2014/11/07 11:06:03, apavlov wrote:
> Please annotate things while you are here

Done.

[yurys, 2014-11-10 16:29:30]

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/bindings/TempFile.js (right):

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:61: return new
Promise(fs.root.getDirectory.bind(fs.root, dirPath, { create: true }));
I'd rather make it more verbose and arguably more readable:

new Promise(function(resolve, reject)
{
   fs.root.getDirectory(dirPath, { create: true }, resolve, reject);
});

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:124: .then(truncateFile);
Where is catch? There should still be errorHandler, otherwise we will never see
this error in the DevTools front-end.

[loislo, 2014-11-10 21:02:17]

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/bindings/TempFile.js (right):

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:61: return new
Promise(fs.root.getDirectory.bind(fs.root, dirPath, { create: true }));
On 2014/11/10 16:29:30, yurys wrote:
> I'd rather make it more verbose and arguably more readable:
> 
> new Promise(function(resolve, reject)
> {
>    fs.root.getDirectory(dirPath, { create: true }, resolve, reject);
> });

it would be too wordy.

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:124: .then(truncateFile);
On 2014/11/10 16:29:30, yurys wrote:
> Where is catch? There should still be errorHandler, otherwise we will never
see
> this error in the DevTools front-end.

as far as I understand .catch statement eats the rejection and returns
fulfilled(resolve) promise

[yurys, 2014-11-10 21:34:45]

On 2014/11/10 21:02:17, loislo wrote:
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> File Source/devtools/front_end/bindings/TempFile.js (right):
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:61: return new
> Promise(fs.root.getDirectory.bind(fs.root, dirPath, { create: true }));
> On 2014/11/10 16:29:30, yurys wrote:
> > I'd rather make it more verbose and arguably more readable:
> > 
> > new Promise(function(resolve, reject)
> > {
> >    fs.root.getDirectory(dirPath, { create: true }, resolve, reject);
> > });
> 
> it would be too wordy.
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:124: .then(truncateFile);
> On 2014/11/10 16:29:30, yurys wrote:
> > Where is catch? There should still be errorHandler, otherwise we will never
> see
> > this error in the DevTools front-end.
> 
> as far as I understand .catch statement eats the rejection and returns
> fulfilled(resolve) promise

Correct and I don't see how it contradicts my suggestion. If there is no 
rejection handler the exception will settle in the console log which can only be
viewed by opening DevTools for DevTools while we'd like to see the error in the
original DevTools window.

[yurys, 2014-11-10 21:34:47]

On 2014/11/10 21:02:17, loislo wrote:
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> File Source/devtools/front_end/bindings/TempFile.js (right):
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:61: return new
> Promise(fs.root.getDirectory.bind(fs.root, dirPath, { create: true }));
> On 2014/11/10 16:29:30, yurys wrote:
> > I'd rather make it more verbose and arguably more readable:
> > 
> > new Promise(function(resolve, reject)
> > {
> >    fs.root.getDirectory(dirPath, { create: true }, resolve, reject);
> > });
> 
> it would be too wordy.
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:124: .then(truncateFile);
> On 2014/11/10 16:29:30, yurys wrote:
> > Where is catch? There should still be errorHandler, otherwise we will never
> see
> > this error in the DevTools front-end.
> 
> as far as I understand .catch statement eats the rejection and returns
> fulfilled(resolve) promise

Correct and I don't see how it contradicts my suggestion. If there is no 
rejection handler the exception will settle in the console log which can only be
viewed by opening DevTools for DevTools while we'd like to see the error in the
original DevTools window.

[loislo, 2014-11-11 08:11:17]

On 2014/11/10 21:34:47, yurys wrote:
> On 2014/11/10 21:02:17, loislo wrote:
> >
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> > File Source/devtools/front_end/bindings/TempFile.js (right):
> > 
> >
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> > Source/devtools/front_end/bindings/TempFile.js:61: return new
> > Promise(fs.root.getDirectory.bind(fs.root, dirPath, { create: true }));
> > On 2014/11/10 16:29:30, yurys wrote:
> > > I'd rather make it more verbose and arguably more readable:
> > > 
> > > new Promise(function(resolve, reject)
> > > {
> > >    fs.root.getDirectory(dirPath, { create: true }, resolve, reject);
> > > });
> > 
> > it would be too wordy.
> > 
> >
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> > Source/devtools/front_end/bindings/TempFile.js:124: .then(truncateFile);
> > On 2014/11/10 16:29:30, yurys wrote:
> > > Where is catch? There should still be errorHandler, otherwise we will
never
> > see
> > > this error in the DevTools front-end.
> > 
> > as far as I understand .catch statement eats the rejection and returns
> > fulfilled(resolve) promise
> 
> Correct and I don't see how it contradicts my suggestion. If there is no 
> rejection handler the exception will settle in the console log which can only
be
> viewed by opening DevTools for DevTools while we'd like to see the error in
the
> original DevTools window.

With the patch the failed promise would be catch by the caller's rejection
handler
WebInspector.DeferredTempFile._failToCreateTempFile
which prints the error and does other necessary things.

I can add .catch handler right into TempFile and rethrow rejection with
return Promise.reject(e); so the caller would get the rejection anyway.

[yurys, 2014-11-11 09:41:04]

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/bindings/TempFile.js (right):

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:110:
WebInspector.console.error("Failed to truncate temp file " + e.code + " : " +
e.message);
Let's move all error handling to the client side. It should have more context to
decide whether the error message should be written to the console or just
swallowed.

[loislo, 2014-11-11 10:00:33]

On 2014/11/11 09:41:04, yurys wrote:
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> File Source/devtools/front_end/bindings/TempFile.js (right):
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:110:
> WebInspector.console.error("Failed to truncate temp file " + e.code + " : " +
> e.message);
> Let's move all error handling to the client side. It should have more context
to
> decide whether the error message should be written to the console or just
> swallowed.

done

[apavlov, 2014-11-11 10:11:05]

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/bindings/TempFile.js (right):

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:43: * @param {!string} dirPath
Please never use '!' with non-object types, as they are non-nullable by default.

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:44: * @param {!string} name
ditto

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:45: * @return {!Promise}
You need to specify the promise resolution type: {!Promise.<something>}

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:51: function
requestTempFileSystem()
Please annotate

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:53: return new
Promise(window.requestFileSystem.bind(window, window.TEMPORARY, 10));
How are you going to handle requestFileSystem failures? Previously, you had
errorHandler handle all failures, now you've got nothing. Ditto for the rest of
the FS calls.

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:67: function getFileEntry(dir)
no "get" prefix

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:98: function didTruncate(e)
Please annotate

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:106: function onTruncateError(e)
Please annotate

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:243:
.then(this._didCreateTempFile.bind(this),
this._failToCreateTempFile.bind(this));
_failedToCreateTempFile

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:273: _failToCreateTempFile:
function(e)
Please annotate

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:370:
WebInspector.TempFile._clearTempStorage = function(fulfill, reject)
Please annotate

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:397: var worker =
Runtime.startSharedWorker("temp_storage_shared_worker", "TempStorageCleaner");
Please rebase, as our workers now start asynchronously. This should become:

var worker = new WorkerRuntime.Worker("temp_storage_shared_worker",
"TempStorageCleaner");

You will also need to expose the actual worker promise inside
WorkerRuntime.Worker to handle its rejection (the thing I asked you yesterday).

https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/bindings/TempFile.js:410: * @return {!Promise}
Please specify the Promise resolution type. If there is none, say
{!Promise.<undefined>}

[loislo, 2014-11-12 08:36:59]

On 2014/11/11 10:11:05, apavlov wrote:
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> File Source/devtools/front_end/bindings/TempFile.js (right):
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:43: * @param {!string} dirPath
> Please never use '!' with non-object types, as they are non-nullable by
default.
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:44: * @param {!string} name
> ditto
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:45: * @return {!Promise}
> You need to specify the promise resolution type: {!Promise.<something>}
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:51: function
> requestTempFileSystem()
> Please annotate
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:53: return new
> Promise(window.requestFileSystem.bind(window, window.TEMPORARY, 10));
> How are you going to handle requestFileSystem failures? Previously, you had
> errorHandler handle all failures, now you've got nothing. Ditto for the rest
of
> the FS calls.
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:67: function getFileEntry(dir)
> no "get" prefix
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:98: function didTruncate(e)
> Please annotate
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:106: function
onTruncateError(e)
> Please annotate
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:243:
> .then(this._didCreateTempFile.bind(this),
> this._failToCreateTempFile.bind(this));
> _failedToCreateTempFile
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:273: _failToCreateTempFile:
> function(e)
> Please annotate
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:370:
> WebInspector.TempFile._clearTempStorage = function(fulfill, reject)
> Please annotate
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:397: var worker =
> Runtime.startSharedWorker("temp_storage_shared_worker", "TempStorageCleaner");
> Please rebase, as our workers now start asynchronously. This should become:
> 
> var worker = new WorkerRuntime.Worker("temp_storage_shared_worker",
> "TempStorageCleaner");
> 
> You will also need to expose the actual worker promise inside
> WorkerRuntime.Worker to handle its rejection (the thing I asked you
yesterday).
> 
>
https://codereview.chromium.org/705063004/diff/40001/Source/devtools/front_en...
> Source/devtools/front_end/bindings/TempFile.js:410: * @return {!Promise}
> Please specify the Promise resolution type. If there is none, say
> {!Promise.<undefined>}

Looks like Promise specification in closure compiler doesn't work properly.
I'd upload another patch with annotations which fails on closure compilation.

[yurys, 2014-11-12 10:29:06]

lgtm with nits

https://codereview.chromium.org/705063004/diff/100001/LayoutTests/inspector/p...
File LayoutTests/inspector/profiler/temp-storage-cleaner.html (right):

https://codereview.chromium.org/705063004/diff/100001/LayoutTests/inspector/p...
LayoutTests/inspector/profiler/temp-storage-cleaner.html:15: .catch(function(e)
just pass it as second argument to "then" above

https://codereview.chromium.org/705063004/diff/100001/Source/devtools/front_e...
File Source/devtools/front_end/bindings/TempFile.js (right):

https://codereview.chromium.org/705063004/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/bindings/TempFile.js:118: .then(didTruncate)
.then(didTruncate, onTruncateError);

[loislo, 2014-11-12 10:37:10]

The CQ bit was checked by loislo@chromium.org

[commit-bot: I haz the power, 2014-11-12 10:38:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/705063004/120001

[commit-bot: I haz the power, 2014-11-12 11:48:02]

Committed patchset #7 (id:120001) as 185201