dart2js: fetch http Resources

dart2js: fetch http Resources


BUG=
R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/bc0deb66d786e73b8ccdc01e15bdf3045c33533b

[Harry Terkelsen, 2015-08-21 23:02:02]

het@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2015-08-22 02:21:16]

very cool!

It would be great to add a couple tests for us too (unless there are any tests
covering the functionality other than the vm tests).

I'm not sure what would be the best way to cover tests for us. The only server
that is available to us is the default test server. The good news is that this
server supports loading files from the repo by providing a path of the form
`/root_dart/path/in/the/repo` (see for example xhr_test.dart
https://github.com/dart-lang/sdk/blob/da0363172adaa266ef2d44cf87e09b61a185e79...).

Could we use Resource to resolve to a URL like that?

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:580: var controller = new
StreamController();
couple comments:
- do we need to add an import to dart:async above?
- use a broadcast controller?

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:587: // single message. So the
stream will only get a single value posted.
seems like a good simplification for us. I guess we could potentially support
streaming if we use a streaming xhr under the hood, but it doesn't seem worth it
at this time. Not sure if we should add a TODO though. Up to you.

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:602: Future<List<int>>
_readAsBytes(Uri uri) {
consider using `=>`:
    Future<List<int>> _readAsBytes(Uri uri) =>
      _readAsString(uri).then((s) => s.codeUnits);

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:604: return data.codeUnits;
do we need to also handle results when the xhr.responseType is arraybuffer or
blob? (I expect responseText might be empty in those cases)

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:608: Future<String>
_readAsString(Uri uri) {
nit: use '=>' here too

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:609: return
readHttp(uri.toString());
nit: consider using '$uri' instead of uri.toString (it's equivalent, but shorter
:))

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/js_helper.dart (right):

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:3938: JS('void', '#.send()',
xhr);
+ return leavingFuture; ?

https://codereview.chromium.org/1308143002/diff/1/tests/corelib/corelib.status
File tests/corelib/corelib.status (left):

https://codereview.chromium.org/1308143002/diff/1/tests/corelib/corelib.statu...
tests/corelib/corelib.status:210: http_resource_test: Skip, OK # VM specific
test, uses dart:io.
undelete?

[Harry Terkelsen, 2015-08-25 00:03:57]

Thanks Siggi, PTAL

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:580: var controller = new
StreamController();
On 2015/08/22 02:21:16, Siggi Cherem (dart-lang) wrote:
> - do we need to add an import to dart:async above?
No, dart:core imports dart:async now
> - use a broadcast controller?
Done

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:587: // single message. So the
stream will only get a single value posted.
On 2015/08/22 02:21:15, Siggi Cherem (dart-lang) wrote:
> seems like a good simplification for us. I guess we could potentially support
> streaming if we use a streaming xhr under the hood, but it doesn't seem worth
it
> at this time. Not sure if we should add a TODO though. Up to you.

Done.

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:602: Future<List<int>>
_readAsBytes(Uri uri) {
On 2015/08/22 02:21:15, Siggi Cherem (dart-lang) wrote:
> consider using `=>`:
>     Future<List<int>> _readAsBytes(Uri uri) =>
>       _readAsString(uri).then((s) => s.codeUnits);

Done.

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:604: return data.codeUnits;
On 2015/08/22 02:21:15, Siggi Cherem (dart-lang) wrote:
> do we need to also handle results when the xhr.responseType is arraybuffer or
> blob? (I expect responseText might be empty in those cases)

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:608: Future<String>
_readAsString(Uri uri) {
On 2015/08/22 02:21:16, Siggi Cherem (dart-lang) wrote:
> nit: use '=>' here too

Done.

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:609: return
readHttp(uri.toString());
On 2015/08/22 02:21:16, Siggi Cherem (dart-lang) wrote:
> nit: consider using '$uri' instead of uri.toString (it's equivalent, but
shorter
> :))

Done.

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/js_helper.dart (right):

https://codereview.chromium.org/1308143002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:3938: JS('void', '#.send()',
xhr);
On 2015/08/22 02:21:16, Siggi Cherem (dart-lang) wrote:
> + return leavingFuture; ?

Done.

[Siggi Cherem (dart-lang), 2015-08-25 00:43:27]

nice!

Just minor things below, but otherwise lgtm!

https://codereview.chromium.org/1308143002/diff/40001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/js_helper.dart (right):

https://codereview.chromium.org/1308143002/diff/40001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:3918:
completer.future.whenComplete(() => leaveJsAsync());
I guess we can also remove the explicit closure :)

  completer.future.whenComplete(leaveJsAsync);

https://codereview.chromium.org/1308143002/diff/40001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:3954: return completer.future;
Question: I see this is also how it was done in _loadHunk. I was wondering
whether we need to chain things to avoid "forking" the future. In other words,
if you have:

  f = readHttp(...);
  f.then(doSomething);

do we care if doSomething executes before leaveJsAsync?

From a quick look at leaveJsAsync, it seems that we should be OK leaving it as
you have it now, but I just wanted to check

https://codereview.chromium.org/1308143002/diff/40001/tests/html/resource_htt...
File tests/html/resource_http_test.dart (right):

https://codereview.chromium.org/1308143002/diff/40001/tests/html/resource_htt...
tests/html/resource_http_test.dart:15: var url =
'/root_dart/tests/html/xhr_cross_origin_data.txt?'
let's use a new data file specific for this test, just to make it easier to
discover/maintain.

https://codereview.chromium.org/1308143002/diff/40001/tests/html/resource_htt...
tests/html/resource_http_test.dart:19: var data = JSON.decode(d);
then this could be a simple string equality check

https://codereview.chromium.org/1308143002/diff/40001/tests/html/resource_htt...
tests/html/resource_http_test.dart:28: r.readAsString().then(expectAsync((s) {
either use await or return the future to ensure we pause the test before moving
to the next one

https://codereview.chromium.org/1308143002/diff/40001/tests/html/resource_htt...
tests/html/resource_http_test.dart:34: r.readAsBytes().then(expectAsync((b) {
ditto

[Harry Terkelsen, 2015-08-25 17:45:06]

Thanks Siggi!

https://codereview.chromium.org/1308143002/diff/40001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/js_helper.dart (right):

https://codereview.chromium.org/1308143002/diff/40001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:3918:
completer.future.whenComplete(() => leaveJsAsync());
On 2015/08/25 00:43:27, Siggi Cherem (dart-lang) wrote:
> I guess we can also remove the explicit closure :)
> 
>   completer.future.whenComplete(leaveJsAsync);

Done.

https://codereview.chromium.org/1308143002/diff/40001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:3954: return completer.future;
On 2015/08/25 00:43:27, Siggi Cherem (dart-lang) wrote:
> Question: I see this is also how it was done in _loadHunk. I was wondering
> whether we need to chain things to avoid "forking" the future. In other words,
> if you have:
> 
>   f = readHttp(...);
>   f.then(doSomething);
> 
> do we care if doSomething executes before leaveJsAsync?
> 
> From a quick look at leaveJsAsync, it seems that we should be OK leaving it as
> you have it now, but I just wanted to check

That's a good question. I think we're OK since all leaveJsAsync does is
decrement a counter, it should be OK if another async operation kicks off before
we decrement the counter.

https://codereview.chromium.org/1308143002/diff/40001/tests/html/resource_htt...
File tests/html/resource_http_test.dart (right):

https://codereview.chromium.org/1308143002/diff/40001/tests/html/resource_htt...
tests/html/resource_http_test.dart:15: var url =
'/root_dart/tests/html/xhr_cross_origin_data.txt?'
On 2015/08/25 00:43:27, Siggi Cherem (dart-lang) wrote:
> let's use a new data file specific for this test, just to make it easier to
> discover/maintain.

Done.

https://codereview.chromium.org/1308143002/diff/40001/tests/html/resource_htt...
tests/html/resource_http_test.dart:19: var data = JSON.decode(d);
On 2015/08/25 00:43:27, Siggi Cherem (dart-lang) wrote:
> then this could be a simple string equality check

Done.

https://codereview.chromium.org/1308143002/diff/40001/tests/html/resource_htt...
tests/html/resource_http_test.dart:28: r.readAsString().then(expectAsync((s) {
On 2015/08/25 00:43:27, Siggi Cherem (dart-lang) wrote:
> either use await or return the future to ensure we pause the test before
moving
> to the next one

Done.

https://codereview.chromium.org/1308143002/diff/40001/tests/html/resource_htt...
tests/html/resource_http_test.dart:34: r.readAsBytes().then(expectAsync((b) {
On 2015/08/25 00:43:27, Siggi Cherem (dart-lang) wrote:
> ditto

Done.

[Harry Terkelsen, 2015-08-25 17:46:31]

Committed patchset #5 (id:80001) manually as
bc0deb66d786e73b8ccdc01e15bdf3045c33533b (presubmit successful).