Enable VM service inside Dartium Renderer processes

This CL does the following:

* When the main DOM isolate is created the VM service isolate is also initialized.
* Whenever isolates come up or go down they register with the VM service.
* Provides a mechanism for C++ code to query the VM service and have a callback called with the reply from the service.
* Adds a pane to Chrome DevTools which renders the Observatory which is hosted on a remote server.
* Adds messages to the devtools protocol for the observatory to communicate with.

Notes:

* There is one VM service isolate per renderer process.
* Whether or not the service is initialized can be controlled by the environment variable DARTIUM_VMSERVICE.
* Instead of an HTTP server the VM service running in a Dartium renderer process has a Dart_Port for submitting requests. The VM service responds to requests by posting to a native port.
* The VM service shares almost all Dart source code with the standalone VM service.

R=jacobr@google.com

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

[Jacob, 2013-12-04 20:39:52]

Looks good! Here's a first batch of comments.

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

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:236: ASSERT(Dart_CurrentIsolate() ==
m_isolate);
why is this assert here?

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:239: bool r =
DartService::Start(document);
can you add a flag that controls whether the DartService is turned on?
We shouldn't check this in with the service on 100% of the time.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:249: // The following is only used for
testing and will be removed.
add a fixme for code that should be removed.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:442:
DartService::SendIsolateStartupMessage();
put enterscope, exit scope in
{

} 
by convention
or at least add whitespace

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

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:17: // These must be kept in sync with
vmservice/constants.dart
nit: these could be kept in sync automatically by adding a native getter so
these constants are only defined once on the C++ side. Perhaps add a FIXME for
this

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:24: namespace dart {
having files with the namespace dart::bin
seems strange. Any way to move this defn to the impl file or keep it with the
standard webcore namespace?

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:107: const char* scriptUri = "vmservice:";
move this constant outside of the method

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:119: SHUTDOWN_ON_ERROR(result);
for consistency use the
Scope auto-release helper classes and the goto error pattern used elsewhere in
this directory.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:237: Dart_Handle list =
MakeServiceControlMessage(
there isn't an 80 char line limit in this directory.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:288: break;
here and elsewhere, remove { 

} 
for single line if statements.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:307: static bool IsDartLibrary(const char*
url)
I assume this is probably defined somewhere else as well

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:320: char buffer[1024];
add FIXME to not hardcode 1024 here.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:383: DART_UNIMPLEMENTED();
DART_UNIMPLEMENTED seems wrong here. this is hitting for a grab bag of error
cases.
Also, DART_UNIMPLEMENTED should only be at the last line of a function and care
needs to be taken that no stack allocated objects exist.

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

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartServiceInternal.cpp:15: 
add a TODO and bug to get this file moved over to the vm.  Dartium should be
using purely the VM embedder APi.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/gyp/res...
File Source/bindings/dart/gyp/resources_sources.gypi (right):

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/gyp/res...
Source/bindings/dart/gyp/resources_sources.gypi:8:
'../../../../../../dart/runtime/bin/vmservice/constants.dart',
this should be specified in the ../../../../dart 
directory instead.

If that is not possible, file a bug and list it here under a FIXME

[Cutch, 2013-12-04 21:20:50]

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

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:236: ASSERT(Dart_CurrentIsolate() ==
m_isolate);
On 2013/12/04 20:39:52, Jacob wrote:
> why is this assert here?

It was just verifying my assumption. Removed.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:239: bool r =
DartService::Start(document);
On 2013/12/04 20:39:52, Jacob wrote:
> can you add a flag that controls whether the DartService is turned on?
> We shouldn't check this in with the service on 100% of the time.

Done.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:249: // The following is only used for
testing and will be removed.
On 2013/12/04 20:39:52, Jacob wrote:
> add a fixme for code that should be removed.

Done.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:442:
DartService::SendIsolateStartupMessage();
On 2013/12/04 20:39:52, Jacob wrote:
> put enterscope, exit scope in
> {
> 
> } 
> by convention
> or at least add whitespace

Done.

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

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:17: // These must be kept in sync with
vmservice/constants.dart
On 2013/12/04 20:39:52, Jacob wrote:
> nit: these could be kept in sync automatically by adding a native getter so
> these constants are only defined once on the C++ side. Perhaps add a FIXME for
> this

Done.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:24: namespace dart {
On 2013/12/04 20:39:52, Jacob wrote:
> having files with the namespace dart::bin
> seems strange. Any way to move this defn to the impl file or keep it with the
> standard webcore namespace?

Added TODO. Moving the namespace requires surgery to runtime/bin and some build
scripts.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:107: const char* scriptUri = "vmservice:";
On 2013/12/04 20:39:52, Jacob wrote:
> move this constant outside of the method

Done.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:119: SHUTDOWN_ON_ERROR(result);
On 2013/12/04 20:39:52, Jacob wrote:
> for consistency use the
> Scope auto-release helper classes and the goto error pattern used elsewhere in
> this directory.

Done.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:237: Dart_Handle list =
MakeServiceControlMessage(
On 2013/12/04 20:39:52, Jacob wrote:
> there isn't an 80 char line limit in this directory.

Done.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:288: break;
On 2013/12/04 20:39:52, Jacob wrote:
> here and elsewhere, remove { 
> 
> } 
> for single line if statements.

Done.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:307: static bool IsDartLibrary(const char*
url)
On 2013/12/04 20:39:52, Jacob wrote:
> I assume this is probably defined somewhere else as well

It actually is not.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:320: char buffer[1024];
On 2013/12/04 20:39:52, Jacob wrote:
> add FIXME to not hardcode 1024 here.

Done.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.cpp:383: DART_UNIMPLEMENTED();
On 2013/12/04 20:39:52, Jacob wrote:
> DART_UNIMPLEMENTED seems wrong here. this is hitting for a grab bag of error
> cases.
> Also, DART_UNIMPLEMENTED should only be at the last line of a function and
care
> needs to be taken that no stack allocated objects exist.

Replaced with ASSERT_NOT_REACHED

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

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartServiceInternal.cpp:15: 
On 2013/12/04 20:39:52, Jacob wrote:
> add a TODO and bug to get this file moved over to the vm.  Dartium should be
> using purely the VM embedder APi.

Done.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/gyp/res...
File Source/bindings/dart/gyp/resources_sources.gypi (right):

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/gyp/res...
Source/bindings/dart/gyp/resources_sources.gypi:8:
'../../../../../../dart/runtime/bin/vmservice/constants.dart',
On 2013/12/04 20:39:52, Jacob wrote:
> this should be specified in the ../../../../dart 
> directory instead.
> 
> If that is not possible, file a bug and list it here under a FIXME

Filed bug and made note in file.

[Cutch, 2013-12-05 00:57:41]

Patch Set 4 is ready for a real review.

[Jacob, 2013-12-05 19:31:30]

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartController.cpp (right):

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
Source/bindings/dart/DartController.cpp:234: const bool shouldStartService =
!getenv("DARTIUM_VMSERVICE");
no need for const in cases like this as they add more syntax clutter than value.

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartService.cpp (right):

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
Source/bindings/dart/DartService.cpp:112: return false;
Use Scope auto close opbjects here and elsewhere to avoid accidentally missing a
matching close call (where possible)

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
Source/bindings/dart/DartService.cpp:190: {
remove return values that provide no value.

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
Source/bindings/dart/DartService.cpp:336: {
no need for the extra {
} 
here.

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartService.h (right):

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
Source/bindings/dart/DartService.h:25: char* GetRequestString() { return
m_request; }
can this at least be a const char*?

To meet WebKit style guides it would be nice to use wtf::String in most places
and only convert to c_string directly before calling low level code, etc.

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartServiceInternal.cpp (right):

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
Source/bindings/dart/DartServiceInternal.cpp:8: // TODO(johnmccutchan): Expose
these methods to the embedder API so
Match Webkit style for TODO
add a bug #.

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/gyp...
File Source/bindings/dart/gyp/resources_sources.gypi (right):

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/gyp...
Source/bindings/dart/gyp/resources_sources.gypi:5: # TODO(johnmccutchan): Move
these sources into runtime. Issue #15450
runtime --> /dart/runtime

[Cutch, 2013-12-05 22:48:15]

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartController.cpp (right):

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
Source/bindings/dart/DartController.cpp:234: const bool shouldStartService =
!getenv("DARTIUM_VMSERVICE");
On 2013/12/05 19:31:31, Jacob wrote:
> no need for const in cases like this as they add more syntax clutter than
value.

Done.

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartService.cpp (right):

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
Source/bindings/dart/DartService.cpp:112: return false;
On 2013/12/05 19:31:31, Jacob wrote:
> Use Scope auto close opbjects here and elsewhere to avoid accidentally missing
a
> matching close call (where possible)

Everywhere but the startup and shutdown of the VM service isolate uses the
Isolate and API scope objects.

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
Source/bindings/dart/DartService.cpp:336: {
On 2013/12/05 19:31:31, Jacob wrote:
> no need for the extra {
> } 
> here.

Done.

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartService.h (right):

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
Source/bindings/dart/DartService.h:25: char* GetRequestString() { return
m_request; }
On 2013/12/05 19:31:31, Jacob wrote:
> can this at least be a const char*?
> 
> To meet WebKit style guides it would be nice to use wtf::String in most places
> and only convert to c_string directly before calling low level code, etc.

Done.

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartServiceInternal.cpp (right):

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/Dar...
Source/bindings/dart/DartServiceInternal.cpp:8: // TODO(johnmccutchan): Expose
these methods to the embedder API so
On 2013/12/05 19:31:31, Jacob wrote:
> Match Webkit style for TODO
> add a bug #.

Done.

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/gyp...
File Source/bindings/dart/gyp/resources_sources.gypi (right):

https://codereview.chromium.org/104433004/diff/50001/Source/bindings/dart/gyp...
Source/bindings/dart/gyp/resources_sources.gypi:5: # TODO(johnmccutchan): Move
these sources into runtime. Issue #15450
On 2013/12/05 19:31:31, Jacob wrote:
> runtime --> /dart/runtime

Done.

[Jacob, 2013-12-05 22:56:12]

last round of comments

https://codereview.chromium.org/104433004/diff/90001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartServiceInternal.cpp (right):

https://codereview.chromium.org/104433004/diff/90001/Source/bindings/dart/Dar...
Source/bindings/dart/DartServiceInternal.cpp:8: // TODO(johnmccutchan): Expose
these methods to the embedder API so
the webkit style is to avoid TODO with a user name.
switch to 
FIXME and add a bug#

https://codereview.chromium.org/104433004/diff/90001/Source/core/inspector/In...
File Source/core/inspector/InspectorDartAgent.cpp (right):

https://codereview.chromium.org/104433004/diff/90001/Source/core/inspector/In...
Source/core/inspector/InspectorDartAgent.cpp:2: * Copyright (C) 2011 Google Inc.
All rights reserved.
2013

https://codereview.chromium.org/104433004/diff/90001/Source/core/inspector/In...
Source/core/inspector/InspectorDartAgent.cpp:59: if (m_frontend) {
add ASSERT(m_frontend)

https://codereview.chromium.org/104433004/diff/90001/Source/core/inspector/In...
Source/core/inspector/InspectorDartAgent.cpp:69:
InspectorDartAgent::~InspectorDartAgent()
remove no-op method.

https://codereview.chromium.org/104433004/diff/90001/Source/core/inspector/In...
File Source/core/inspector/InspectorDartAgent.h (right):

https://codereview.chromium.org/104433004/diff/90001/Source/core/inspector/In...
Source/core/inspector/InspectorDartAgent.h:2: * Copyright (C) 2011 Google Inc.
All rights reserved.
2013

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
File Source/devtools/front_end/DartPanel.js (right):

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/DartPanel.js:2: * Copyright (C) 2012 Google Inc. All
rights reserved.
2012-->2013

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
File Source/devtools/front_end/DartView.js (right):

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/DartView.js:2: * Copyright (C) 2012 Google Inc. All
rights reserved.
2012->2013

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/DartView.js:40: // FIXME this one isn't hooked up on
the other side. is it needed?
remove FIXME. it is out of date

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/DartView.js:43: window.console.log("XXX payload is:",
id, data);
remove XXX log message.

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
File Source/devtools/front_end/inspector.js (right):

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/inspector.js:47: var console = new
WebInspector.PanelDescriptor("console", WebInspector.UIString("Console"),
"ConsolePanel");
comment out these next 4 lines before checkin so users aren't confused.

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/inspector.js:177: // Once drawer is closed console
should be shown if it was shown before current view replaced it in drawer.
add back in the space to keep our diff small with upstream.

[Jacob, 2013-12-05 22:59:58]

https://codereview.chromium.org/104433004/diff/110001/Source/bindings/dart/Da...
File Source/bindings/dart/DartController.cpp (right):

https://codereview.chromium.org/104433004/diff/110001/Source/bindings/dart/Da...
Source/bindings/dart/DartController.cpp:234: bool shouldStartService =
!getenv("DARTIUM_VMSERVICE");
remove this local variable and include this directly in the if clause.

https://codereview.chromium.org/104433004/diff/110001/Source/core/inspector/I...
File Source/core/inspector/InspectorDartAgent.cpp (right):

https://codereview.chromium.org/104433004/diff/110001/Source/core/inspector/I...
Source/core/inspector/InspectorDartAgent.cpp:50: class
InspectorDartAgentDartServiceRequest : public DartServiceRequest {
add a fixme to use ref counting rather than manual memory management for this
class.

https://codereview.chromium.org/104433004/diff/110001/Source/devtools/protoco...
File Source/devtools/protocol.json (left):

https://codereview.chromium.org/104433004/diff/110001/Source/devtools/protoco...
Source/devtools/protocol.json:1109: },
remove whitespace diff here

https://codereview.chromium.org/104433004/diff/110001/Source/devtools/protoco...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/104433004/diff/110001/Source/devtools/protoco...
Source/devtools/protocol.json:3258: { "name": "isContentScript", "type":
"boolean", "optional": true, "description": "Determines whether this script is a
user extension script." },
remove whitespace only diff here.

[Cutch, 2013-12-05 23:09:36]

https://codereview.chromium.org/104433004/diff/90001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartServiceInternal.cpp (right):

https://codereview.chromium.org/104433004/diff/90001/Source/bindings/dart/Dar...
Source/bindings/dart/DartServiceInternal.cpp:8: // TODO(johnmccutchan): Expose
these methods to the embedder API so
On 2013/12/05 22:56:12, Jacob wrote:
> the webkit style is to avoid TODO with a user name.
> switch to 
> FIXME and add a bug#

Done.

https://codereview.chromium.org/104433004/diff/90001/Source/core/inspector/In...
File Source/core/inspector/InspectorDartAgent.cpp (right):

https://codereview.chromium.org/104433004/diff/90001/Source/core/inspector/In...
Source/core/inspector/InspectorDartAgent.cpp:2: * Copyright (C) 2011 Google Inc.
All rights reserved.
On 2013/12/05 22:56:12, Jacob wrote:
> 2013

Done.

https://codereview.chromium.org/104433004/diff/90001/Source/core/inspector/In...
Source/core/inspector/InspectorDartAgent.cpp:59: if (m_frontend) {
On 2013/12/05 22:56:12, Jacob wrote:
> add ASSERT(m_frontend)

Done.

https://codereview.chromium.org/104433004/diff/90001/Source/core/inspector/In...
Source/core/inspector/InspectorDartAgent.cpp:69:
InspectorDartAgent::~InspectorDartAgent()
On 2013/12/05 22:56:12, Jacob wrote:
> remove no-op method.

Done.

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
File Source/devtools/front_end/DartPanel.js (right):

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/DartPanel.js:2: * Copyright (C) 2012 Google Inc. All
rights reserved.
On 2013/12/05 22:56:12, Jacob wrote:
> 2012-->2013

Done.

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
File Source/devtools/front_end/DartView.js (right):

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/DartView.js:40: // FIXME this one isn't hooked up on
the other side. is it needed?
On 2013/12/05 22:56:12, Jacob wrote:
> remove FIXME. it is out of date

Done.

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/DartView.js:43: window.console.log("XXX payload is:",
id, data);
On 2013/12/05 22:56:12, Jacob wrote:
> remove XXX log message.

Done.

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
File Source/devtools/front_end/inspector.js (right):

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/inspector.js:47: var console = new
WebInspector.PanelDescriptor("console", WebInspector.UIString("Console"),
"ConsolePanel");
On 2013/12/05 22:56:12, Jacob wrote:
> comment out these next 4 lines before checkin so users aren't confused.

Done.

https://codereview.chromium.org/104433004/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/inspector.js:177: // Once drawer is closed console
should be shown if it was shown before current view replaced it in drawer.
On 2013/12/05 22:56:12, Jacob wrote:
> add back in the space to keep our diff small with upstream.

Done.

https://codereview.chromium.org/104433004/diff/110001/Source/bindings/dart/Da...
File Source/bindings/dart/DartController.cpp (right):

https://codereview.chromium.org/104433004/diff/110001/Source/bindings/dart/Da...
Source/bindings/dart/DartController.cpp:234: bool shouldStartService =
!getenv("DARTIUM_VMSERVICE");
On 2013/12/05 22:59:58, Jacob wrote:
> remove this local variable and include this directly in the if clause.

Done.

https://codereview.chromium.org/104433004/diff/110001/Source/devtools/protoco...
File Source/devtools/protocol.json (left):

https://codereview.chromium.org/104433004/diff/110001/Source/devtools/protoco...
Source/devtools/protocol.json:1109: },
On 2013/12/05 22:59:58, Jacob wrote:
> remove whitespace diff here

Done.

https://codereview.chromium.org/104433004/diff/110001/Source/devtools/protoco...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/104433004/diff/110001/Source/devtools/protoco...
Source/devtools/protocol.json:3258: { "name": "isContentScript", "type":
"boolean", "optional": true, "description": "Determines whether this script is a
user extension script." },
On 2013/12/05 22:59:58, Jacob wrote:
> remove whitespace only diff here.

Done.

[Jacob, 2013-12-05 23:12:13]

lgtm

[Cutch, 2013-12-05 23:26:22]

Committed patchset #9 manually as r1624.

[siva, 2013-12-13 21:48:22]

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

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:243: Dart_EnterIsolate(m_isolate);
Why not move the EnterScope/ExitScope stuff to SendIsolateStartupMessage().

Also use DartApiScope instead of the Enter/Exit stuff.

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:440: Dart_Isolate isolate =
createIsolate(scriptURL, entryPoint, document, false, errorMsg);
Ditto comment about Enter/Exit Scope

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartCon...
Source/bindings/dart/DartController.cpp:590:
Dart_Initialize(&createPureIsolateCallback, 0, 0,
DartService::VmServiceShutdownCallback, openFileCallback, readFileCallback,
writeFileCallback, closeFileCallback, generateEntropy);
You should have a  generic shutdown callback and inside of that call
DartService::VmServiceShutdownCallback if it is the VM service Isolate.

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

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartService.h:49: }
}  // namespace WebCore

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
File Source/bindings/dart/DartServiceInternal.h (right):

https://codereview.chromium.org/104433004/diff/1/Source/bindings/dart/DartSer...
Source/bindings/dart/DartServiceInternal.h:19: }
}  // namespace WebCore

https://codereview.chromium.org/104433004/diff/150001/Source/bindings/dart/Da...
File Source/bindings/dart/DartController.cpp (right):

https://codereview.chromium.org/104433004/diff/150001/Source/bindings/dart/Da...
Source/bindings/dart/DartController.cpp:238: if (getenv("DARTIUM_VMSERVICE")) {
I am hoping this will be a stop gap arrangement until we find a better way of
starting up the vm service, maybe it could be inside the VM and vm flags would
take care of it.

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

https://codereview.chromium.org/104433004/diff/150001/Source/bindings/dart/Da...
Source/bindings/dart/DartService.h:27: const String& GetRequestString() { return
m_request; };
GetRequestString() const { ... }

https://codereview.chromium.org/104433004/diff/150001/Source/bindings/dart/Da...
File Source/bindings/dart/DartServiceInternal.cpp (right):

https://codereview.chromium.org/104433004/diff/150001/Source/bindings/dart/Da...
Source/bindings/dart/DartServiceInternal.cpp:26: #include "vm/snapshot.h"  //
NOLINT.
I am presuming all these includes are also temporary, we are supposed to only
use the API calls from external code.