Per isolate package root.

Per isolate package root.

BUG=
R=iposva@google.com, ricow@google.com, sgjesse@google.com, turnidge@google.com

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

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

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

[Anders Johnsen, 2014-09-05 09:20:44]

ajohnsen@google.com changed reviewers:
+ iposva@google.com

[Anders Johnsen, 2014-09-05 09:20:45]

The Isolate.spawnUri packageRoot flag implementation, as discussed.

You should be able to apply this directly on bleeding edge.

[Ivan Posva, 2014-09-08 05:46:34]

iposva@google.com changed reviewers:
+ turnidge@google.com

[Ivan Posva, 2014-09-08 05:46:35]

First set of comments. Adding Todd to make sure the Dart C API changes fit with
his plans.

-Ivan

https://codereview.chromium.org/545483002/diff/80001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/545483002/diff/80001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:557: void* data,
The data parameter should be typed as IsolateData*.

https://codereview.chromium.org/545483002/diff/80001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:590: IsolateData* isolate_data =
reinterpret_cast<IsolateData*>(data);
Unneeded.

https://codereview.chromium.org/545483002/diff/80001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:703: builtin_lib);
I would expect the service isolate to not expect a valid package root. The
command line package root is certainly not the right one to use.

https://codereview.chromium.org/545483002/diff/80001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/545483002/diff/80001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:285: if (packageRoot != null) packageRootString =
packageRoot.toString();
{}

https://codereview.chromium.org/545483002/diff/80001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:286: _spawnUri(readyPort.sendPort,
uri.toString(), args, message,
Awkward wrap around.

[turnidge, 2014-09-08 16:40:48]

https://codereview.chromium.org/545483002/diff/80001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/545483002/diff/80001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:215: NULL);
Please add the package_root to IsolateSpawnState as a constructor argument
instead.  I have been moving all of the Spawn params inside this object in
preparation for initializing isolates in a separate thread.

https://codereview.chromium.org/545483002/diff/80001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:254: utf8_package_root);
Ditto above, please pass the package root via IsolateSpawnState.

[turnidge, 2014-09-08 19:47:01]

https://codereview.chromium.org/545483002/diff/80001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/545483002/diff/80001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:662: isolate_data,
You are treating script_uri and package_root assymetrically here.

I see that you are tucking away script_uri and package_root in the data for
later.  But you then explicilty pass "script_uri" to CreateIsolateAndSetupHelper
but rely on isolate_data to pass in "package_root".  Instead how about you pass
in package_root as an explicit parameter?

https://codereview.chromium.org/545483002/diff/80001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/545483002/diff/80001/runtime/include/dart_api...
runtime/include/dart_api.h:726: *   C string containing an error message in the
case of failures.
Update comment.

[Anders Johnsen, 2014-09-09 06:45:44]

Good feedback, PTAL!

I agree that the CreateIsolateAndSetupHelper is not very well organized at this
point, might be worth to look into.

https://codereview.chromium.org/545483002/diff/80001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/545483002/diff/80001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:557: void* data,
On 2014/09/08 05:46:35, Ivan Posva wrote:
> The data parameter should be typed as IsolateData*.

Done.

https://codereview.chromium.org/545483002/diff/80001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:590: IsolateData* isolate_data =
reinterpret_cast<IsolateData*>(data);
On 2014/09/08 05:46:35, Ivan Posva wrote:
> Unneeded.

Done.

https://codereview.chromium.org/545483002/diff/80001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:662: isolate_data,
On 2014/09/08 19:47:00, turnidge wrote:
> You are treating script_uri and package_root assymetrically here.
> 
> I see that you are tucking away script_uri and package_root in the data for
> later.  But you then explicilty pass "script_uri" to
CreateIsolateAndSetupHelper
> but rely on isolate_data to pass in "package_root".  Instead how about you
pass
> in package_root as an explicit parameter? 

It would needed to be passed both places (like script_uri). Changing, but let me
know if this is not what you meant.

https://codereview.chromium.org/545483002/diff/80001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:703: builtin_lib);
On 2014/09/08 05:46:35, Ivan Posva wrote:
> I would expect the service isolate to not expect a valid package root. The
> command line package root is certainly not the right one to use.

Will pass 'NULL' then.

https://codereview.chromium.org/545483002/diff/80001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/545483002/diff/80001/runtime/include/dart_api...
runtime/include/dart_api.h:726: *   C string containing an error message in the
case of failures.
On 2014/09/08 19:47:00, turnidge wrote:
> Update comment.

Done.

https://codereview.chromium.org/545483002/diff/80001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/545483002/diff/80001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:215: NULL);
On 2014/09/08 16:40:48, turnidge wrote:
> Please add the package_root to IsolateSpawnState as a constructor argument
> instead.  I have been moving all of the Spawn params inside this object in
> preparation for initializing isolates in a separate thread.

Done.

https://codereview.chromium.org/545483002/diff/80001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:254: utf8_package_root);
On 2014/09/08 16:40:48, turnidge wrote:
> Ditto above, please pass the package root via IsolateSpawnState.

Done.

https://codereview.chromium.org/545483002/diff/80001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/545483002/diff/80001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:285: if (packageRoot != null) packageRootString =
packageRoot.toString();
On 2014/09/08 05:46:35, Ivan Posva wrote:
> {}

Done.

https://codereview.chromium.org/545483002/diff/80001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:286: _spawnUri(readyPort.sendPort,
uri.toString(), args, message,
On 2014/09/08 05:46:35, Ivan Posva wrote:
> Awkward wrap around.

Done.

[turnidge, 2014-09-09 15:46:42]

LGTM w/ 1 moderately-sized comment below.  Also please wait on Ivan's LGTM.

https://codereview.chromium.org/545483002/diff/100001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/545483002/diff/100001/runtime/bin/main.cc#new...
runtime/bin/main.cc:555: bool* is_compile_error) {
Thanks Anders.

I think I want to change my earlier recommendation after reading the code a bit
more.  Since script_uri is always equal to isolate_data->script_url and the same
with package_root, I think we should do the following:

- Remove the isolate_data parameter from this function signature.

- Move the creation of IsolateData inside of this function:

  ASSERT(script_uri != NULL);
  ASSERT(package_root != NULL);
  IsolateData* isolate_data = new IsolateData(script_uri, package_root);

...or even...

   Dart_Isolate isolate = Dart_CreateIsolate(
       script_uri, main, snapshot_buffer, new IsolateData(script_uri,
package_root, error);

What do you think?

https://codereview.chromium.org/545483002/diff/100001/runtime/bin/main.cc#new...
runtime/bin/main.cc:587: ASSERT(isolate_data->script_url != NULL);
Remove if you follow the above recommendation.

https://codereview.chromium.org/545483002/diff/100001/runtime/bin/main.cc#new...
runtime/bin/main.cc:594: result =
DartUtils::LoadScript(isolate_data->script_url, builtin_lib);
Use script_uri instead of isolate_data->script_url.

https://codereview.chromium.org/545483002/diff/100001/runtime/bin/main.cc#new...
runtime/bin/main.cc:653: IsolateData* isolate_data = new IsolateData(script_uri,
package_root);
Creation moves into helper function.

https://codereview.chromium.org/545483002/diff/100001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1049: commandline_package_root);
Creation moves into helper function.

[Anders Johnsen, 2014-09-10 05:42:16]

ajohnsen@google.com changed reviewers:
+ asiva@google.com

[Anders Johnsen, 2014-09-10 05:42:17]

That was more or less the cleanup I had in mind - done. :)

Siva, can you help me prepare Dartium for this change? I suggest we start out by
just ignoring the package-root so this can land, and then work on actually
honoring the flag. Thanks!

https://codereview.chromium.org/545483002/diff/100001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/545483002/diff/100001/runtime/bin/main.cc#new...
runtime/bin/main.cc:555: bool* is_compile_error) {
On 2014/09/09 15:46:41, turnidge wrote:
> Thanks Anders.
> 
> I think I want to change my earlier recommendation after reading the code a
bit
> more.  Since script_uri is always equal to isolate_data->script_url and the
same
> with package_root, I think we should do the following:
> 
> - Remove the isolate_data parameter from this function signature.
> 
> - Move the creation of IsolateData inside of this function:
> 
>   ASSERT(script_uri != NULL);
>   ASSERT(package_root != NULL);
>   IsolateData* isolate_data = new IsolateData(script_uri, package_root);
> 
> ...or even...
> 
>    Dart_Isolate isolate = Dart_CreateIsolate(
>        script_uri, main, snapshot_buffer, new IsolateData(script_uri,
> package_root, error);
> 
> What do you think?
> 

Done.

https://codereview.chromium.org/545483002/diff/100001/runtime/bin/main.cc#new...
runtime/bin/main.cc:587: ASSERT(isolate_data->script_url != NULL);
On 2014/09/09 15:46:41, turnidge wrote:
> Remove if you follow the above recommendation.

Done.

https://codereview.chromium.org/545483002/diff/100001/runtime/bin/main.cc#new...
runtime/bin/main.cc:594: result =
DartUtils::LoadScript(isolate_data->script_url, builtin_lib);
On 2014/09/09 15:46:41, turnidge wrote:
> Use script_uri instead of isolate_data->script_url.

Done.

https://codereview.chromium.org/545483002/diff/100001/runtime/bin/main.cc#new...
runtime/bin/main.cc:653: IsolateData* isolate_data = new IsolateData(script_uri,
package_root);
On 2014/09/09 15:46:41, turnidge wrote:
> Creation moves into helper function.

Done.

https://codereview.chromium.org/545483002/diff/100001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1049: commandline_package_root);
On 2014/09/09 15:46:41, turnidge wrote:
> Creation moves into helper function.

Done.

[Ivan Posva, 2014-09-19 16:50:17]

LGTM

-Ivan

https://codereview.chromium.org/545483002/diff/120001/runtime/lib/isolate_pat...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/545483002/diff/120001/runtime/lib/isolate_pat...
runtime/lib/isolate_patch.dart:284: var packageRootString;
var packageRootString = (packageRoot == null) ? null : packageRoot.toString();

To make it explicit that the string is null if the root is null.

[Anders Johnsen, 2014-09-22 07:08:33]

https://codereview.chromium.org/545483002/diff/120001/runtime/lib/isolate_pat...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/545483002/diff/120001/runtime/lib/isolate_pat...
runtime/lib/isolate_patch.dart:284: var packageRootString;
On 2014/09/19 16:50:17, Ivan Posva wrote:
> var packageRootString = (packageRoot == null) ? null : packageRoot.toString();
> 
> To make it explicit that the string is null if the root is null.

Done.

[Søren Gjesse, 2014-09-22 07:19:52]

sgjesse@google.com changed reviewers:
+ sgjesse@google.com

[Søren Gjesse, 2014-09-22 07:19:53]

Test lgtm

[Anders Johnsen, 2014-09-22 08:07:20]

Committed patchset #8 (id:140001) manually as 40534 (presubmit successful).

[Anders Johnsen, 2014-09-22 11:13:57]

Reverted, as Dartium is closed atm.

[nweiz, 2014-09-22 17:49:16]

On 2014/09/22 11:13:57, Anders Johnsen wrote:
> Reverted, as Dartium is closed atm.

So close! Can you ping me when this gets un-reverted?

[Anders Johnsen, 2014-09-23 05:42:55]

ajohnsen@google.com changed reviewers:
+ ricow@google.com

[ricow1, 2014-09-23 05:45:28]

only reviewed patchset 8 changes, rest already covered, lgtm

[Anders Johnsen, 2014-09-23 05:46:30]

Committed patchset #9 (id:160001) manually as 40581 (presubmit successful).

[terry, 2014-09-23 14:32:56]

Committed patchset #9 (id:160001) manually as 40593 (presubmit successful).