Replace compileAndRunInternalScript with not internal in DocumentWriteEvaluator

Replace compileAndRunInternalScript with not internal in DocumentWriteEvaluator

compileAndRunInternalScript passed isInternalScript flag with script and won't run microtasks after execution.
Flag is used in DevTools, e.g. to filter out this scripts from search over all loaded scripts.

BUG=none
R=csharrison@chromium.org,haraken@chromium.org

[kozy, 2016-07-23 02:28:42]

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

[commit-bot: I haz the power, 2016-07-23 02:29:03]

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

[kozy, 2016-07-23 02:29:43]

Description was changed from

==========
Replace compileAndRunInternalScript with not internal in DocumentWriteEvaluator

compileAndRunInternalScript passed isInternalScript flag with script and won't
run microtasks after execution.
Flag is used in DevTools, e.g. to filter out this scripts from search over all
scripts content.

BUG=none
R=csharrison@chromium.org,haraken@chromium.org
==========

to

==========
Replace compileAndRunInternalScript with not internal in DocumentWriteEvaluator

compileAndRunInternalScript passed isInternalScript flag with script and won't
run microtasks after execution.
Flag is used in DevTools, e.g. to filter out this scripts from search over all
loaded scripts.

BUG=none
R=csharrison@chromium.org,haraken@chromium.org
==========

[commit-bot: I haz the power, 2016-07-23 03:54:52]

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

[commit-bot: I haz the power, 2016-07-23 03:54:54]

Dry run: This issue passed the CQ dry run.

[kozy, 2016-07-23 19:41:10]

On 2016/07/23 03:54:54, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Please take a look!
I'm not sure about this change. Should we run microtasks after script that was
runned by Document Evaluator, e.g. if script content is
Promise.resolve().then(foo), should function foo be called after script
evaluation? And should we be able to pause in this script with DevTools? If
answers are yes, then we need to replace compile and run internal with regular
one.

[haraken, 2016-07-25 07:40:43]

On 2016/07/23 19:41:10, kozyatinskiy wrote:
> On 2016/07/23 03:54:54, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> Please take a look!
> I'm not sure about this change. Should we run microtasks after script that was
> runned by Document Evaluator, e.g. if script content is
> Promise.resolve().then(foo), should function foo be called after script
> evaluation? And should we be able to pause in this script with DevTools? If
> answers are yes, then we need to replace compile and run internal with regular
> one.

I think the answer would be yes. It looks strange that we switch how to run the
script depending on whether |executionContext| is null or not.

[kozy, 2016-07-25 07:51:19]

On 2016/07/25 07:40:43, haraken wrote:
> On 2016/07/23 19:41:10, kozyatinskiy wrote:
> > On 2016/07/23 03:54:54, commit-bot: I haz the power wrote:
> > > Dry run: This issue passed the CQ dry run.
> > 
> > Please take a look!
> > I'm not sure about this change. Should we run microtasks after script that
was
> > runned by Document Evaluator, e.g. if script content is
> > Promise.resolve().then(foo), should function foo be called after script
> > evaluation? And should we be able to pause in this script with DevTools? If
> > answers are yes, then we need to replace compile and run internal with
regular
> > one.
> 
> I think the answer would be yes. It looks strange that we switch how to run
the
> script depending on whether |executionContext| is null or not.

I added a way to run scripts without execution context for tests. I can
introduce separate method for tests or try to call it with fake context in test.

[haraken, 2016-07-25 07:59:19]

On 2016/07/25 07:51:19, kozyatinskiy wrote:
> On 2016/07/25 07:40:43, haraken wrote:
> > On 2016/07/23 19:41:10, kozyatinskiy wrote:
> > > On 2016/07/23 03:54:54, commit-bot: I haz the power wrote:
> > > > Dry run: This issue passed the CQ dry run.
> > > 
> > > Please take a look!
> > > I'm not sure about this change. Should we run microtasks after script that
> was
> > > runned by Document Evaluator, e.g. if script content is
> > > Promise.resolve().then(foo), should function foo be called after script
> > > evaluation? And should we be able to pause in this script with DevTools?
If
> > > answers are yes, then we need to replace compile and run internal with
> regular
> > > one.
> > 
> > I think the answer would be yes. It looks strange that we switch how to run
> the
> > script depending on whether |executionContext| is null or not.
> 
> I added a way to run scripts without execution context for tests. I can
> introduce separate method for tests or try to call it with fake context in
test.

Can you use DummyPageHolder for testing? It will give you a valid
ExecutionContext.

[Charlie Harrison, 2016-07-25 13:07:28]

Honestly I'm not sure microtask execution is needed for the
DocumentWriteEvaluator. It is doubtful that a callback will end up using
document.write to write an inline script, and even if it does the code is not
currently set up to handle that case.

I'm happy to defer to haraken@ here though because this is not really my area of
expertise.

[haraken, 2016-07-25 13:28:00]

On 2016/07/25 13:07:28, csharrison wrote:
> Honestly I'm not sure microtask execution is needed for the
> DocumentWriteEvaluator. It is doubtful that a callback will end up using
> document.write to write an inline script, and even if it does the code is not
> currently set up to handle that case.
> 
> I'm happy to defer to haraken@ here though because this is not really my area
of
> expertise.

I looked at the code again.

I think that microtasks should be *unconditionally* disabled for all scripts
executed by DocumentWriteEvaluator because the script execution is totally
internal a thing. Whether we should run the internal thing or not should not
affect user-facing behavior.

[Charlie Harrison, 2016-07-25 14:01:34]

On 2016/07/25 13:28:00, haraken wrote:
> On 2016/07/25 13:07:28, csharrison wrote:
> > Honestly I'm not sure microtask execution is needed for the
> > DocumentWriteEvaluator. It is doubtful that a callback will end up using
> > document.write to write an inline script, and even if it does the code is
not
> > currently set up to handle that case.
> > 
> > I'm happy to defer to haraken@ here though because this is not really my
area
> of
> > expertise.
> 
> I looked at the code again.
> 
> I think that microtasks should be *unconditionally* disabled for all scripts
> executed by DocumentWriteEvaluator because the script execution is totally
> internal a thing. Whether we should run the internal thing or not should not
> affect user-facing behavior.

Agreed, this makes sense.

[kozy, 2016-07-25 20:19:14]

On 2016/07/25 14:01:34, csharrison wrote:
> Agreed, this makes sense.

Thanks! I think that we can just remove isInternalFlag from V8ScriptRunner and
leave DocumentWriteEvaluator implementation as is. Microtasks will be still
disabled, DevTools could be paused in this scripts if it contains debugger
statement.
I prepared another CL: https://codereview.chromium.org/2179943002/ .