Move chrome.runtime to //extensions.

Move chrome.runtime to //extensions.

This moves the runtime API schema and implementation to //extensions,
offloading some of its responsibilities out to a new RuntimeAPIDelegate
provided by the active ExtensionsBrowserClient.

BUG=369391
TBR=derat for mechanical chromeos changes
TBR=tzik for +webkit/browser/fileapi to extensions/browser/DEPS
TBR=sky for new chrome/test dependency

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268749

[Ken Rockot(use gerrit already), 2014-05-03 04:16:12]

I ended up using nocompile to pacify the runtime schema compilation. It turns
out that we really can't split the generated API code across different platforms
without either needing to generate different build files for app_shell vs other
targets (ew), or having two targets per api.gyp with each writing their outputs
to different paths (ew). Really worth the mess right now.

I feel pretty good about the delegate setup here, though some delegate methods
(like Un/RegisterUpdateObserver) seem like they shouldn't be necessary or should
live outside of the runtime API altogether. Anyway...

LMKWYT.

[Ken Rockot(use gerrit already), 2014-05-03 04:22:49]

https://chromiumcodereview.appspot.com/264743014/diff/20001/apps/shell/common...
File apps/shell/common/shell_extensions_client.cc (right):

https://chromiumcodereview.appspot.com/264743014/diff/20001/apps/shell/common...
apps/shell/common/shell_extensions_client.cc:25: //#include
"grit/common_resources.h"
Ugh, I must have left this here. Will fix later, please ignore.

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/browse...
File extensions/browser/runtime_api_delegate.cc (right):

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/browse...
extensions/browser/runtime_api_delegate.cc:2: // Use of this source code is
governed by a BSD-style license that can be
This file shouldn't exist. It will be deleted.

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

Better idea just to add nocompile, lucky things turned out that way. Guess I'll
add the platform thing later :)

A lot of naming nits (sorry) and rants (hope you enjoyed).

https://chromiumcodereview.appspot.com/264743014/diff/20001/chrome/browser/ex...
File chrome/browser/extensions/chrome_runtime_api_delegate.cc (right):

https://chromiumcodereview.appspot.com/264743014/diff/20001/chrome/browser/ex...
chrome/browser/extensions/chrome_runtime_api_delegate.cc:65: void
ChromeRuntimeAPIDelegate::RegisterUpdateObserver(
why is this called "RegisterUpdateObserver" while the same method on ES is
called "AddUpdateObserver"?

https://chromiumcodereview.appspot.com/264743014/diff/20001/chrome/browser/ex...
File chrome/browser/extensions/chrome_runtime_api_delegate.h (right):

https://chromiumcodereview.appspot.com/264743014/diff/20001/chrome/browser/ex...
chrome/browser/extensions/chrome_runtime_api_delegate.h:31: class
ChromeRuntimeAPIDelegate : public extensions::RuntimeAPIDelegate,
can this be in chrome/browser/extensions/api/runtime/...?

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/browse...
File extensions/browser/runtime_api_delegate.h (right):

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/browse...
extensions/browser/runtime_api_delegate.h:10: #include
"extensions/browser/api/runtime/runtime_api.h"
odd having a circular dependency like this, even if forward declarations help it
compile.

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/browse...
extensions/browser/runtime_api_delegate.h:19: 
comment

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/browse...
extensions/browser/runtime_api_delegate.h:23: virtual void
RegisterUpdateObserver(UpdateObserver* observer) = 0;
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.

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/browse...
extensions/browser/runtime_api_delegate.h:29: virtual base::Version
GetOldExtensionVersion(const Extension* extension) = 0;
the comment uses "previously" which is a better name than "old".
GetPreviousExtensionVersion?

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/browse...
extensions/browser/runtime_api_delegate.h:32: virtual void
MaybeReloadExtension(const std::string& extension_id) = 0;
What does it mean "if possible"? why "maybe"? It's an implementation detail.
I.e. just call this ReloadExtension.

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/browse...
extensions/browser/runtime_api_delegate.h:37: virtual bool RequestUpdateCheck(
"request" is implicit because you're calling it. "CheckForUpdates" would be
better.

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/browse...
extensions/browser/runtime_api_delegate.h:39: const
RuntimeAPI::UpdateCheckCallback& callback) = 0;
seems like UpdateCheckCallback (and its struct) should be defined in here, see
comment at top.

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/browse...
extensions/browser/runtime_api_delegate.h:43: virtual void HandleUninstall(const
std::string& extension_id,
"Handle uninstall" is an odd way to phrase this. it's a method, so it's
handling. "UninstallExtension" would be better.

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/browse...
extensions/browser/runtime_api_delegate.h:52: virtual bool
RequestRestart(std::string* error_message) = 0;
what does the return value mean?

also: RestartDevice() might be better.

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/common...
File extensions/common/api/_api_features.json (right):

https://chromiumcodereview.appspot.com/264743014/diff/20001/extensions/common...
extensions/common/api/_api_features.json:17: "extension": {
why did the extension API need to be moved?

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

Thanks. Got to rebase too, and changed these all to UIThreadExtensionFunctions
using the new typesafe Run(). Pretty cool stuff in practice.

On 2014/05/05 19:26:14, kalman wrote:
> Better idea just to add nocompile, lucky things turned out that way. Guess
I'll
> add the platform thing later :)

Actually I did start doing the platform thing anyway. I'll finish and upload it
soon.

https://codereview.chromium.org/264743014/diff/20001/apps/shell/common/shell_...
File apps/shell/common/shell_extensions_client.cc (right):

https://codereview.chromium.org/264743014/diff/20001/apps/shell/common/shell_...
apps/shell/common/shell_extensions_client.cc:25: //#include
"grit/common_resources.h"
On 2014/05/03 04:22:49, Ken Rockot wrote:
> Ugh, I must have left this here. Will fix later, please ignore.

Done.

https://codereview.chromium.org/264743014/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/chrome_runtime_api_delegate.cc (right):

https://codereview.chromium.org/264743014/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_runtime_api_delegate.cc:65: void
ChromeRuntimeAPIDelegate::RegisterUpdateObserver(
On 2014/05/05 19:26:15, kalman wrote:
> why is this called "RegisterUpdateObserver" while the same method on ES is
> called "AddUpdateObserver"?

I don't have a good answer. Changed.

https://codereview.chromium.org/264743014/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/chrome_runtime_api_delegate.h (right):

https://codereview.chromium.org/264743014/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_runtime_api_delegate.h:31: class
ChromeRuntimeAPIDelegate : public extensions::RuntimeAPIDelegate,
On 2014/05/05 19:26:15, kalman wrote:
> can this be in chrome/browser/extensions/api/runtime/...?

Done.

https://codereview.chromium.org/264743014/diff/20001/extensions/browser/runti...
File extensions/browser/runtime_api_delegate.h (right):

https://codereview.chromium.org/264743014/diff/20001/extensions/browser/runti...
extensions/browser/runtime_api_delegate.h:19: 
On 2014/05/05 19:26:15, kalman wrote:
> comment

Done.

https://codereview.chromium.org/264743014/diff/20001/extensions/browser/runti...
extensions/browser/runtime_api_delegate.h:23: virtual void
RegisterUpdateObserver(UpdateObserver* observer) = 0;
On 2014/05/05 19:26:15, kalman wrote:
> 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.

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.

https://codereview.chromium.org/264743014/diff/20001/extensions/browser/runti...
extensions/browser/runtime_api_delegate.h:29: virtual base::Version
GetOldExtensionVersion(const Extension* extension) = 0;
On 2014/05/05 19:26:15, kalman wrote:
> the comment uses "previously" which is a better name than "old".
> GetPreviousExtensionVersion?

Done.

https://codereview.chromium.org/264743014/diff/20001/extensions/browser/runti...
extensions/browser/runtime_api_delegate.h:32: virtual void
MaybeReloadExtension(const std::string& extension_id) = 0;
On 2014/05/05 19:26:15, kalman wrote:
> What does it mean "if possible"? why "maybe"? It's an implementation detail.
> I.e. just call this ReloadExtension.

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

https://codereview.chromium.org/264743014/diff/20001/extensions/browser/runti...
extensions/browser/runtime_api_delegate.h:37: virtual bool RequestUpdateCheck(
On 2014/05/05 19:26:15, kalman wrote:
> "request" is implicit because you're calling it. "CheckForUpdates" would be
> better.

Done.

https://codereview.chromium.org/264743014/diff/20001/extensions/browser/runti...
extensions/browser/runtime_api_delegate.h:39: const
RuntimeAPI::UpdateCheckCallback& callback) = 0;
On 2014/05/05 19:26:15, kalman wrote:
> seems like UpdateCheckCallback (and its struct) should be defined in here, see
> comment at top. 

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

https://codereview.chromium.org/264743014/diff/20001/extensions/browser/runti...
extensions/browser/runtime_api_delegate.h:43: virtual void HandleUninstall(const
std::string& extension_id,
On 2014/05/05 19:26:15, kalman wrote:
> "Handle uninstall" is an odd way to phrase this. it's a method, so it's
> handling. "UninstallExtension" would be better.

Renamed to OpenURL as discussed.

https://codereview.chromium.org/264743014/diff/20001/extensions/browser/runti...
extensions/browser/runtime_api_delegate.h:52: virtual bool
RequestRestart(std::string* error_message) = 0;
On 2014/05/05 19:26:15, kalman wrote:
> what does the return value mean?
> 
> also: RestartDevice() might be better.

Done.

https://codereview.chromium.org/264743014/diff/20001/extensions/common/api/_a...
File extensions/common/api/_api_features.json (right):

https://codereview.chromium.org/264743014/diff/20001/extensions/common/api/_a...
extensions/common/api/_api_features.json:17: "extension": {
On 2014/05/05 19:26:15, kalman wrote:
> why did the extension API need to be moved?

This was sort of a last minute change, since moving runtime was supposed to
eliminate app shell's dependence on chrome API features. As it turns out,
last_error.js still needs extension.lastError, and the shortest path to make it
available was to move these features.

I can save this for a separate CL if you'd prefer, or we can discuss some other
way of addressing the extension.lastError dependency.

[not at google - send to devlin, 2014-05-05 21:50:25]

lgtm

https://codereview.chromium.org/264743014/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/_api_features.json (left):

https://codereview.chromium.org/264743014/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/_api_features.json:305: "extension": {
as discussed, you should be able to change the custom bindings rather than this.

https://codereview.chromium.org/264743014/diff/40001/extensions/browser/api/r...
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/264743014/diff/40001/extensions/browser/api/r...
extensions/browser/api/runtime/runtime_api.cc:414: return
RespondNow(Error(kNoBackgroundPageError));
yay!

[Ken Rockot(use gerrit already), 2014-05-05 23:07:11]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-05-05 23:08:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/264743014/80001

[Ken Rockot(use gerrit already), 2014-05-05 23:22:23]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-05-06 00:45:49]

james could you ptal for apps/shell

[James Cook, 2014-05-06 15:57:02]

apps/shell LGTM but with several nits

https://codereview.chromium.org/264743014/diff/100001/apps/shell/browser/shel...
File apps/shell/browser/shell_runtime_api_delegate.cc (right):

https://codereview.chromium.org/264743014/diff/100001/apps/shell/browser/shel...
apps/shell/browser/shell_runtime_api_delegate.cc:6: 
#include PlatformInfo since you use it

https://codereview.chromium.org/264743014/diff/100001/apps/shell/browser/shel...
apps/shell/browser/shell_runtime_api_delegate.cc:42: info->os =
PlatformInfo::OS_CROS_;
Maybe ifdef OS_CHROMEOS and otherwise return Linux?  Dunno.

https://codereview.chromium.org/264743014/diff/100001/apps/shell/browser/shel...
File apps/shell/browser/shell_runtime_api_delegate.h (right):

https://codereview.chromium.org/264743014/diff/100001/apps/shell/browser/shel...
apps/shell/browser/shell_runtime_api_delegate.h:13: public:
out-of-line constructor and virtual destructor please

https://codereview.chromium.org/264743014/diff/100001/apps/shell/browser/shel...
apps/shell/browser/shell_runtime_api_delegate.h:28: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/264743014/diff/100001/apps/shell/common/shell...
File apps/shell/common/shell_extensions_client.cc (left):

https://codereview.chromium.org/264743014/diff/100001/apps/shell/common/shell...
apps/shell/common/shell_extensions_client.cc:138: // TODO(yoz): Don't include
Chrome resources.
Hooray!

https://codereview.chromium.org/264743014/diff/100001/extensions/browser/api/...
File extensions/browser/api/runtime/runtime_api_delegate.h (right):

https://codereview.chromium.org/264743014/diff/100001/extensions/browser/api/...
extensions/browser/api/runtime/runtime_api_delegate.h:64:
core_api::runtime::GetPlatformInfo::Results::PlatformInfo* info) = 0;
What do you think about forward-declaring this type?

[Ken Rockot(use gerrit already), 2014-05-06 17:22:22]

Heeeere we go

https://codereview.chromium.org/264743014/diff/100001/apps/shell/browser/shel...
File apps/shell/browser/shell_runtime_api_delegate.cc (right):

https://codereview.chromium.org/264743014/diff/100001/apps/shell/browser/shel...
apps/shell/browser/shell_runtime_api_delegate.cc:6: 
On 2014/05/06 15:57:03, James Cook wrote:
> #include PlatformInfo since you use it

Done.

https://codereview.chromium.org/264743014/diff/100001/apps/shell/browser/shel...
apps/shell/browser/shell_runtime_api_delegate.cc:42: info->os =
PlatformInfo::OS_CROS_;
On 2014/05/06 15:57:03, James Cook wrote:
> Maybe ifdef OS_CHROMEOS and otherwise return Linux?  Dunno.

Done.

https://codereview.chromium.org/264743014/diff/100001/apps/shell/browser/shel...
File apps/shell/browser/shell_runtime_api_delegate.h (right):

https://codereview.chromium.org/264743014/diff/100001/apps/shell/browser/shel...
apps/shell/browser/shell_runtime_api_delegate.h:13: public:
On 2014/05/06 15:57:03, James Cook wrote:
> out-of-line constructor and virtual destructor please

Done.

https://codereview.chromium.org/264743014/diff/100001/apps/shell/browser/shel...
apps/shell/browser/shell_runtime_api_delegate.h:28: };
On 2014/05/06 15:57:03, James Cook wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/264743014/diff/100001/extensions/browser/api/...
File extensions/browser/api/runtime/runtime_api_delegate.h (right):

https://codereview.chromium.org/264743014/diff/100001/extensions/browser/api/...
extensions/browser/api/runtime/runtime_api_delegate.h:64:
core_api::runtime::GetPlatformInfo::Results::PlatformInfo* info) = 0;
On 2014/05/06 15:57:03, James Cook wrote:
> What do you think about forward-declaring this type?

Fine by me. I didn't want to because of the horrible nesting, but I realized I
can just bump it up into the runtime namespace with no harmful side effects.
Done.

[Ken Rockot(use gerrit already), 2014-05-06 17:22:31]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-05-06 17:23:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/264743014/120001

[Ken Rockot(use gerrit already), 2014-05-06 18:06:03]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-05-06 18:52:38]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-05-06 18:54:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/264743014/140001

[Ken Rockot(use gerrit already), 2014-05-06 20:50:27]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-05-07 00:54:49]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-05-07 00:57:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/264743014/160001

[commit-bot: I haz the power, 2014-05-07 14:26:52]

Change committed as 268749