Move chrome.runtime to //extensions.

extensions/browser/runtime_api_delegate.h

+// Copyright 2014 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.
+
+#ifndef EXTENSIONS_BROWSER_RUNTIME_API_DELEGATE_H
+#define EXTENSIONS_BROWSER_RUNTIME_API_DELEGATE_H
+
+#include "base/callback.h"
+#include "base/version.h"
+#include "extensions/browser/api/runtime/runtime_api.h"

[not at google - send to devlin, 2014/05/05 19:26:15]

odd having a circular dependency like this, even if forward declarations help it
compile.

+#include "extensions/common/api/runtime.h"
+
+class GURL;
+
+namespace extensions {
+
+class Extension;
+class UpdateObserver;
+

[not at google - send to devlin, 2014/05/05 19:26:15]

comment

[Ken Rockot(use gerrit already), 2014/05/05 21:08:07]

Done.

+class RuntimeAPIDelegate {
+ public:
+  // Registers an UpdateObserver on behalf of the runtime API.
+  virtual void RegisterUpdateObserver(UpdateObserver* observer) = 0;

[not at google - send to devlin, 2014/05/05 19:26:15]

you're right -- it's odd that you need to do this, given UpdateObserver is in
//extensions. the managing of them should be in //extensions, too, e.g. strawman
would be to have something like
UpdateObserverList::Get(browser_context)->AddObserver(...), similar to how we do
that sort of thing for ExtensionRegsitry.

HOWEVER

Looking into UpdateObserver it's pretty weird. One method deals with updates to
apps. The other deals with updates to Chrome. At a high level I can see how this
makes sense -- apps are separate from the browser. Either the browser updates,
or an app updates.

However, from what I can tell the OnAppUpdateAvailable fires for all extensions
not just apps. And then tracing through the code it eventually calls just
runtime.onUpdateAvailable which has nothing to do with apps. So really it's just
a badly named function.

With all that in mind, perhaps "OnAppUpdateAvailable" should be moved to
ExtensionRegistry? ExtensionRegistry... might as well grow in that way.

As for browser restart... well, I guess we're back to having nowhere to put it.
The event is just a translation of the chrome::NOTIFICATION_UPGRADE_RECOMMENDED
notification. It's also only listened to from one place, ExtensionService, so
you could even be super lazy and just fire the runtime event from there
directly.

Take as much of this rant on board as you like.

[Ken Rockot(use gerrit already), 2014/05/05 21:08:07]

It's a good rant :)  I think I'd like to defer on this for now and leave these
delegate methods here.

A good approach for the near future would be to introduce a per-browser-context
ExtensionUpdateService, rather than extending ExtensionRegistry. This would
provide the update observer hooks ExtensionService has now, and it would
delegate the update checking/polling implementation to the browser client.

This feels like too big a change to make for this CL as it would touch a
significant portion of non-runtime-API-related code.

+
+  // Unregisters an UpdateObserver on behalf of the runtime API.
+  virtual void UnregisterUpdateObserver(UpdateObserver* observer) = 0;
+
+  // Determines an extension's previously installed version if applicable.
+  virtual base::Version GetOldExtensionVersion(const Extension* extension) = 0;

[not at google - send to devlin, 2014/05/05 19:26:15]

the comment uses "previously" which is a better name than "old".
GetPreviousExtensionVersion?

[Ken Rockot(use gerrit already), 2014/05/05 21:08:07]

Done.

+
+  // Reloads an extension if possible.
+  virtual void MaybeReloadExtension(const std::string& extension_id) = 0;

[not at google - send to devlin, 2014/05/05 19:26:15]

What does it mean "if possible"? why "maybe"? It's an implementation detail.
I.e. just call this ReloadExtension.

[Ken Rockot(use gerrit already), 2014/05/05 21:08:07]

Yeah, I realized that after I uploaded too... Done.

+
+  // Requests an extensions update update check. Returns |false| if updates
+  // are disabled. Otherwise |callback| is called with the result of the
+  // update check.
+  virtual bool RequestUpdateCheck(

[not at google - send to devlin, 2014/05/05 19:26:15]

"request" is implicit because you're calling it. "CheckForUpdates" would be
better.

[Ken Rockot(use gerrit already), 2014/05/05 21:08:07]

Done.

+      const std::string& extension_id,
+      const RuntimeAPI::UpdateCheckCallback& callback) = 0;

[not at google - send to devlin, 2014/05/05 19:26:15]

seems like UpdateCheckCallback (and its struct) should be defined in here, see
comment at top. 

[Ken Rockot(use gerrit already), 2014/05/05 21:08:07]

Yeah that's where they started. I have no idea why I moved them. Done.

+
+  // Handles uninstallation if the uninstalled extension provides an uninstall
+  // URL.
+  virtual void HandleUninstall(const std::string& extension_id,

[not at google - send to devlin, 2014/05/05 19:26:15]

"Handle uninstall" is an odd way to phrase this. it's a method, so it's
handling. "UninstallExtension" would be better.

[Ken Rockot(use gerrit already), 2014/05/05 21:08:07]

Renamed to OpenURL as discussed.

+                               const GURL& uninstall_url) = 0;
+
+  // Populates platform info to be provided by the getPlatformInfo function.
+  // Returns false iff no info is provided.
+  virtual bool GetPlatformInfo(
+      core_api::runtime::GetPlatformInfo::Results::PlatformInfo* info) = 0;
+
+  // Request a restart of the host device.
+  virtual bool RequestRestart(std::string* error_message) = 0;

[not at google - send to devlin, 2014/05/05 19:26:15]

what does the return value mean?

also: RestartDevice() might be better.

[Ken Rockot(use gerrit already), 2014/05/05 21:08:07]

Done.

+};
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_BROWSER_RUNTIME_API_DELEGATE_H