Use serialized scope info to test incremental compiler.

Use serialized scope info to test incremental compiler.

R=johnniwinther@google.com

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

[ahe, 2014-11-20 15:25:07]

Patchset #1 (id:1) has been deleted

[ahe, 2014-11-20 15:27:19]

Patchset #1 (id:20001) has been deleted

[ahe, 2014-11-20 15:28:37]

ahe@google.com changed reviewers:
+ johnniwinther@google.com

[ahe, 2014-11-21 18:37:47]

I've uploaded a new patch set.

[Johnni Winther, 2014-11-24 10:30:46]

lgtm

https://codereview.chromium.org/738423002/diff/80001/dart/tests/try/web/incre...
File dart/tests/try/web/incremental_compilation_update_test.dart (right):

https://codereview.chromium.org/738423002/diff/80001/dart/tests/try/web/incre...
dart/tests/try/web/incremental_compilation_update_test.dart:1158: Future run()
=> mainApp.then(checkScopes);
Maybe `mainApp` should be a function `compileMainApp()` so that this would read
`compileMainApp().then(checkScopes)`.

[ahe, 2014-11-25 14:19:23]

Thank you, Johnni!

https://codereview.chromium.org/738423002/diff/80001/dart/tests/try/web/incre...
File dart/tests/try/web/incremental_compilation_update_test.dart (right):

https://codereview.chromium.org/738423002/diff/80001/dart/tests/try/web/incre...
dart/tests/try/web/incremental_compilation_update_test.dart:1158: Future run()
=> mainApp.then(checkScopes);
On 2014/11/24 10:30:46, Johnni Winther wrote:
> Maybe `mainApp` should be a function `compileMainApp()` so that this would
read
> `compileMainApp().then(checkScopes)`.

Then it should be something like "loadMainApp", as it doesn't actually compile
the code. I don't see this as a major improvement, and since it would mean
changing existing tests I hesitate to make such a change, unless you feel it
would be a great improvement.

[Johnni Winther, 2014-11-25 14:22:21]

https://codereview.chromium.org/738423002/diff/80001/dart/tests/try/web/incre...
File dart/tests/try/web/incremental_compilation_update_test.dart (right):

https://codereview.chromium.org/738423002/diff/80001/dart/tests/try/web/incre...
dart/tests/try/web/incremental_compilation_update_test.dart:1158: Future run()
=> mainApp.then(checkScopes);
On 2014/11/25 14:19:22, ahe wrote:
> On 2014/11/24 10:30:46, Johnni Winther wrote:
> > Maybe `mainApp` should be a function `compileMainApp()` so that this would
> read
> > `compileMainApp().then(checkScopes)`.
> 
> Then it should be something like "loadMainApp", as it doesn't actually compile
> the code. I don't see this as a major improvement, and since it would mean
> changing existing tests I hesitate to make such a change, unless you feel it
> would be a great improvement.

I found it difficult to reason about the control flow since `mainApp` does not
indicate that its an invocation that has side-effects. The renaming can be done
in a separate CL.

[ahe, 2014-11-25 14:30:55]

https://codereview.chromium.org/738423002/diff/80001/dart/tests/try/web/incre...
File dart/tests/try/web/incremental_compilation_update_test.dart (right):

https://codereview.chromium.org/738423002/diff/80001/dart/tests/try/web/incre...
dart/tests/try/web/incremental_compilation_update_test.dart:1158: Future run()
=> mainApp.then(checkScopes);
On 2014/11/25 14:22:21, Johnni Winther wrote:
> I found it difficult to reason about the control flow since `mainApp` does not
> indicate that its an invocation that has side-effects. The renaming can be
done
> in a separate CL.

Since you found it difficult to reason about the code, I'll follow up with a CL
where I rename it.

[ahe, 2014-11-25 14:44:15]

Submitting, but PTAL at patch set 4.

[ahe, 2014-11-25 14:48:39]

Committed patchset #6 (id:140001) manually as 41955 (presubmit successful).

[ahe, 2014-11-25 15:15:36]

https://codereview.chromium.org/738423002/diff/80001/dart/tests/try/web/incre...
File dart/tests/try/web/incremental_compilation_update_test.dart (right):

https://codereview.chromium.org/738423002/diff/80001/dart/tests/try/web/incre...
dart/tests/try/web/incremental_compilation_update_test.dart:1158: Future run()
=> mainApp.then(checkScopes);
On 2014/11/25 14:30:55, ahe wrote:
> On 2014/11/25 14:22:21, Johnni Winther wrote:
> > I found it difficult to reason about the control flow since `mainApp` does
not
> > indicate that its an invocation that has side-effects. The renaming can be
> done
> > in a separate CL.
> 
> Since you found it difficult to reason about the code, I'll follow up with a
CL
> where I rename it.

Follow up in https://codereview.chromium.org/757043002.

[Johnni Winther, 2014-11-26 11:59:42]

Still lgtm