[modules] Do path resolution relative to each module file in d8

[modules] Do path resolution relative to each module file in d8

BUG=v8:1569
Committed: https://crrev.com/3bbd11c23390243a237158af4a01358497958b5b
Cr-Commit-Position: refs/heads/master@{#39646}

[adamk, 2016-09-21 20:41:43]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 20:41:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adamk, 2016-09-21 20:44:36]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 20:44:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 21:00:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 21:00:09]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/14755)

[adamk, 2016-09-21 21:03:27]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 21:03:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adamk, 2016-09-21 21:13:52]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 21:13:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adamk, 2016-09-21 22:50:34]

Okay, this is ready for review

[adamk, 2016-09-21 22:50:44]

adamk@chromium.org changed reviewers:
+ neis@chromium.org

[adamk, 2016-09-21 22:50:45]

+neis

[adamk, 2016-09-21 22:50:52]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 22:51:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 23:15:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 23:15:20]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[adamk, 2016-09-21 23:31:05]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 23:31:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adamk, 2016-09-21 23:42:49]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 23:42:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adamk, 2016-09-22 00:05:50]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 00:06:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 00:37:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 00:37:50]

Dry run: This issue passed the CQ dry run.

[neis, 2016-09-22 01:07:39]

Thanks!  Some comments below.  Will have another look tomorrow.

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc#newcode524
src/d8.cc:524: #else
I don't understand why you make it so complicated if it's clearly not correct in
general.  For now, can't you just test for a colon and that's it?

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc#newcode566
src/d8.cc:566: std::map<std::string, Global<Module>>* module_map) {
Add a DCHECK(IsAbsolute) here?

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc#newcode588
src/d8.cc:588: std::string dir_name = file_name.substr(0, last_slash + 1);
Please move this into a separate function.

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc#newcode596
src/d8.cc:596: CHECK(!IsAbsolutePath(*utf8_value));
Can you add a comment about this restriction?

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc#newcode622
src/d8.cc:622: }
Can you move this stuff up a little?

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc#newcode1955
src/d8.cc:1955: #undef StrDup
??

[adamk, 2016-09-22 19:06:51]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[adamk, 2016-09-22 19:06:57]

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc#newcode524
src/d8.cc:524: #else
On 2016/09/22 01:07:39, neis wrote:
> I don't understand why you make it so complicated if it's clearly not correct
in
> general.  For now, can't you just test for a colon and that's it?

Just looking for a colon now.

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc#newcode566
src/d8.cc:566: std::map<std::string, Global<Module>>* module_map) {
On 2016/09/22 01:07:39, neis wrote:
> Add a DCHECK(IsAbsolute) here?

Done.

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc#newcode588
src/d8.cc:588: std::string dir_name = file_name.substr(0, last_slash + 1);
On 2016/09/22 01:07:39, neis wrote:
> Please move this into a separate function.

Done.

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc#newcode596
src/d8.cc:596: CHECK(!IsAbsolutePath(*utf8_value));
On 2016/09/22 01:07:39, neis wrote:
> Can you add a comment about this restriction?

Removed the restriction instead.

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc#newcode622
src/d8.cc:622: }
On 2016/09/22 01:07:39, neis wrote:
> Can you move this stuff up a little?

Done.

https://codereview.chromium.org/2361593002/diff/280001/src/d8.cc#newcode1955
src/d8.cc:1955: #undef StrDup
On 2016/09/22 01:07:39, neis wrote:
> ??

This was from a previous attempt to call the Win32 PathIsRelative API. Removed.

[commit-bot: I haz the power, 2016-09-22 19:06:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 19:41:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 19:41:46]

Dry run: This issue passed the CQ dry run.

[neis, 2016-09-22 21:00:30]

lgtm

[adamk, 2016-09-22 21:01:00]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2016-09-22 21:01:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 21:03:26]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-09-22 21:03:52]

Description was changed from

==========
[modules] Do path resolution relative to each module file in d8

BUG=v8:1569
==========

to

==========
[modules] Do path resolution relative to each module file in d8

BUG=v8:1569

Committed: https://crrev.com/3bbd11c23390243a237158af4a01358497958b5b
Cr-Commit-Position: refs/heads/master@{#39646}
==========

[commit-bot: I haz the power, 2016-09-22 21:03:53]

Patchset 16 (id:??) landed as
https://crrev.com/3bbd11c23390243a237158af4a01358497958b5b
Cr-Commit-Position: refs/heads/master@{#39646}