Move ContentScripts out of Extension

Move ContentScripts out of Extension

TBR=ben@chromium.org (gypis)

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

[Devlin, 2012-12-29 21:18:37]

https://codereview.chromium.org/11724002/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/content_scripts/content_scripts_handler.cc
(right):

https://codereview.chromium.org/11724002/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/content_scripts/content_scripts_handler.cc:149:
extension->set_wants_file_access(true);
This is bad. Do we know how we want to handle things like this? We could have
wants_file_access as manifest data, but that seems a little odd...

[Yoyo Zhou, 2013-01-04 00:13:30]

It's good to see that this doesn't even require the renderer to know about
parsing content_scripts. Just a few comments.

https://codereview.chromium.org/11724002/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/content_scripts/content_scripts_handler.cc
(right):

https://codereview.chromium.org/11724002/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/content_scripts/content_scripts_handler.cc:149:
extension->set_wants_file_access(true);
On 2012/12/29 21:18:38, D Cronin wrote:
> This is bad. Do we know how we want to handle things like this? We could have
> wants_file_access as manifest data, but that seems a little odd...

I'd put a TODO saying what's wrong, at least until we get (host) permissions
straightened out; it seems like "wants file access" pertains to content scripts,
so eventually we may have a ContentScriptsInfo::WantsFileAccess.

https://codereview.chromium.org/11724002/diff/7001/chrome/common/extensions/e...
File chrome/common/extensions/extension_unittest.cc (right):

https://codereview.chromium.org/11724002/diff/7001/chrome/common/extensions/e...
chrome/common/extensions/extension_unittest.cc:760:
&ContentScriptsInfo::GetContentScripts(extension)[0], NULL));
NULL on a new line?

https://codereview.chromium.org/11724002/diff/7001/chrome/common/extensions/m...
File
chrome/common/extensions/manifest_tests/extension_manifests_excludematches_unittest.cc
(right):

https://codereview.chromium.org/11724002/diff/7001/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_excludematches_unittest.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
This is also a content scripts test - put it in the same directory as content
scripts.

[Devlin, 2013-01-11 20:30:02]

https://codereview.chromium.org/11724002/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/content_scripts/content_scripts_handler.cc
(right):

https://codereview.chromium.org/11724002/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/content_scripts/content_scripts_handler.cc:149:
extension->set_wants_file_access(true);
On 2013/01/04 00:13:30, Yoyo Zhou wrote:
> On 2012/12/29 21:18:38, D Cronin wrote:
> > This is bad. Do we know how we want to handle things like this? We could
have
> > wants_file_access as manifest data, but that seems a little odd...
> 
> I'd put a TODO saying what's wrong, at least until we get (host) permissions
> straightened out; it seems like "wants file access" pertains to content
scripts,
> so eventually we may have a ContentScriptsInfo::WantsFileAccess.

Done (in Extension.h, with the method).

https://codereview.chromium.org/11724002/diff/7001/chrome/common/extensions/e...
File chrome/common/extensions/extension_unittest.cc (right):

https://codereview.chromium.org/11724002/diff/7001/chrome/common/extensions/e...
chrome/common/extensions/extension_unittest.cc:760:
&ContentScriptsInfo::GetContentScripts(extension)[0], NULL));
On 2013/01/04 00:13:30, Yoyo Zhou wrote:
> NULL on a new line?

Done.

https://codereview.chromium.org/11724002/diff/7001/chrome/common/extensions/m...
File
chrome/common/extensions/manifest_tests/extension_manifests_excludematches_unittest.cc
(right):

https://codereview.chromium.org/11724002/diff/7001/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_excludematches_unittest.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/01/04 00:13:30, Yoyo Zhou wrote:
> This is also a content scripts test - put it in the same directory as content
> scripts.

Done.

[Yoyo Zhou, 2013-01-11 23:19:44]

LGTM

https://chromiumcodereview.appspot.com/11724002/diff/17001/chrome/browser/pro...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://chromiumcodereview.appspot.com/11724002/diff/17001/chrome/browser/pro...
chrome/browser/profiles/profile_dependency_manager.cc:266:
extensions::ProfileKeyedAPIFactory<extensions::ContentScriptsAPI>::
Please rebase to head to get the latest version of this.

[Devlin, 2013-01-28 19:03:35]

+ mirandac for c/b/profiles/

[Miranda Callahan, 2013-02-06 22:10:54]

sorry for the delay, I've been OOO. profiles/ lgtm with nit.

https://chromiumcodereview.appspot.com/11724002/diff/58001/chrome/browser/pro...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://chromiumcodereview.appspot.com/11724002/diff/58001/chrome/browser/pro...
chrome/browser/profiles/profile_dependency_manager.cc:261:
extensions::ContentScriptsAPI::GetFactoryInstance();
nit: alphabetical order.

[commit-bot: I haz the power, 2013-03-20 17:01:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/11724002/1...

[not at google - send to devlin, 2013-03-20 17:13:38]

https://chromiumcodereview.appspot.com/11724002/diff/108001/chrome/browser/ex...
File chrome/browser/extensions/api/content_scripts/content_scripts_api.cc
(right):

https://chromiumcodereview.appspot.com/11724002/diff/108001/chrome/browser/ex...
chrome/browser/extensions/api/content_scripts/content_scripts_api.cc:5: #include
"chrome/browser/extensions/api/content_scripts/content_scripts_api.h"
There is a (loose) convention at the moment where everything in
chrome/browser/extensions/api is an actual API implementation, think a matching
schema in chrome/common/extensions/api. This stuff isn't really an API, it's a
feature of the platform. Did you consider putting this in just
chrome/browser/extensions/content_scripts?

[Devlin, 2013-03-20 17:40:49]

On 2013/03/20 17:13:38, kalman wrote:
>
https://chromiumcodereview.appspot.com/11724002/diff/108001/chrome/browser/ex...
> File chrome/browser/extensions/api/content_scripts/content_scripts_api.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/11724002/diff/108001/chrome/browser/ex...
> chrome/browser/extensions/api/content_scripts/content_scripts_api.cc:5:
#include
> "chrome/browser/extensions/api/content_scripts/content_scripts_api.h"
> There is a (loose) convention at the moment where everything in
> chrome/browser/extensions/api is an actual API implementation, think a
matching
> schema in chrome/common/extensions/api. This stuff isn't really an API, it's a
> feature of the platform. Did you consider putting this in just
> chrome/browser/extensions/content_scripts?

I wasn't sure if that was still the convention with the recent refactors going
around (since I'm not aware of a themes, icons, or plugins API - all of which
are in c/c/e/api, though admittedly all are recent patches -, and since others
which *do* have api implementations are spread out, e.g.
c/b/speech/extension_api/). I'm more than happy to move this out, but if it's
okay, I'd like to wait to see if it goes through and do it in a separate CL, so
that chances of update failure are reduced.  If the patch fails here, I'll make
the update in this CL; otherwise, should I also be moving out those other
extension features which do not have a corresponding API?

[not at google - send to devlin, 2013-03-20 18:07:49]

lgtm (so cq doesn't fail, I hope)

yep sounds fine. Yeah, I'd prefer we keep api/ with things that
DECLARE_FUNCTION_NAME or whatever (and surrounding files). Don't know what
happened with themes/icons.

(and sounds like speech hasn't been moved yet is all - the schema compilation
basically enforces that it's under api/ so they'll eventually all end up there)

[Yoyo Zhou, 2013-03-20 19:18:29]

On 2013/03/20 18:07:49, kalman wrote:
> lgtm (so cq doesn't fail, I hope)
> 
> yep sounds fine. Yeah, I'd prefer we keep api/ with things that
> DECLARE_FUNCTION_NAME or whatever (and surrounding files). Don't know what
> happened with themes/icons.
> 
> (and sounds like speech hasn't been moved yet is all - the schema compilation
> basically enforces that it's under api/ so they'll eventually all end up
there)

lgtm
Yeah, I think the api directory is more suitable for APIs with namespaces rather
than browser hooks. But I don't think it's important to put all APIs in
c/b/e/api; it's enough that they live in a separate directory so that they can
be have appropriate DEPS.

[not at google - send to devlin, 2013-03-20 19:31:02]

> But I don't think it's important to put all APIs in
> c/b/e/api; it's enough that they live in a separate directory so that they can
> be have appropriate DEPS.

and I realised after making my comment that we do indeed allow for this, it's
what the implemented_in json property is for.

lgtm

[commit-bot: I haz the power, 2013-03-20 23:54:00]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-03-21 00:01:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/11724002/1...

[commit-bot: I haz the power, 2013-03-21 06:46:48]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-03-23 18:05:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/11724002/1...

[commit-bot: I haz the power, 2013-03-23 20:03:59]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-23 21:32:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/11724002/1...

[commit-bot: I haz the power, 2013-03-23 21:32:37]

Failed to apply patch for
chrome/browser/chromeos/accessibility/accessibility_util.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/accessibility/accessibility_util.cc
  Hunk #1 FAILED at 30.
  Hunk #2 succeeded at 197 with fuzz 1.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/accessibility/accessibility_util.cc.rej

Patch:       chrome/browser/chromeos/accessibility/accessibility_util.cc
Index: chrome/browser/chromeos/accessibility/accessibility_util.cc
diff --git a/chrome/browser/chromeos/accessibility/accessibility_util.cc
b/chrome/browser/chromeos/accessibility/accessibility_util.cc
index
e5a8476b68122ffd1ae4a857a2e58e92770232ce..767f168e10c443a81a19640191ce105a0650f09c
100644
--- a/chrome/browser/chromeos/accessibility/accessibility_util.cc
+++ b/chrome/browser/chromeos/accessibility/accessibility_util.cc
@@ -30,6 +30,7 @@
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_messages.h"
 #include "chrome/common/extensions/extension_resource.h"
+#include "chrome/common/extensions/manifest_handlers/content_scripts_handler.h"
 #include "chrome/common/extensions/user_script.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
@@ -197,8 +198,10 @@ void EnableSpokenFeedback(bool enabled,
           extension->id(), render_view_host->GetProcess()->GetID(),
           render_view_host->GetRoutingID());
 
-      for (size_t i = 0; i < extension->content_scripts().size(); i++) {
-        const extensions::UserScript& script = extension->content_scripts()[i];
+      const extensions::UserScriptList& content_scripts =
+          extensions::ContentScriptsInfo::GetContentScripts(extension);
+      for (size_t i = 0; i < content_scripts.size(); i++) {
+        const extensions::UserScript& script = content_scripts[i];
         for (size_t j = 0; j < script.js_scripts().size(); ++j) {
           const extensions::UserScript::File &file = script.js_scripts()[j];
           ExtensionResource resource = extension->GetResource(

[commit-bot: I haz the power, 2013-03-23 22:22:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/11724002/1...

[commit-bot: I haz the power, 2013-03-23 23:04:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/11724002/1...

[commit-bot: I haz the power, 2013-03-24 01:36:18]

Change committed as 190141