Implement chromium.self in content scripts, fix bugs

Implement chromium.self in content scripts, so that developers don't
have to know and copy/paste their extension ID.

This required moving the code that defaults the extension ID earlier
in the load process.

Also fixed some bugs:
* fixed a bug that was causing all user scripts to get executed in
  the same context.
* made the greasemonkey api only available in 'standalone' user
  scripts.
* re-added the anonymous function wrapper that is supposed to wrap
  content scripts.

Also added unit tests for the fixed bugs.

[Erik does not do reviews, 2009-04-06 17:25:16]

http://codereview.chromium.org/60112/diff/1001/1005
File chrome/browser/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/60112/diff/1001/1005#newcode41
Line 41: ASSERT_TRUE(extension.InitFromValue(*valid_value, true, &error));
we should add back in a test that requires an id, but it's missing

http://codereview.chromium.org/60112/diff/1001/1006
File chrome/browser/extensions/extension_user_script_inject_unittest.cc (right):

http://codereview.chromium.org/60112/diff/1001/1006#newcode16
Line 16: const char* kExtensionId = "00123456789abcdef0123456789abcdef0123456";
can't we just use the auto-generated id here?

http://codereview.chromium.org/60112/diff/1001/1006#newcode20
Line 20: class ExtensionUserScriptInjectTest : public InProcessBrowserTest {
Is the plan to keep the code references UserScript and to just refer to them
externally and in docs as ContentScript?  Or should we be changing our class
names too?

http://codereview.chromium.org/60112/diff/1001/1006#newcode32
Line 32: IN_PROC_BROWSER_TEST_F(ExtensionUserScriptInjectTest, Simple) {
This is a great start.  Could you add some TODOs for future tests to extend
here:
- early injection vs. late injection
- css injection

http://codereview.chromium.org/60112/diff/1001/1008
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/60112/diff/1001/1008#newcode633
Line 633: if (!extension.InitFromValue(*dict, false, &error)) {
Since this is install / update, don't we need to require an id?  Also, the bool
should be named / commented.

http://codereview.chromium.org/60112/diff/1001/1010
File chrome/browser/extensions/test_extension_loader.cc (right):

http://codereview.chromium.org/60112/diff/1001/1010#newcode52
Line 52: ExtensionList* extensions = Details<ExtensionList>(details).ptr();
since we only ever load one extension at a time, maybe we should DCHECK size()
== 1 and ditch the loop

http://codereview.chromium.org/60112/diff/1001/1010#newcode53
Line 53: for (size_t i = 0; i < (*extensions).size(); i++) {
style nit: ++i

http://codereview.chromium.org/60112/diff/1001/1011
File chrome/browser/extensions/test_extension_loader.h (right):

http://codereview.chromium.org/60112/diff/1001/1011#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
just 2009 is right for new files (other files affected as well)

http://codereview.chromium.org/60112/diff/1001/1011#newcode20
Line 20: Extension* Load(const char* extension_id, const FilePath& path);
it seems like it would be useful to have a version that used id generation
rather than a passed in id

http://codereview.chromium.org/60112/diff/1001/1015
File chrome/test/data/extensions/user_script_inject/js_test.js (right):

http://codereview.chromium.org/60112/diff/1001/1015#newcode1
Line 1: // A quick and dirty JavaScript test runner.
need a copyright header?

http://codereview.chromium.org/60112/diff/1001/1016
File chrome/test/data/extensions/user_script_inject/manifest.json (right):

http://codereview.chromium.org/60112/diff/1001/1016#newcode2
Line 2: "format_version": 1,
didn't you check in the change so we don't need this?

http://codereview.chromium.org/60112/diff/1001/1016#newcode6
Line 6: "description": "User script inejct",
typo inejct -> inject

http://codereview.chromium.org/60112/diff/1001/1017
File chrome/test/data/extensions/user_script_inject/script1a.js (right):

http://codereview.chromium.org/60112/diff/1001/1017#newcode2
Line 2: // script2.js tests to make sure it is not.
script2b.js

http://codereview.chromium.org/60112/diff/1001/1018
File chrome/test/data/extensions/user_script_inject/script1b.js (right):

http://codereview.chromium.org/60112/diff/1001/1018#newcode8
Line 8: assert(typeof contentWindow != "x");
sorry if I'm being dense, but I don't get this line.  why would it ever be "x"?

http://codereview.chromium.org/60112/diff/1001/1019
File chrome/test/data/extensions/user_script_inject/script2a.js (right):

http://codereview.chromium.org/60112/diff/1001/1019#newcode1
Line 1: var script2_var = "foobar";
this file appears to be unused

http://codereview.chromium.org/60112/diff/1001/1020
File chrome/test/data/extensions/user_script_inject/script2b.js (right):

http://codereview.chromium.org/60112/diff/1001/1020#newcode1
Line 1: function testCannotSeeOtherContentScriptGlobals() {
add a comment explaining this test like you did with the others

http://codereview.chromium.org/60112/diff/1001/1022
File chrome/test/unit/unittests.vcproj (right):

http://codereview.chromium.org/60112/diff/1001/1022#newcode483
Line 483:
RelativePath="..\..\browser\extensions\extension_user_script_inject_unittest.cc"
need scons/gyp changes for these as well

[Aaron Boodman, 2009-04-06 20:51:01]

Ready for another look.

http://codereview.chromium.org/60112/diff/1001/1005
File chrome/browser/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/60112/diff/1001/1005#newcode41
Line 41: ASSERT_TRUE(extension.InitFromValue(*valid_value, true, &error));
On 2009/04/06 17:25:16, Erik Kay wrote:
> we should add back in a test that requires an id, but it's missing

Good point, done.

http://codereview.chromium.org/60112/diff/1001/1006
File chrome/browser/extensions/extension_user_script_inject_unittest.cc (right):

http://codereview.chromium.org/60112/diff/1001/1006#newcode16
Line 16: const char* kExtensionId = "00123456789abcdef0123456789abcdef0123456";
On 2009/04/06 17:25:16, Erik Kay wrote:
> can't we just use the auto-generated id here?

See my later comment about changing ExtensionLoader to use auto-generated IDs.

http://codereview.chromium.org/60112/diff/1001/1006#newcode20
Line 20: class ExtensionUserScriptInjectTest : public InProcessBrowserTest {
On 2009/04/06 17:25:16, Erik Kay wrote:
> Is the plan to keep the code references UserScript and to just refer to them
> externally and in docs as ContentScript?  Or should we be changing our class
> names too?

The current state is that we have two concepts "user scripts" and "content
scripts". Content scripts happen to be implemented using the user script system.

I admit this whole business is slightly confusing, but I always prefer to stick
with the slightly weird state until there is a better plan. That way at least
things are consistently weird.

This class technically tests _content script_ injection, though, so I have
renamed in.

http://codereview.chromium.org/60112/diff/1001/1006#newcode32
Line 32: IN_PROC_BROWSER_TEST_F(ExtensionUserScriptInjectTest, Simple) {
On 2009/04/06 17:25:16, Erik Kay wrote:
> This is a great start.  Could you add some TODOs for future tests to extend
> here:
> - early injection vs. late injection
> - css injection

I just went ahead and added them.

http://codereview.chromium.org/60112/diff/1001/1008
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/60112/diff/1001/1008#newcode633
Line 633: if (!extension.InitFromValue(*dict, false, &error)) {
On 2009/04/06 17:25:16, Erik Kay wrote:
> Since this is install / update, don't we need to require an id?  Also, the
bool
> should be named / commented.

Yes good catch.

http://codereview.chromium.org/60112/diff/1001/1010
File chrome/browser/extensions/test_extension_loader.cc (right):

http://codereview.chromium.org/60112/diff/1001/1010#newcode52
Line 52: ExtensionList* extensions = Details<ExtensionList>(details).ptr();
On 2009/04/06 17:25:16, Erik Kay wrote:
> since we only ever load one extension at a time, maybe we should DCHECK size()
> == 1 and ditch the loop

See point about ditching IDs.

http://codereview.chromium.org/60112/diff/1001/1010#newcode53
Line 53: for (size_t i = 0; i < (*extensions).size(); i++) {
On 2009/04/06 17:25:16, Erik Kay wrote:
> style nit: ++i

n/a.

http://codereview.chromium.org/60112/diff/1001/1011
File chrome/browser/extensions/test_extension_loader.h (right):

http://codereview.chromium.org/60112/diff/1001/1011#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/04/06 17:25:16, Erik Kay wrote:
> just 2009 is right for new files (other files affected as well)

*aa does his best dance for the cargo gods.

Fixed.

http://codereview.chromium.org/60112/diff/1001/1011#newcode20
Line 20: Extension* Load(const char* extension_id, const FilePath& path);
On 2009/04/06 17:25:16, Erik Kay wrote:
> it seems like it would be useful to have a version that used id generation
> rather than a passed in id

So I looked into doing this and it isn't possible since this class also receives
the observer notifications as the extension system comes up normally. I tried
moving things around so that we wouldn't receive the normal startup
notifications but it is difficult since the work all happens asynchronously. I
couldn't think of a way to improve things without bigger refactoring.

http://codereview.chromium.org/60112/diff/1001/1015
File chrome/test/data/extensions/user_script_inject/js_test.js (right):

http://codereview.chromium.org/60112/diff/1001/1015#newcode1
Line 1: // A quick and dirty JavaScript test runner.
On 2009/04/06 17:25:16, Erik Kay wrote:
> need a copyright header?

Done.

http://codereview.chromium.org/60112/diff/1001/1016
File chrome/test/data/extensions/user_script_inject/manifest.json (right):

http://codereview.chromium.org/60112/diff/1001/1016#newcode2
Line 2: "format_version": 1,
On 2009/04/06 17:25:16, Erik Kay wrote:
> didn't you check in the change so we don't need this?

Done.

http://codereview.chromium.org/60112/diff/1001/1016#newcode6
Line 6: "description": "User script inejct",
On 2009/04/06 17:25:16, Erik Kay wrote:
> typo inejct -> inject

Done.

http://codereview.chromium.org/60112/diff/1001/1017
File chrome/test/data/extensions/user_script_inject/script1a.js (right):

http://codereview.chromium.org/60112/diff/1001/1017#newcode2
Line 2: // script2.js tests to make sure it is not.
On 2009/04/06 17:25:16, Erik Kay wrote:
> script2b.js

Whoops the file actually should be called script2.js, since there is only one of
those. I changed that instead.

http://codereview.chromium.org/60112/diff/1001/1018
File chrome/test/data/extensions/user_script_inject/script1b.js (right):

http://codereview.chromium.org/60112/diff/1001/1018#newcode8
Line 8: assert(typeof contentWindow != "x");
On 2009/04/06 17:25:16, Erik Kay wrote:
> sorry if I'm being dense, but I don't get this line.  why would it ever be
"x"?

Heh, it wouldn't. This was left over from me trying to figure out that weird
null-termination bug.

http://codereview.chromium.org/60112/diff/1001/1019
File chrome/test/data/extensions/user_script_inject/script2a.js (right):

http://codereview.chromium.org/60112/diff/1001/1019#newcode1
Line 1: var script2_var = "foobar";
On 2009/04/06 17:25:16, Erik Kay wrote:
> this file appears to be unused

Yup, removed.

http://codereview.chromium.org/60112/diff/1001/1022
File chrome/test/unit/unittests.vcproj (right):

http://codereview.chromium.org/60112/diff/1001/1022#newcode483
Line 483:
RelativePath="..\..\browser\extensions\extension_user_script_inject_unittest.cc"
On 2009/04/06 17:25:16, Erik Kay wrote:
> need scons/gyp changes for these as well

Done.

[Erik does not do reviews, 2009-04-06 21:12:06]

LGTM

http://codereview.chromium.org/60112/diff/3001/4022
File chrome/test/data/extensions/content_script_inject/script3.js (right):

http://codereview.chromium.org/60112/diff/3001/4022#newcode11
Line 11: // This tests that early injection is working properly and that wea re
getting
typo: wea re -> we are

http://codereview.chromium.org/60112/diff/3001/4022#newcode12
Line 12: // run after the documentElement is available, but before anything else
happens.
this doesn't run at the proper time