Add Resource class.

Add Resource class. Currently unimplemented.

R=herhut@google.com, iposva@google.com

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

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

[Lasse Reichstein Nielsen, 2015-06-11 12:44:20]

lrn@google.com changed reviewers:
+ herhut@google.com, iposva@google.com, sethladd@google.com

[Lasse Reichstein Nielsen, 2015-06-11 12:44:20]

Adding just the resource class first, no implementation.
I believe the prior proof-of-concept implementation needed to be rewritten
entirely anyway.

[sethladd, 2015-06-11 12:50:13]

https://codereview.chromium.org/1181663002/diff/1/sdk/lib/core/resource.dart
File sdk/lib/core/resource.dart (right):

https://codereview.chromium.org/1181663002/diff/1/sdk/lib/core/resource.dart#...
sdk/lib/core/resource.dart:10: * A resource is data that can be located using a
URI and read into
Might want to specify that implementations much also support package: URIs.

[Lasse Reichstein Nielsen, 2015-06-11 13:53:55]

https://codereview.chromium.org/1181663002/diff/1/sdk/lib/core/resource.dart
File sdk/lib/core/resource.dart (right):

https://codereview.chromium.org/1181663002/diff/1/sdk/lib/core/resource.dart#...
sdk/lib/core/resource.dart:10: * A resource is data that can be located using a
URI and read into
I moved that to the constructor, but it might be worth saying it twice.
Actually, package URIs are the *only* URIs that is guaranteed to be supported.

[sethladd, 2015-06-12 07:25:44]

Yes, please state this in the class docs. Thanks!

[Ivan Posva, 2015-06-12 10:51:03]

LGTM once the unneeded code is removed.

Thanks,
-Ivan

https://codereview.chromium.org/1181663002/diff/1/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1181663002/diff/1/runtime/lib/core_patch.dart...
runtime/lib/core_patch.dart:7: import "dart:convert" show UTF8;
Not needed. Please remove.

https://codereview.chromium.org/1181663002/diff/1/runtime/lib/internal_patch....
File runtime/lib/internal_patch.dart (right):

https://codereview.chromium.org/1181663002/diff/1/runtime/lib/internal_patch....
runtime/lib/internal_patch.dart:16: // Example: "dart:core
Resource._readAsStream"
Not needed in this CL.

https://codereview.chromium.org/1181663002/diff/1/runtime/lib/internal_patch....
runtime/lib/internal_patch.dart:30: bool _inquireIs64Bit() native
"Internal_inquireIs64Bit";
I assume this adds a missing new line?

[Lasse Reichstein Nielsen, 2015-06-16 12:05:01]

https://codereview.chromium.org/1181663002/diff/1/runtime/lib/internal_patch....
File runtime/lib/internal_patch.dart (right):

https://codereview.chromium.org/1181663002/diff/1/runtime/lib/internal_patch....
runtime/lib/internal_patch.dart:16: // Example: "dart:core
Resource._readAsStream"
On 2015/06/12 10:51:03, Ivan Posva wrote:
> Not needed in this CL.

Done.

https://codereview.chromium.org/1181663002/diff/1/runtime/lib/internal_patch....
runtime/lib/internal_patch.dart:30: bool _inquireIs64Bit() native
"Internal_inquireIs64Bit";
Yes, seems so.

https://codereview.chromium.org/1181663002/diff/1/sdk/lib/core/resource.dart
File sdk/lib/core/resource.dart (right):

https://codereview.chromium.org/1181663002/diff/1/sdk/lib/core/resource.dart#...
sdk/lib/core/resource.dart:10: * A resource is data that can be located using a
URI and read into
On 2015/06/11 13:53:54, Lasse Reichstein Nielsen wrote:
> I moved that to the constructor, but it might be worth saying it twice.
> Actually, package URIs are the *only* URIs that is guaranteed to be supported.

Done.

[Lasse Reichstein Nielsen, 2015-06-16 12:06:53]

lrn@google.com changed reviewers:
+ floitsch@google.com, sra@google.com

[Lasse Reichstein Nielsen, 2015-06-16 12:06:54]

The current location of the Resource class, in dart:core, means that dart:core
must import dart:async because the Resource class is declared as returning
Future/Stream values.

Is that a problem? If so, where should we put the Resource class?

[herhut, 2015-06-16 12:17:04]

As it is not used right now and has no implementation behind it, I expect no
impact on size. It is, however, a very elaborated way of saying that it is not
implemented. Probably will all get inlined away if ever called though.

lgtm

[floitsch, 2015-06-16 12:27:49]

On 2015/06/16 12:17:04, herhut wrote:
> As it is not used right now and has no implementation behind it, I expect no
> impact on size. It is, however, a very elaborated way of saying that it is not
> implemented. Probably will all get inlined away if ever called though.
> 
> lgtm

I'm not that sure. I could be that 'import dart:async' triggers an 'import
dart:isolate' which increases the size of the output (and generally makes it
less efficient).

[sethladd, 2015-06-16 15:14:27]

I wonder if any dart:html app doesn't use async in some way. Perhaps most if not
all apps already pay for dart:async ?

[floitsch, 2015-06-16 15:17:02]

On 2015/06/16 15:14:27, sethladd wrote:
> I wonder if any dart:html app doesn't use async in some way. Perhaps most if
not
> all apps already pay for dart:async ?

HTML apps already pay for it, but the "hello world" application will probably
get bigger.

[Ivan Posva, 2015-06-16 15:27:17]

On 2015/06/16 15:17:02, floitsch wrote:
> On 2015/06/16 15:14:27, sethladd wrote:
> > I wonder if any dart:html app doesn't use async in some way. Perhaps most if
> not
> > all apps already pay for dart:async ?
> 
> HTML apps already pay for it, but the "hello world" application will probably
> get bigger.

Florian,

Designing something to minimize the size of the standalone dart2js "hello world"
application is a generally broken approach. As noted, all of the customer
targets of dart2js's output will include dart:async in some form.

-Ivan

[sethladd, 2015-06-20 08:59:45]

Are we good to commit this? We understand this may increase a "hello world" app
for dart2js, but that's OK for now, because "hello world" is not the goal, real
apps that use a framework or dart:html are.

[floitsch, 2015-06-22 08:47:26]

On 2015/06/20 08:59:45, sethladd wrote:
> Are we good to commit this? We understand this may increase a "hello world"
app
> for dart2js, but that's OK for now, because "hello world" is not the goal,
real
> apps that use a framework or dart:html are.

I don't (and didn't have) any objections to committing.

[floitsch, 2015-06-22 08:47:28]

On 2015/06/20 08:59:45, sethladd wrote:
> Are we good to commit this? We understand this may increase a "hello world"
app
> for dart2js, but that's OK for now, because "hello world" is not the goal,
real
> apps that use a framework or dart:html are.

I don't (and didn't have) any objections to committing.

[Lasse Reichstein Nielsen, 2015-06-22 13:05:20]

Committed patchset #3 (id:40001) manually as
2890a7a2a94ff54c7c0050e69c40ca6a60a28474 (presubmit successful).

[Lasse Reichstein Nielsen, 2015-06-22 14:47:06]

Adding a Resource class to dart:core does cause a number of warnings in the
analyzer tests.
When another library declares a Resource class, it overrides the one in
dart:core, but not without a warning. There are Resource classes declared in a
number of libraries.

[Lasse Reichstein Nielsen, 2015-06-26 09:41:54]

I've added suppression for the warnings caused by this. It all goes back to the
analyzer file_system.dart file containing a Resource class.

[Lasse Reichstein Nielsen, 2015-06-26 09:42:07]

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

[kevmoo, 2015-06-30 21:29:32]

On 2015/06/26 09:42:07, Lasse Reichstein Nielsen wrote:
> Committed patchset #5 (id:80001) manually as
> f8ce36df557af9028ef72612a43e9942d6f16585 (presubmit successful).

Please update CHANGELOG.md for API changes – even if it's just to say this is a
place-holder.

It'll keep us honest as we get closer to release 1.12 – make sure it's fully
implemented, removed, etc.