Refactor service usage in Dartium

Refactor service usage in Dartium.

Paired with: https://codereview.chromium.org/125103004/

R=asiva@google.com

Committed: https://src.chromium.org/viewvc/multivm/branches/1650/blink?view=rev&revision=165096

[Jacob, 2014-01-08 18:47:09]

I like seeing all the deleted lines! This looks like it is headed in a good
direction.

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartCon...
File Source/bindings/dart/DartController.cpp (right):

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:257: if (true) {
remove
if (true) {

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:410: void
DartController::shutdownIsolateCallback(void* data)
The method name seems generic while the behavior is specific to the service.
Perhaps use the name
shutdownServiceIsolateCallback?

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:419: Dart_Isolate serviceIsolate =
DartController::createIsolate("dart:vmservice_dartium", "main", document, true,
error);
As the package is internal only, change the name from
dart:vmservice_dartium
to
dart:_vmservice_dartium

and add an entry to 
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/sdk/...
specifying that
documented: false

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
File Source/bindings/dart/DartService.cpp (right):

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:18: #define SHUTDOWN_ON_ERROR(handle)      
               \
Consider moving this to DartUtils or using existing helpers in DartUtils (if
any).

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:38: m_errorMsg = "Internal error.";
add a better error message

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:46: ASSERT(library != Dart_Null());
why not assert that this is a library?

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:124: "library vmservice_dartium;\n"
this script should be moved to an internal library
e.g. 
dart:_vmservice_dartium
rather than specified in code here.

[siva, 2014-01-10 00:10:13]

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartCon...
File Source/bindings/dart/DartController.cpp (right):

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:414: 
See comment in other CL, we need to see if it makes sense to auto
register/unregister then this callback would not be necessary.

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
File Source/bindings/dart/DartService.cpp (right):

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:36: Dart_Isolate isolate =
Dart_GetServiceIsolate(document);
As discussed offline we should see if it is possible to just get the ServicePort
here instead of the isolate

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:101: // we must be careful about entering
the isolate.
Is this comment valid? You indicated that the service isolate has to be a DOM
isolate in order to post replies.

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:116: }
If the API handed back the service request port this sort of field access would
not be necessary.

[Cutch, 2014-01-10 21:34:26]

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartCon...
File Source/bindings/dart/DartController.cpp (right):

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:257: if (true) {
On 2014/01/08 18:47:10, Jacob wrote:
> remove
> if (true) {

Done.

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:410: void
DartController::shutdownIsolateCallback(void* data)
On 2014/01/08 18:47:10, Jacob wrote:
> The method name seems generic while the behavior is specific to the service.
> Perhaps use the name
> shutdownServiceIsolateCallback?

This method was registered as the isolate shutdown callback, hence the name. I
don't need a shutdown callback anymore so I've dropped it.

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:419: Dart_Isolate serviceIsolate =
DartController::createIsolate("dart:vmservice_dartium", "main", document, true,
error);
On 2014/01/08 18:47:10, Jacob wrote:
> As the package is internal only, change the name from
> dart:vmservice_dartium
> to
> dart:_vmservice_dartium
> 
> and add an entry to 
>
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/sdk/...
> specifying that
> documented: false

This isn't a library that a user could ever encounter. It will never be visible
outside of the VM controlled service isolate.

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
File Source/bindings/dart/DartService.cpp (right):

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:18: #define SHUTDOWN_ON_ERROR(handle)      
               \
On 2014/01/08 18:47:10, Jacob wrote:
> Consider moving this to DartUtils or using existing helpers in DartUtils (if
> any).

This code is specific to the service startup code. It also must be a macro
because it returns from the function.

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:38: m_errorMsg = "Internal error.";
On 2014/01/08 18:47:10, Jacob wrote:
> add a better error message

Done.

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:46: ASSERT(library != Dart_Null());
On 2014/01/08 18:47:10, Jacob wrote:
> why not assert that this is a library?

Done.

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:101: // we must be careful about entering
the isolate.
On 2014/01/10 00:10:13, siva wrote:
> Is this comment valid? You indicated that the service isolate has to be a DOM
> isolate in order to post replies.

The comment was optimistic that one day the service won't be running inside a
DOM isolate.

https://codereview.chromium.org/128733002/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:124: "library vmservice_dartium;\n"
On 2014/01/08 18:47:10, Jacob wrote:
> this script should be moved to an internal library
> e.g. 
> dart:_vmservice_dartium
> rather than specified in code here.

We do not want to add the VM service glue script to any internal libraries or as
part of the snapshot. It makes sense to define it here because it will only ever
be used in this file.

[siva, 2014-01-14 18:57:34]

lgtm

https://codereview.chromium.org/128733002/diff/120001/Source/bindings/dart/Da...
File Source/bindings/dart/DartService.cpp (right):

https://codereview.chromium.org/128733002/diff/120001/Source/bindings/dart/Da...
Source/bindings/dart/DartService.cpp:176: "}\n";
Maybe you could move this string out into a global string so that in the future
if the library tag handler needs to get the source corresponding to
"dart:vmservice_dartium" it can return that string.

https://codereview.chromium.org/128733002/diff/120001/Source/bindings/dart/Da...
Source/bindings/dart/DartService.cpp:179: Dart_Handle source =
Dart_NewStringFromCString(kScriptChars);
Ditto comment about using Dart_NewStringFromUTF8 instead of NewStringFromCString
as the script could contain some string literals that are not C string
compatible.

https://codereview.chromium.org/128733002/diff/120001/Source/bindings/dart/Da...
File Source/bindings/dart/DartService.h (right):

https://codereview.chromium.org/128733002/diff/120001/Source/bindings/dart/Da...
Source/bindings/dart/DartService.h:38: static bool IsRunning() { return
m_isolate != 0; }  // NOLINT
I think we don't compare against 0 in dartium code, instead we just use return
m_isolate here that would avoid the NOLINT comment.

[Cutch, 2014-01-14 21:17:17]

https://codereview.chromium.org/128733002/diff/120001/Source/bindings/dart/Da...
File Source/bindings/dart/DartService.cpp (right):

https://codereview.chromium.org/128733002/diff/120001/Source/bindings/dart/Da...
Source/bindings/dart/DartService.cpp:176: "}\n";
On 2014/01/14 18:57:34, siva wrote:
> Maybe you could move this string out into a global string so that in the
future
> if the library tag handler needs to get the source corresponding to
> "dart:vmservice_dartium" it can return that string.

Done.

https://codereview.chromium.org/128733002/diff/120001/Source/bindings/dart/Da...
Source/bindings/dart/DartService.cpp:179: Dart_Handle source =
Dart_NewStringFromCString(kScriptChars);
On 2014/01/14 18:57:34, siva wrote:
> Ditto comment about using Dart_NewStringFromUTF8 instead of
NewStringFromCString
> as the script could contain some string literals that are not C string
> compatible.

Done.

https://codereview.chromium.org/128733002/diff/120001/Source/bindings/dart/Da...
File Source/bindings/dart/DartService.h (right):

https://codereview.chromium.org/128733002/diff/120001/Source/bindings/dart/Da...
Source/bindings/dart/DartService.h:38: static bool IsRunning() { return
m_isolate != 0; }  // NOLINT
On 2014/01/14 18:57:34, siva wrote:
> I think we don't compare against 0 in dartium code, instead we just use return
> m_isolate here that would avoid the NOLINT comment.

Done.

[Cutch, 2014-01-14 22:36:03]

Committed patchset #2 manually as r165096 (presubmit successful).