Revert 53892 - Initial scriptable object implementation.

Broke ChromiumOS ARM build, reverting. Errors from log:
remoting/client/plugin/chromoting_scriptable_object.cc: In member function 'virtual bool remoting::ChromotingScriptableObject::HasProperty(const pp::Var&, pp::Var*)':
remoting/client/plugin/chromoting_scriptable_object.cc:48: error: NULL used in arithmetic
remoting/client/plugin/chromoting_scriptable_object.cc: In member function 'virtual bool remoting::ChromotingScriptableObject::HasMethod(const pp::Var&, pp::Var*)':
remoting/client/plugin/chromoting_scriptable_object.cc:63: error: NULL used in arithmetic

BUG=50248
TEST=write javascript to manually setup connection.

Review URL: http://codereview.chromium.org/3064009

TBR=ajwong@chromium.org
Review URL: http://codereview.chromium.org/3020038

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@53949 0039d316-1c4b-4281-b951-d872f2087c98

remoting/client/client_util.cc

@@ -78,4 +78,42 @@ bool GetLoginInfoFromArgs(int argc, char** argv, ClientConfig* config) {
   return true;
 }
 
+// Get host JID from command line arguments, or stdin if not specified.
+bool GetLoginInfoFromUrlParams(const std::string& url, ClientConfig* config) {
+  // TODO(ajwong): We should use GURL or something.  Don't parse this by hand!
+
+  // The Url should be of the form:
+  //
+  //   chrome://remoting?user=<userid>&auth=<authtoken>&jid=<hostjid>
+  //
+  vector<string> parts;
+  SplitString(url, '&', &parts);
+  if (parts.size() != 3) {
+    return false;
+  }
+
+  size_t pos = parts[0].rfind('=');
+  if (pos == string::npos && (pos + 1) != string::npos) {
+    return false;
+  }
+  std::string username = parts[0].substr(pos + 1);
+
+  pos = parts[1].rfind('=');
+  if (pos == string::npos && (pos + 1) != string::npos) {
+    return false;
+  }
+  std::string auth_token = parts[1].substr(pos + 1);
+
+  pos = parts[2].rfind('=');
+  if (pos == string::npos && (pos + 1) != string::npos) {
+    return false;
+  }
+  std::string host_jid = parts[2].substr(pos + 1);
+
+  config->host_jid = host_jid;
+  config->username = username;
+  config->auth_token = auth_token;
+  return true;
+}
+
 }  // namespace remoting

remoting/client/client_util.h

@@ -16,6 +16,10 @@ struct ClientConfig;
 // Return true if successful.
 bool GetLoginInfoFromArgs(int argc, char** argv, ClientConfig* config);
 
+// Get the login info from the URL params and write values into |config|.
+// Return true if successful.
+bool GetLoginInfoFromUrlParams(const std::string& url, ClientConfig* config);
+
 }  // namespace remoting
 
 #endif  // REMOTING_CLIENT_CLIENT_UTIL_H_

remoting/client/client_util_unittest.cc

@@ -10,11 +10,26 @@
 
 namespace remoting {
 
-// TODO(ajwong): Fill this out.
+// TODO(ajwong): Once ChromotingPlugin stablizes a little more, come up with
+// sane unittests.
 class ClientUtilTest : public testing::Test {
  protected:
   virtual void SetUp() {
   }
 };
 
+TEST_F(ClientUtilTest, GetLoginInfoFromUrlParams) {
+  const char url[] = "chromotocol://hostid?user=auser&auth=someauth&jid=ajid";
+  std::string user_id;
+  std::string auth_token;
+  std::string host_jid;
+  ClientConfig config;
+
+  ASSERT_TRUE(GetLoginInfoFromUrlParams(url, &config));
+
+  EXPECT_EQ("auser", config.username);
+  EXPECT_EQ("someauth", config.auth_token);
+  EXPECT_EQ("ajid", config.host_jid);
+}
+
 }  // namespace remoting

remoting/client/plugin/chromoting_plugin.cc

@@ -15,13 +15,12 @@
 #include "remoting/client/chromoting_client.h"
 #include "remoting/client/host_connection.h"
 #include "remoting/client/jingle_host_connection.h"
-#include "remoting/client/plugin/chromoting_scriptable_object.h"
 #include "remoting/client/plugin/pepper_input_handler.h"
 #include "remoting/client/plugin/pepper_view.h"
 #include "remoting/jingle_glue/jingle_thread.h"
 #include "third_party/ppapi/c/pp_event.h"
+#include "third_party/ppapi/c/pp_rect.h"
 #include "third_party/ppapi/cpp/completion_callback.h"
-#include "third_party/ppapi/cpp/rect.h"
 
 namespace remoting {
 
@@ -41,7 +40,9 @@ ChromotingPlugin::~ChromotingPlugin() {
   // to the message loop before this point.  Right now, we don't have a well
   // defined stop for the plugin process, and the thread shutdown is likely a
   // race condition.
-  context_.Stop();
+  if (context_.get()) {
+    context_->Stop();
+  }
 }
 
 bool ChromotingPlugin::Init(uint32_t argc,
@@ -62,48 +63,60 @@ bool ChromotingPlugin::Init(uint32_t argc,
   pepper_main_loop_dont_post_to_me_ = MessageLoop::current();
   LOG(INFO) << "Started ChromotingPlugin::Init";
 
-  // Start all the threads.
-  context_.Start();
+  // Extract the URL from the arguments.
+  const char* url = NULL;
+  for (uint32_t i = 0; i < argc; ++i) {
+    if (strcmp(argn[i], "src") == 0) {
+      url = argv[i];
+      break;
+    }
+  }
+
+  if (!url) {
+    return false;
+  }
+
+  ClientConfig config;
+  if (!GetLoginInfoFromUrlParams(url, &config)) {
+    LOG(WARNING) << "Could not parse URL: " << url;
+    return false;
+  }
 
   // Create the chromoting objects.
-  host_connection_.reset(new JingleHostConnection(&context_));
+  host_connection_.reset(new JingleHostConnection(context_.get()));
   view_.reset(new PepperView(this));
   input_handler_.reset(new PepperInputHandler());
-
-  // Default to a medium grey.
-  view_->SetSolidFill(0xFFCDCDCD);
-
-  return true;
-}
-
-void ChromotingPlugin::Connect(const ClientConfig& config) {
-  DCHECK(CurrentlyOnPluginThread());
-
   client_.reset(new ChromotingClient(config,
-                                     &context_,
+                                     context_.get(),
                                      host_connection_.get(),
                                      view_.get(),
                                      input_handler_.get(),
                                      NULL));
 
+  // Default to a medium grey.
+  view_->SetSolidFill(0xFFCDCDCD);
+
   // Kick off the connection.
+  context_->Start();
   client_->Start();
+
+  return true;
 }
 
-void ChromotingPlugin::ViewChanged(const pp::Rect& position,
-                                   const pp::Rect& clip) {
+void ChromotingPlugin::ViewChanged(const PP_Rect& position,
+                                   const PP_Rect& clip) {
   DCHECK(CurrentlyOnPluginThread());
 
   // TODO(ajwong): This is going to be a race condition when the view changes
   // and we're in the middle of a Paint().
   LOG(INFO) << "ViewChanged "
-            << position.x() << ","
-            << position.y() << ","
-            << position.width() << ","
-            << position.height();
+            << position.point.x << ","
+            << position.point.y << ","
+            << position.size.width << ","
+            << position.size.height;
 
-  view_->SetViewport(position.x(), position.y(),
-                     position.width(), position.height());
+  view_->SetViewport(position.point.x, position.point.y,
+                       position.size.width, position.size.height);
   view_->Paint();
 }
 
@@ -134,18 +147,4 @@ bool ChromotingPlugin::HandleEvent(const PP_Event& event) {
   return false;
 }
 
-pp::Var ChromotingPlugin::GetInstanceObject() {
-  LOG(ERROR) << "Getting instance object.";
-  if (instance_object_.is_void()) {
-    ChromotingScriptableObject* object = new ChromotingScriptableObject(this);
-    object->Init();
-
-    LOG(ERROR) << "Object initted.";
-    // The pp::Var takes ownership of object here.
-    instance_object_ = pp::Var(object);
-  }
-
-  return instance_object_;
-}
-
 }  // namespace remoting

remoting/client/plugin/chromoting_plugin.h

@@ -12,7 +12,6 @@
 
 #include "base/at_exit.h"
 #include "base/scoped_ptr.h"
-#include "remoting/client/client_context.h"
 #include "remoting/client/host_connection.h"
 #include "testing/gtest/include/gtest/gtest_prod.h"
 #include "third_party/ppapi/c/pp_event.h"
@@ -21,7 +20,6 @@
 #include "third_party/ppapi/c/pp_resource.h"
 #include "third_party/ppapi/cpp/instance.h"
 #include "third_party/ppapi/cpp/device_context_2d.h"
-#include "third_party/ppapi/cpp/var.h"
 
 class MessageLoop;
 
@@ -51,14 +49,13 @@ class ChromotingPlugin : public pp::Instance {
   virtual ~ChromotingPlugin();
 
   virtual bool Init(uint32_t argc, const char* argn[], const char* argv[]);
-  virtual void Connect(const ClientConfig& config);
   virtual bool HandleEvent(const PP_Event& event);
-  virtual pp::Var GetInstanceObject();
-  virtual void ViewChanged(const pp::Rect& position, const pp::Rect& clip);
+  virtual void ViewChanged(const PP_Rect& position, const PP_Rect& clip);
 
   virtual bool CurrentlyOnPluginThread() const;
 
  private:
+  FRIEND_TEST(ChromotingPluginTest, ParseUrl);
   FRIEND_TEST(ChromotingPluginTest, TestCaseSetup);
 
   // Since we're an internal plugin, we can just grab the message loop during
@@ -69,12 +66,15 @@ class ChromotingPlugin : public pp::Instance {
   // TODO(ajwong): Think if there is a better way to safeguard this.
   MessageLoop* pepper_main_loop_dont_post_to_me_;
 
-  ClientContext context_;
+  scoped_ptr<ClientContext> context_;
+
   scoped_ptr<HostConnection> host_connection_;
+
   scoped_ptr<PepperView> view_;
+
   scoped_ptr<InputHandler> input_handler_;
+
   scoped_ptr<ChromotingClient> client_;
-  pp::Var instance_object_;  // JavaScript interface to control this instance.
 
   DISALLOW_COPY_AND_ASSIGN(ChromotingPlugin);
 };

remoting/client/plugin/chromoting_scriptable_object.cc

-// Copyright 2010 Google Inc. All Rights Reserved.
-// Author: ajwong@google.com (Albert Wong)
-
-#include "remoting/client/plugin/chromoting_scriptable_object.h"
-
-#include "remoting/client/client_config.h"
-#include "remoting/client/plugin/chromoting_plugin.h"
-
-#include "third_party/ppapi/cpp/var.h"
-
-using pp::Var;
-
-namespace remoting {
-ChromotingScriptableObject::ChromotingScriptableObject(
-    ChromotingPlugin* instance)
-    : instance_(instance) {
-}
-
-ChromotingScriptableObject::~ChromotingScriptableObject() {
-}
-
-void ChromotingScriptableObject::Init() {
-  // Property addition order should match the interface description at the
-  // top of chromoting_scriptable_object.h.
-  AddAttribute("onreadystatechange", Var());
-
-  AddAttribute("NOT_CONNECTED", Var(0));
-  AddAttribute("CONNECTED", Var(1));
-
-  AddMethod("connect", &ChromotingScriptableObject::DoConnect);
-
-  // TODO(ajwong): Figure out how to implement the status variable.
-  AddAttribute("status", Var("not_implemented_yet"));
-}
-
-bool ChromotingScriptableObject::HasProperty(const Var& name, Var* exception) {
-  // TODO(ajwong): Check if all these name.is_string() sentinels are required.
-  if (!name.is_string()) {
-    *exception = Var("HasProperty expects a string for the name.");
-    return false;
-  }
-
-  PropertyNameMap::const_iterator iter = property_names_.find(name.AsString());
-  if (iter == property_names_.end()) {
-    return false;
-  }
-
-  return properties_[iter->second].method == NULL;
-}
-
-bool ChromotingScriptableObject::HasMethod(const Var& name, Var* exception) {
-  // TODO(ajwong): Check if all these name.is_string() sentinels are required.
-  if (!name.is_string()) {
-    *exception = Var("HasMethod expects a string for the name.");
-    return false;
-  }
-
-  PropertyNameMap::const_iterator iter = property_names_.find(name.AsString());
-  if (iter == property_names_.end()) {
-    return false;
-  }
-
-  return properties_[iter->second].method != NULL;
-}
-
-Var ChromotingScriptableObject::GetProperty(const Var& name, Var* exception) {
-  // TODO(ajwong): Check if all these name.is_string() sentinels are required.
-  if (!name.is_string()) {
-    *exception = Var("GetProperty expects a string for the name.");
-    return Var();
-  }
-
-  PropertyNameMap::const_iterator iter = property_names_.find(name.AsString());
-
-  // No property found.
-  if (iter == property_names_.end()) {
-    return ScriptableObject::GetProperty(name, exception);
-  }
-
-  // TODO(ajwong): This incorrectly return a null object if a function
-  // property is requested.
-  return properties_[iter->second].attribute;
-}
-
-void ChromotingScriptableObject::GetAllPropertyNames(
-    std::vector<Var>* properties,
-    Var* exception) {
-  for (size_t i = 0; i < properties_.size(); i++) {
-    properties->push_back(Var(properties_[i].name));
-  }
-}
-
-void ChromotingScriptableObject::SetProperty(const Var& name,
-                                             const Var& value,
-                                             Var* exception) {
-  // TODO(ajwong): Check if all these name.is_string() sentinels are required.
-  if (!name.is_string()) {
-    *exception = Var("SetProperty expects a string for the name.");
-    return;
-  }
-
-  // Only allow writing to onreadystatechange.  See top of
-  // chromoting_scriptable_object.h for the object interface definition.
-  std::string property_name = name.AsString();
-  if (property_name != "onstatechange") {
-    *exception =
-        Var("Cannot set property " + property_name + " on this object.");
-    return;
-  }
-
-  // Since we're whitelisting the propertie that are settable above, we can
-  // assume that the property exists in the map.
-  properties_[property_names_[property_name]].attribute = value;
-}
-
-Var ChromotingScriptableObject::Call(const Var& method_name,
-                                     const std::vector<Var>& args,
-                                     Var* exception) {
-  PropertyNameMap::const_iterator iter =
-      property_names_.find(method_name.AsString());
-  if (iter == property_names_.end()) {
-    return pp::ScriptableObject::Call(method_name, args, exception);
-  }
-
-  return (this->*(properties_[iter->second].method))(args, exception);
-}
-
-void ChromotingScriptableObject::AddAttribute(const std::string& name,
-                                              Var attribute) {
-  property_names_[name] = properties_.size();
-  properties_.push_back(PropertyDescriptor(name, attribute));
-}
-
-void ChromotingScriptableObject::AddMethod(const std::string& name,
-                                           MethodHandler handler) {
-  property_names_[name] = properties_.size();
-  properties_.push_back(PropertyDescriptor(name, handler));
-}
-
-pp::Var ChromotingScriptableObject::DoConnect(const std::vector<Var>& args,
-                                           Var* exception) {
-  if (args.size() != 3) {
-    *exception = Var("Usage: connect(username, host_jid, auth_token");
-    return Var();
-  }
-
-  ClientConfig config;
-
-  if (!args[0].is_string()) {
-    *exception = Var("The username must be a string.");
-    return Var();
-  }
-  config.username = args[0].AsString();
-
-  if (!args[1].is_string()) {
-    *exception = Var("The host_jid must be a string.");
-    return Var();
-  }
-  config.host_jid = args[1].AsString();
-
-  if (!args[2].is_string()) {
-    *exception = Var("The auth_token must be a string.");
-    return Var();
-  }
-  config.auth_token = args[2].AsString();
-
-  instance_->Connect(config);
-
-  return Var();
-}
-
-}  // namespace remoting

remoting/client/plugin/chromoting_scriptable_object.h

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-// This implements the JavaScript class entrypoint for the plugin.
-// The Javascript API is defined as follows.
-//
-// interface ChromotingScriptableObject {
-//   // Called when the Chromoting plugin has had a state change such as
-//   // connection completed.
-//   attribute Function onreadystatechange;
-//
-//   // Constants for states, etc.
-//   const unsigned short NOT_CONNECTED = 0;
-//   const unsigned short CONNECTED = 1;
-//
-//   // Methods on the object.
-//   void connect(string username, string host_jid, string auth_token);
-//
-//   // Attributes.
-//   readonly attribute unsigned short status;
-// }
-//
-//  onreadystatechange
-//
-// Methods:
-//   Connect(username, auth_token, host_jid, onstatechange);
-
-#ifndef REMOTING_CLIENT_PLUGIN_CHROMOTING_SCRIPTABLE_OBJECT_H_
-#define REMOTING_CLIENT_PLUGIN_CHROMOTING_SCRIPTABLE_OBJECT_H_
-
-#include <map>
-#include <string>
-#include <vector>
-
-#include "third_party/ppapi/cpp/scriptable_object.h"
-#include "third_party/ppapi/cpp/var.h"
-
-namespace remoting {
-
-class ChromotingPlugin;
-
-class ChromotingScriptableObject : public pp::ScriptableObject {
- public:
-  explicit ChromotingScriptableObject(ChromotingPlugin* instance);
-  virtual ~ChromotingScriptableObject();
-
-  virtual void Init();
-
-  // Override the ScriptableObject functions.
-  virtual bool HasProperty(const pp::Var& name, pp::Var* exception);
-  virtual bool HasMethod(const pp::Var& name, pp::Var* exception);
-  virtual pp::Var GetProperty(const pp::Var& name, pp::Var* exception);
-  virtual void GetAllPropertyNames(std::vector<pp::Var>* properties,
-                                   pp::Var* exception);
-  virtual void SetProperty(const pp::Var& name,
-                           const pp::Var& value,
-                           pp::Var* exception);
-  virtual pp::Var Call(const pp::Var& method_name,
-                       const std::vector<pp::Var>& args,
-                       pp::Var* exception);
-
- private:
-  typedef std::map<std::string, int> PropertyNameMap;
-  typedef pp::Var (ChromotingScriptableObject::*MethodHandler)(
-      const std::vector<pp::Var>& args, pp::Var* exception);
-  struct PropertyDescriptor {
-    explicit PropertyDescriptor(const std::string& n, pp::Var a)
-        : name(n), attribute(a), method(NULL) {
-    }
-
-    explicit PropertyDescriptor(const std::string& n, MethodHandler m)
-        : name(n), method(m) {
-    }
-
-    enum Type {
-      ATTRIBUTE,
-      METHOD,
-    } type;
-
-    std::string name;
-    pp::Var attribute;
-    MethodHandler method;
-  };
-
-
-  void AddAttribute(const std::string& name, pp::Var attribute);
-  void AddMethod(const std::string& name, MethodHandler handler);
-
-  pp::Var DoConnect(const std::vector<pp::Var>& args, pp::Var* exception);
-
-  PropertyNameMap property_names_;
-  std::vector<PropertyDescriptor> properties_;
-
-  ChromotingPlugin* instance_;
-};
-
-}  // namespace remoting
-
-#endif  // REMOTING_CLIENT_PLUGIN_CHROMOTING_SCRIPTABLE_OBJECT_H_

remoting/client/plugin/pepper_view.cc

@@ -43,14 +43,12 @@ void PepperView::Paint() {
     return;
   }
 
-  // TODO(ajwong): We're assuming the native format is BGRA_PREMUL below. This
-  // is wrong.
-  pp::ImageData image(pp::ImageData::GetNativeImageDataFormat(),
+  // TODO(ajwong): We shouldn't assume the image data format.
+  pp::ImageData image(PP_IMAGEDATAFORMAT_BGRA_PREMUL,
                       pp::Size(viewport_width_, viewport_height_),
                       false);
   if (image.is_null()) {
-    LOG(ERROR) << "Unable to allocate image of size: "
-               << viewport_width_ << "x" << viewport_height_;
+    LOG(ERROR) << "Unable to allocate image.";
     return;
   }
 

remoting/client/x11_view.cc

@@ -73,7 +73,7 @@ void X11View::TearDown() {
 
 void X11View::Paint() {
   // Don't bother attempting to paint if the display hasn't been set up.
-  if (!display_ || !window_ || !height_ || !width_ || !frame_) {
+  if (!display_ || !window_ || !height_ || !width_) {
     return;
   }
 
@@ -146,8 +146,6 @@ void X11View::SetHostScreenSize(int width, int height) {
   width_ = width;
   height_ = height;
   XResizeWindow(display_, window_, width_, height_);
-  media::VideoFrame::CreateFrame(media::VideoFrame::RGB32, width_, height_,
-                                 base::TimeDelta(), base::TimeDelta(), &frame_);
 }
 
 void X11View::InitPaintTarget() {

remoting/remoting.gyp

@@ -78,8 +78,6 @@
       'sources': [
         'client/plugin/chromoting_plugin.cc',
         'client/plugin/chromoting_plugin.h',
-        'client/plugin/chromoting_scriptable_object.cc',
-        'client/plugin/chromoting_scriptable_object.h',
         'client/plugin/pepper_entrypoints.cc',
         'client/plugin/pepper_entrypoints.h',
         'client/plugin/pepper_input_handler.cc',