Load patches via callback.

Load patches via callback.

This CL is a prerequisite for moving patching to the JavaScript backend.

BUG=
R=ahe@google.com, floitsch@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37651

[floitsch, 2014-06-23 19:12:34]

LGTM but I don't like the names.

onX should be a function that takes a handler/callback or returns a
future/stream, which is triggered when X happens.
Eg: element.onClick().then(...)

Here onX is used differently: when "X" happens invoke "onX". I prefer
"register", "notify", ... instead of "on" in this context.

https://codereview.chromium.org/350693007/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/350693007/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:141: abstract
class LibraryLoaderCallback {
Don't call this "callback". It took me some time to realize that the
LibraryLoaderCallback is not just a function typedef.

https://codereview.chromium.org/350693007/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:142: /// Call this
when a new synthesized/patch library has been created.
This is ambiguous.
Who needs to call this method? The implementer of the interface? The user of it?
The framework (in this file)?

"Must be called when a ..."


Why not "registerLibrary" and "processTags" ?
This should make it clearer what the purpose of these methods are.

https://codereview.chromium.org/350693007/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:306: //
TODO(rnystrom): Remove .toList() here if #11523 is fixed.
11523 has been closed with 'asDesigned'.
It should be safe to remove the 'toList()', but I don't think it costs much to
keep it.

https://codereview.chromium.org/350693007/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/350693007/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:822: &&
library.entryCompilationUnit == compilationUnitElement;
indent by 4.

[Johnni Winther, 2014-06-24 08:09:33]

Updated cf. comments.

[Johnni Winther, 2014-06-24 08:11:21]

PTAL -- I renamed several classes.

https://codereview.chromium.org/350693007/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/350693007/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:141: abstract
class LibraryLoaderCallback {
On 2014/06/23 19:12:33, floitsch wrote:
> Don't call this "callback". It took me some time to realize that the
> LibraryLoaderCallback is not just a function typedef.

Done.

https://codereview.chromium.org/350693007/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:142: /// Call this
when a new synthesized/patch library has been created.
On 2014/06/23 19:12:33, floitsch wrote:
> This is ambiguous.
> Who needs to call this method? The implementer of the interface? The user of
it?
> The framework (in this file)?
> 
> "Must be called when a ..."
> 
> 
> Why not "registerLibrary" and "processTags" ?
> This should make it clearer what the purpose of these methods are.

Done.

https://codereview.chromium.org/350693007/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:306: //
TODO(rnystrom): Remove .toList() here if #11523 is fixed.
On 2014/06/23 19:12:33, floitsch wrote:
> 11523 has been closed with 'asDesigned'.
> It should be safe to remove the 'toList()', but I don't think it costs much to
> keep it.

Removed the comment. Still need the `toList()` call sine `Link` is not
`Iterable`.

https://codereview.chromium.org/350693007/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/350693007/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:822: &&
library.entryCompilationUnit == compilationUnitElement;
On 2014/06/23 19:12:33, floitsch wrote:
> indent by 4.

Done.

[floitsch, 2014-06-24 09:01:54]

LGTM.
much better, thanks.

https://codereview.chromium.org/350693007/diff/30001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/350693007/diff/30001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:1066: /// Use [callback]
to register the creation and scanning of a patch library
s/callback/loader/

https://codereview.chromium.org/350693007/diff/30001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:1069: LibraryLoader
loader) {
might fit on one line now?

https://codereview.chromium.org/350693007/diff/30001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/350693007/diff/30001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1605:
LibraryLoader loader) {
might fit on one line?

https://codereview.chromium.org/350693007/diff/30001/tests/compiler/dart2js/e...
File tests/compiler/dart2js/exit_code_test.dart (right):

https://codereview.chromium.org/350693007/diff/30001/tests/compiler/dart2js/e...
tests/compiler/dart2js/exit_code_test.dart:53: LibraryLoader loader) {
one line?

[Johnni Winther, 2014-06-24 09:12:59]

https://codereview.chromium.org/350693007/diff/30001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/350693007/diff/30001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:1066: /// Use [callback]
to register the creation and scanning of a patch library
On 2014/06/24 09:01:54, floitsch wrote:
> s/callback/loader/

Done.

https://codereview.chromium.org/350693007/diff/30001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:1069: LibraryLoader
loader) {
On 2014/06/24 09:01:54, floitsch wrote:
> might fit on one line now?

Done.

https://codereview.chromium.org/350693007/diff/30001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/350693007/diff/30001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1605:
LibraryLoader loader) {
On 2014/06/24 09:01:54, floitsch wrote:
> might fit on one line?

Done.

https://codereview.chromium.org/350693007/diff/30001/tests/compiler/dart2js/e...
File tests/compiler/dart2js/exit_code_test.dart (right):

https://codereview.chromium.org/350693007/diff/30001/tests/compiler/dart2js/e...
tests/compiler/dart2js/exit_code_test.dart:53: LibraryLoader loader) {
On 2014/06/24 09:01:54, floitsch wrote:
> one line?

Done.

[Johnni Winther, 2014-06-24 09:45:15]

@ahe: PTAL at the last patch.

[ahe, 2014-06-24 09:49:21]

https://codereview.chromium.org/350693007/diff/90001/site/try/src/caching_com...
File site/try/src/caching_compiler.dart (left):

https://codereview.chromium.org/350693007/diff/90001/site/try/src/caching_com...
site/try/src/caching_compiler.dart:20: LibraryLoaderTask, // TODO(ahe): Remove
this import.
Changes in this file, LGTM!

[Johnni Winther, 2014-06-24 10:41:37]

Committed patchset #6 manually as r37651 (presubmit successful).