headless: Ensure all commands have parameters and return types

When generating C++ bindings, ensure all commands have a method which
takes a full parameters object and returns a return value object. This
ensures the API for experimental commands is retained even if that
command is made stable.

BUG=640919

Review-Url: https://codereview.chromium.org/2278123002
Cr-Commit-Position: refs/heads/master@{#414431}

headless/lib/browser/client_api_generator.py

@@ -373,16 +373,21 @@ def PatchExperimentalCommandsAndEvents(json_api):
         command['experimental'] = True
       for event in domain.get('events', []):
         event['experimental'] = True
+
+
+def EnsureCommandsHaveParametersAndReturnTypes(json_api):
+  """
+  Make sure all commands have at least empty parameters and return values. This
+  guarantees API compatibility if a previously experimental command is made
+  stable.
+  """
+  for domain in json_api['domains']:
     for command in domain.get('commands', []):
-      if not command.get('experimental', False):
-        continue
       if not 'parameters' in command:
         command['parameters'] = []
       if not 'returns' in command:
         command['returns'] = []
     for event in domain.get('events', []):
-      if not event.get('experimental', False):
-        continue
       if not 'parameters' in event:
         event['parameters'] = []
 
@@ -422,6 +427,7 @@ if __name__ == '__main__':
   json_api, output_dirname = ParseArguments(sys.argv[1:])
   jinja_env = InitializeJinjaEnv(output_dirname)
   PatchExperimentalCommandsAndEvents(json_api)
+  EnsureCommandsHaveParametersAndReturnTypes(json_api)
   SynthesizeCommandTypes(json_api)
   SynthesizeEventTypes(json_api)
   PatchFullQualifiedRefs(json_api)

headless/lib/browser/client_api_generator_unittest.py

@@ -370,37 +370,13 @@ class ClientApiGeneratorTest(unittest.TestCase):
         }
       ]
     }
-    expected_types = [
-      {
-        'type': 'object',
-        'id': 'FooCommandParams',
-        'description': 'Parameters for the FooCommand command.',
-        'properties': [],
-      },
-      {
-        'type': 'object',
-        'id': 'FooCommandResult',
-        'description': 'Result for the FooCommand command.',
-        'properties': [],
-      },
-      {
-        'type': 'object',
-        'id': 'BarEventParams',
-        'description': 'Parameters for the BarEvent event.',
-        'properties': [],
-      }
-    ]
     client_api_generator.PatchExperimentalCommandsAndEvents(json_api)
-    client_api_generator.SynthesizeCommandTypes(json_api)
-    client_api_generator.SynthesizeEventTypes(json_api)
     for command in json_api['domains'][0]['commands']:
       self.assertTrue(command['experimental'])
     for event in json_api['domains'][0]['events']:
       self.assertTrue(command['experimental'])
-    types = json_api['domains'][0]['types']
-    self.assertListEqual(types, expected_types)
 
-  def test_PatchExperimentalCommandsAndEvents(self):
+  def test_EnsureCommandsHaveParametersAndReturnTypes(self):
     json_api = {
       'domains': [
         {
@@ -408,13 +384,11 @@ class ClientApiGeneratorTest(unittest.TestCase):
           'commands': [
             {
               'name': 'FooCommand',
-              'experimental': True,
             }
           ],
           'events': [
             {
               'name': 'BarEvent',
-              'experimental': True,
             }
           ]
         }
@@ -440,13 +414,9 @@ class ClientApiGeneratorTest(unittest.TestCase):
         'properties': [],
       }
     ]
-    client_api_generator.PatchExperimentalCommandsAndEvents(json_api)
+    client_api_generator.EnsureCommandsHaveParametersAndReturnTypes(json_api)
     client_api_generator.SynthesizeCommandTypes(json_api)
     client_api_generator.SynthesizeEventTypes(json_api)
-    for command in json_api['domains'][0]['commands']:
-      self.assertTrue(command['experimental'])
-    for event in json_api['domains'][0]['events']:
-      self.assertTrue(command['experimental'])
     types = json_api['domains'][0]['types']
     self.assertListEqual(types, expected_types)
 

headless/lib/browser/domain_cc.template

@@ -32,19 +32,8 @@ void Domain::RemoveObserver(Observer* observer) {
 
     {% set class_name = 'ExperimentalDomain' if command.experimental else 'Domain' %}
     {% set method_name = command.name | to_title_case %}
-    {% if "parameters" in command and "returns" in command %}
 void {{class_name}}::{{method_name}}(std::unique_ptr<{{method_name}}Params> params, base::Callback<void(std::unique_ptr<{{method_name}}Result>)> callback) {
   dispatcher_->SendMessage("{{domain.domain}}.{{command.name}}", params->Serialize(), base::Bind(&Domain::Handle{{method_name}}Response, callback));
-    {% elif "parameters" in command %}
-void {{class_name}}::{{method_name}}(std::unique_ptr<{{method_name}}Params> params, base::Callback<void()> callback) {
-  dispatcher_->SendMessage("{{domain.domain}}.{{command.name}}", params->Serialize(), std::move(callback));
-    {% elif "returns" in command %}
-void {{class_name}}::{{method_name}}(base::Callback<void(std::unique_ptr<{{method_name}}Result>)> callback) {
-  dispatcher_->SendMessage("{{domain.domain}}.{{command.name}}", base::Bind(&Domain::Handle{{method_name}}Response, std::move(callback)));
-    {% else %}
-void {{class_name}}::{{method_name}}(base::Callback<void()> callback) {
-  dispatcher_->SendMessage("{{domain.domain}}.{{command.name}}", std::move(callback));
-    {% endif %}
 }
     {# Generate convenience methods that take the required parameters directly. #}
     {% if not "parameters" in command %}{% continue %}{% endif %}
@@ -59,8 +48,8 @@ void {{class_name}}::{{method_name}}({##}
         {% if not loop.first %}, {% endif %}
 {{resolve_type(parameter).pass_type}} {{parameter.name | camelcase_to_hacker_style -}}
     {% endfor %}
-    {% if "parameters" in command and not command.parameters[0].get("optional", False) %}, {% endif %}{# -#}
-    {% if "returns" in command -%}
+    {% if command.get("parameters", []) and not command.parameters[0].get("optional", False) %}, {% endif %}{# -#}
+    {% if command.get("returns", []) -%}
       base::Callback<void(std::unique_ptr<{{method_name}}Result>)> callback{##}
     {% else -%}
       base::Callback<void()> callback{##}
@@ -75,8 +64,19 @@ void {{class_name}}::{{method_name}}({##}
     {% endfor %}
       .Build();
     {# Send the message. #}
-  {{method_name}}(std::move(params), std::move(callback));
+    {% if command.get("returns", []) -%}
+  dispatcher_->SendMessage("{{domain.domain}}.{{command.name}}", params->Serialize(), base::Bind(&Domain::Handle{{method_name}}Response, callback));
+    {% else %}
+  dispatcher_->SendMessage("{{domain.domain}}.{{command.name}}", params->Serialize(), std::move(callback));
+    {% endif %}
+}
+    {# If the command has no return value, generate a convenience method that #}
+    {# accepts a base::Callback<void()> together with the parameters object. #}
+    {% if not command.get("returns", []) %}
+void {{class_name}}::{{method_name}}(std::unique_ptr<{{method_name}}Params> params, base::Callback<void()> callback) {
+  dispatcher_->SendMessage("{{domain.domain}}.{{command.name}}", params->Serialize(), std::move(callback));
 }
+    {% endif %}
   {% endfor %}
 
 {# Generate response handlers for commands that need them. #}

headless/lib/browser/domain_h.template

@@ -20,15 +20,7 @@
   {% if command.description %}
   // {{ command.description }}
   {% endif %}
-  {% if "parameters" in command and "returns" in command %}
   void {{method_name}}(std::unique_ptr<{{method_name}}Params> params, base::Callback<void(std::unique_ptr<{{method_name}}Result>)> callback = base::Callback<void(std::unique_ptr<{{method_name}}Result>)>());
-  {% elif "parameters" in command %}
-  void {{method_name}}(std::unique_ptr<{{method_name}}Params> params, base::Callback<void()> callback = base::Callback<void()>());
-  {% elif "returns" in command %}
-  void {{method_name}}(base::Callback<void(std::unique_ptr<{{method_name}}Result>)> callback = base::Callback<void(std::unique_ptr<{{method_name}}Result>)>());
-  {% else %}
-  void {{method_name}}(base::Callback<void()> callback = base::Callback<void()>());
-  {% endif %}
   {# Generate convenience methods that take the required parameters directly. #}
   {# Don't generate these for experimental commands. #}
   {% if "parameters" in command and not command.experimental %}
@@ -40,12 +32,17 @@
       {% if not loop.first %}, {% endif %}
 {{resolve_type(parameter).pass_type}} {{parameter.name | camelcase_to_hacker_style -}}
     {% endfor %}
-    {% if "parameters" in command and not command.parameters[0].get("optional", False) %}, {% endif %}{# -#}
-    {% if "returns" in command -%}
+    {% if command.get("parameters", []) and not command.parameters[0].get("optional", False) %}, {% endif %}{# -#}
+    {% if command.get("returns", []) -%}
       base::Callback<void(std::unique_ptr<{{method_name}}Result>)> callback = base::Callback<void(std::unique_ptr<{{method_name}}Result>)>(){##}
     {% else -%}
       base::Callback<void()> callback = base::Callback<void()>(){##}
     {% endif %});
+    {# If the command has no return value, generate a convenience method that #}
+    {# accepts a base::Callback<void()> together with the parameters object. #}
+    {% if not command.get("returns", []) %}
+  void {{method_name}}(std::unique_ptr<{{method_name}}Params> params, base::Callback<void()> callback);
+    {% endif %}
   {% endif %}
 {% endmacro %}
 

headless/lib/headless_devtools_client_browsertest.cc

@@ -192,6 +192,11 @@ class HeadlessDevToolsClientExperimentalTest
                 .SetEnabled(false)
                 .Build());
 
+    // Check that a previously experimental command which takes no parameters
+    // still works by giving it a parameter object.
+    devtools_client_->GetRuntime()->GetExperimental()->RunIfWaitingForDebugger(
+        runtime::RunIfWaitingForDebuggerParams::Builder().Build());
+
     devtools_client_->GetPage()->GetExperimental()->AddObserver(this);
     devtools_client_->GetPage()->Enable();
     devtools_client_->GetPage()->Navigate(
@@ -200,8 +205,15 @@ class HeadlessDevToolsClientExperimentalTest
 
   void OnFrameStoppedLoading(
       const page::FrameStoppedLoadingParams& params) override {
-    FinishAsynchronousTest();
+    // Check that a non-experimental command which has no return value can be
+    // called with a void() callback.
+    devtools_client_->GetPage()->Reload(
+        page::ReloadParams::Builder().Build(),
+        base::Bind(&HeadlessDevToolsClientExperimentalTest::OnReloadStarted,
+                   base::Unretained(this)));
   }
+
+  void OnReloadStarted() { FinishAsynchronousTest(); }
 };
 
 HEADLESS_ASYNC_DEVTOOLED_TEST_F(HeadlessDevToolsClientExperimentalTest);