Add necessary support functions so that embedders can implemented pause on start and exit

Add necessary support functions so that embedders can implemented pause on start and exit

- Add Dart_ShouldPauseOnStart, Dart_SetShouldPauseOnStart, Dart_IsPausedOnStart, Dart_SetPausedOnStart, Dart_ShouldPauseOnExit, Dart_SetShouldPauseOnExit, Dart_IsPausedOnExit, Dart_SetPausedOnExit to query and control pause on start / exit from outside of the default message handler.
- Add Dart_HandleMessages to process all regular messages from outside the default message handler.
- Allow per isolate overriding of pause on start / pause on exit flags.
- Set the resume_request bit when resuming from isolate start / exit.
- Clear the resume_request bit in the default message handler.
- Rename MessageHandler fields and accessors.
- Rename Isolate::Resume to Isolate::SetResumeRequest.

R=turnidge@google.com

Committed: https://github.com/dart-lang/sdk/commit/6f76e2af994a08b1aa08dbf737991eece01186e0

[Cutch, 2016-02-03 19:37:46]

Description was changed from

==========
Add necessary support functions so that embedders can implemented pause on start
and exit

BUG=
==========

to

==========
Add necessary support functions so that embedders can implemented pause on start
and exit

- Add Dart_ShouldPauseOnStart, Dart_SetShouldPauseOnStart, Dart_IsPausedOnStart,
Dart_SetPausedOnStart, Dart_ShouldPauseOnExit, Dart_SetShouldPauseOnExit,
Dart_IsPausedOnExit, Dart_SetPausedOnExit to query and control pause on start /
exit from outside of the default message handler.
- Add Dart_HandleMessages to process all regular messages from outside the
default message handler.
- Allow per isolate overriding of pause on start / pause on exit flags.
- Set the resume_request bit when resuming from isolate start / exit.
- Clear the resume_request bit in the default message handler.
==========

[Cutch, 2016-02-03 19:37:46]

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

[turnidge, 2016-02-03 21:33:37]

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

https://codereview.chromium.org/1665773004/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:1063: DART_EXPORT void
Dart_SetShouldPauseOnStart(bool v);
v -> value, here and below.

https://codereview.chromium.org/1665773004/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:1107: DART_EXPORT void Dart_SetPausedOnExit(bool v);
We discussed turning this into something like:

void Dart_SetPaused(kPauseExit, true)

I'm torn about whether this is a good idea or not.  I'm just putting it in the
review to document.

https://codereview.chromium.org/1665773004/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:1120: * Handles all pending message for the current
isolate.
message -> messages

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1375: DART_EXPORT void
Dart_SetShouldPauseOnStart(bool v) {
v -> value or perhaps should_pause.

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1390: DART_EXPORT void Dart_SetPausedOnStart(bool v)
{
v -> value or paused, here and below.

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1398: }
As discussed offline, consider making a single api entry point
(SetPausedOnStart) instead of the current two public ones (PausedOnStart,
NotifyPauseOnStart).

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:1076: }
As discussed offline, we may be able to remove pause_isolates_flags_overridden_.

We would move the initialization of pause_on_start/pause_on_exit to the
constructor for the message handler.  Then, inside the message handler, we could
use the runnable bit to determine whether or not we should look at the value...

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/message_handler.cc
File runtime/vm/message_handler.cc (right):

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/message_handler....
runtime/vm/message_handler.cc:332: }
I'm guessing it doesn't work to move this into SetPausedOnStart(false) (the new
combo of PausedOnStart/NotifyPauseOnStart)... but if it did, it would be nice to
get this code tucked away.

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/message_handler.h
File runtime/vm/message_handler.h (right):

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/message_handler....
runtime/vm/message_handler.h:59: // Handles all normals messages for this
message handler.  Should only
Delete "normals".

This functions actually handles all messages, so consider naming it
HandleMessages or perhaps HandleAllMessages.

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/message_handler....
runtime/vm/message_handler.h:88: bool pause_on_start() const {
Consider renaming these to match your CL:

pause_on_start() -> ShouldPauseOnStart()
set_pause_on_start() -> SetShouldPauseOnStart()
paused_on_start() -> IsPausedOnStart(),

I think your new names are more clear than my old ones.

etc.

Perhaps rename the fields as well:

pause_on_start_ -> should_pause_on_start_.

You get the drill.

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:2978: isolate->Resume();
Does this send a resume event?  Do we want to suppress it?

[turnidge, 2016-02-03 21:35:53]

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:1076: }
On 2016/02/03 21:33:36, turnidge wrote:
> As discussed offline, we may be able to remove
pause_isolates_flags_overridden_.
> 
> We would move the initialization of pause_on_start/pause_on_exit to the
> constructor for the message handler.  Then, inside the message handler, we
could
> use the runnable bit to determine whether or not we should look at the
value...

One more thing, we wouldn't want pause_on_start to ever happen for native
message handlers, so we would probably guard the whole thing in an isolate() !=
NULL...

[Cutch, 2016-02-03 22:56:56]

Description was changed from

==========
Add necessary support functions so that embedders can implemented pause on start
and exit

- Add Dart_ShouldPauseOnStart, Dart_SetShouldPauseOnStart, Dart_IsPausedOnStart,
Dart_SetPausedOnStart, Dart_ShouldPauseOnExit, Dart_SetShouldPauseOnExit,
Dart_IsPausedOnExit, Dart_SetPausedOnExit to query and control pause on start /
exit from outside of the default message handler.
- Add Dart_HandleMessages to process all regular messages from outside the
default message handler.
- Allow per isolate overriding of pause on start / pause on exit flags.
- Set the resume_request bit when resuming from isolate start / exit.
- Clear the resume_request bit in the default message handler.
==========

to

==========
Add necessary support functions so that embedders can implemented pause on start
and exit

- Add Dart_ShouldPauseOnStart, Dart_SetShouldPauseOnStart, Dart_IsPausedOnStart,
Dart_SetPausedOnStart, Dart_ShouldPauseOnExit, Dart_SetShouldPauseOnExit,
Dart_IsPausedOnExit, Dart_SetPausedOnExit to query and control pause on start /
exit from outside of the default message handler.
- Add Dart_HandleMessages to process all regular messages from outside the
default message handler.
- Allow per isolate overriding of pause on start / pause on exit flags.
- Set the resume_request bit when resuming from isolate start / exit.
- Clear the resume_request bit in the default message handler.
- Rename MessageHandler fields and accessors.
- Rename Isolate::Resume to Isolate::SetResumeRequest.
==========

[Cutch, 2016-02-03 23:04:05]

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

https://codereview.chromium.org/1665773004/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:1063: DART_EXPORT void
Dart_SetShouldPauseOnStart(bool v);
On 2016/02/03 21:33:36, turnidge wrote:
> v -> value, here and below.

Done.

https://codereview.chromium.org/1665773004/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:1107: DART_EXPORT void Dart_SetPausedOnExit(bool v);
On 2016/02/03 21:33:36, turnidge wrote:
> We discussed turning this into something like:
> 
> void Dart_SetPaused(kPauseExit, true)
> 
> I'm torn about whether this is a good idea or not.  I'm just putting it in the
> review to document.

Acknowledged.

https://codereview.chromium.org/1665773004/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:1120: * Handles all pending message for the current
isolate.
On 2016/02/03 21:33:36, turnidge wrote:
> message -> messages

Done.

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1375: DART_EXPORT void
Dart_SetShouldPauseOnStart(bool v) {
On 2016/02/03 21:33:36, turnidge wrote:
> v -> value or perhaps should_pause.

Done.

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1390: DART_EXPORT void Dart_SetPausedOnStart(bool v)
{
On 2016/02/03 21:33:36, turnidge wrote:
> v -> value or paused, here and below.

Done.

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1398: }
On 2016/02/03 21:33:36, turnidge wrote:
> As discussed offline, consider making a single api entry point
> (SetPausedOnStart) instead of the current two public ones (PausedOnStart,
> NotifyPauseOnStart).

Done.

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:1076: }
On 2016/02/03 21:33:36, turnidge wrote:
> As discussed offline, we may be able to remove
pause_isolates_flags_overridden_.
> 
> We would move the initialization of pause_on_start/pause_on_exit to the
> constructor for the message handler.  Then, inside the message handler, we
could
> use the runnable bit to determine whether or not we should look at the
value...

Done.

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:1076: }
On 2016/02/03 21:35:53, turnidge wrote:
> On 2016/02/03 21:33:36, turnidge wrote:
> > As discussed offline, we may be able to remove
> pause_isolates_flags_overridden_.
> > 
> > We would move the initialization of pause_on_start/pause_on_exit to the
> > constructor for the message handler.  Then, inside the message handler, we
> could
> > use the runnable bit to determine whether or not we should look at the
> value...
> 
> One more thing, we wouldn't want pause_on_start to ever happen for native
> message handlers, so we would probably guard the whole thing in an isolate()
!=
> NULL...

Acknowledged.

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/message_handler.h
File runtime/vm/message_handler.h (right):

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/message_handler....
runtime/vm/message_handler.h:59: // Handles all normals messages for this
message handler.  Should only
On 2016/02/03 21:33:37, turnidge wrote:
> Delete "normals".
> 
> This functions actually handles all messages, so consider naming it
> HandleMessages or perhaps HandleAllMessages.

Done.

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/message_handler....
runtime/vm/message_handler.h:88: bool pause_on_start() const {
On 2016/02/03 21:33:37, turnidge wrote:
> Consider renaming these to match your CL:
> 
> pause_on_start() -> ShouldPauseOnStart()
> set_pause_on_start() -> SetShouldPauseOnStart()
> paused_on_start() -> IsPausedOnStart(),
> 
> I think your new names are more clear than my old ones.
> 
> etc.
> 
> Perhaps rename the fields as well:
> 
> pause_on_start_ -> should_pause_on_start_.
> 
> You get the drill.

Done.

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/1665773004/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:2978: isolate->Resume();
On 2016/02/03 21:33:37, turnidge wrote:
> Does this send a resume event?  Do we want to suppress it?

No it doesn't and I've actually renamed this to be "SetResumeRequest" so that it
matches it's sibling "GetAndClearResumeRequest".

[turnidge, 2016-02-03 23:17:57]

lgtm with comments

https://codereview.chromium.org/1665773004/diff/40001/runtime/vm/message_hand...
File runtime/vm/message_handler.cc (right):

https://codereview.chromium.org/1665773004/diff/40001/runtime/vm/message_hand...
runtime/vm/message_handler.cc:313: status != MessageHandler::kShutdown);
I wonder if we can fold ShouldPause() into ShoudlPauseOnStart() and
ShouldPauseOnExit.

https://codereview.chromium.org/1665773004/diff/40001/runtime/vm/message_hand...
runtime/vm/message_handler.cc:464: if (paused) {
Do we want to ASSERT(!is_paused_on_start_)?

Similarly for the false case and for the cases in PausedOnExitLocked?

https://codereview.chromium.org/1665773004/diff/40001/runtime/vm/message_hand...
File runtime/vm/message_handler.h (right):

https://codereview.chromium.org/1665773004/diff/40001/runtime/vm/message_hand...
runtime/vm/message_handler.h:99: // even if we haven't hit the pause point yet.
This comment seems out of date.  Remove?  Problem predates your cl.

[Cutch, 2016-02-03 23:25:07]

https://codereview.chromium.org/1665773004/diff/40001/runtime/vm/message_hand...
File runtime/vm/message_handler.cc (right):

https://codereview.chromium.org/1665773004/diff/40001/runtime/vm/message_hand...
runtime/vm/message_handler.cc:313: status != MessageHandler::kShutdown);
On 2016/02/03 23:17:56, turnidge wrote:
> I wonder if we can fold ShouldPause() into ShoudlPauseOnStart() and
> ShouldPauseOnExit.

Done.

https://codereview.chromium.org/1665773004/diff/40001/runtime/vm/message_hand...
runtime/vm/message_handler.cc:464: if (paused) {
On 2016/02/03 23:17:57, turnidge wrote:
> Do we want to ASSERT(!is_paused_on_start_)?
> 
> Similarly for the false case and for the cases in PausedOnExitLocked?

Done.

https://codereview.chromium.org/1665773004/diff/40001/runtime/vm/message_hand...
File runtime/vm/message_handler.h (right):

https://codereview.chromium.org/1665773004/diff/40001/runtime/vm/message_hand...
runtime/vm/message_handler.h:99: // even if we haven't hit the pause point yet.
On 2016/02/03 23:17:57, turnidge wrote:
> This comment seems out of date.  Remove?  Problem predates your cl.

Done.

[Cutch, 2016-02-03 23:33:44]

Description was changed from

==========
Add necessary support functions so that embedders can implemented pause on start
and exit

- Add Dart_ShouldPauseOnStart, Dart_SetShouldPauseOnStart, Dart_IsPausedOnStart,
Dart_SetPausedOnStart, Dart_ShouldPauseOnExit, Dart_SetShouldPauseOnExit,
Dart_IsPausedOnExit, Dart_SetPausedOnExit to query and control pause on start /
exit from outside of the default message handler.
- Add Dart_HandleMessages to process all regular messages from outside the
default message handler.
- Allow per isolate overriding of pause on start / pause on exit flags.
- Set the resume_request bit when resuming from isolate start / exit.
- Clear the resume_request bit in the default message handler.
- Rename MessageHandler fields and accessors.
- Rename Isolate::Resume to Isolate::SetResumeRequest.
==========

to

==========
Add necessary support functions so that embedders can implemented pause on start
and exit

- Add Dart_ShouldPauseOnStart, Dart_SetShouldPauseOnStart, Dart_IsPausedOnStart,
Dart_SetPausedOnStart, Dart_ShouldPauseOnExit, Dart_SetShouldPauseOnExit,
Dart_IsPausedOnExit, Dart_SetPausedOnExit to query and control pause on start /
exit from outside of the default message handler.
- Add Dart_HandleMessages to process all regular messages from outside the
default message handler.
- Allow per isolate overriding of pause on start / pause on exit flags.
- Set the resume_request bit when resuming from isolate start / exit.
- Clear the resume_request bit in the default message handler.
- Rename MessageHandler fields and accessors.
- Rename Isolate::Resume to Isolate::SetResumeRequest.

R=turnidge@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6f76e2af994a08b1aa08dbf737991eece01186e0
==========

[Cutch, 2016-02-03 23:33:45]

Committed patchset #5 (id:80001) manually as
6f76e2af994a08b1aa08dbf737991eece01186e0 (presubmit successful).