Expand usage of platform-apps flag and permission features.

Expand usage of platform-apps flag and permission features.

Reapplication of http://codereview.chromium.org/9837045/.

BUG=119758
TEST=added

TBR=asargent@chromium.org,jstritar@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128838

[commit-bot: I haz the power, 2012-03-25 20:21:33]

No reviewers yet.

[commit-bot: I haz the power, 2012-03-25 20:22:46]

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

[commit-bot: I haz the power, 2012-03-25 22:40:51]

Change committed as 128838

[jstritar, 2012-03-26 13:41:51]

LGTM

http://codereview.chromium.org/9839105/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_apitest.h (right):

http://codereview.chromium.org/9839105/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_apitest.h:182: void VerifyPermissions(const
FilePath& extension_path);
Do these VerifyPermissions tests really get us much? Just wondering if it needs
to be in every platform api test.

[miket_OOO, 2012-03-26 15:19:57]

http://codereview.chromium.org/9839105/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_apitest.h (right):

http://codereview.chromium.org/9839105/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_apitest.h:182: void VerifyPermissions(const
FilePath& extension_path);
On 2012/03/26 13:41:51, jstritar wrote:
> Do these VerifyPermissions tests really get us much? Just wondering if it
needs
> to be in every platform api test.

That's a fair point. It's a bit of a belt-and-suspenders approach, giving us
another opportunity to catch that someone forgot to declare the permissions if
they happen to copy/paste an existing apitest.cc to a new API. But it's a pretty
heavyweight way to do it. How about this multi-step approach instead:

1. I'll add a comment into each existing description of a platform-app API
reminding the author to take a look at the permissions model.

2. Then I'll refactor the usage of VerifyPermissions into a separate test that's
not associated with any particular API, so its purpose (making sure permissions
are enforced) is clear.

3. Finally, asargent and I will continue our quest to let authors declare
permissions in the IDL. Then most of this problem will go away, because anyone
writing a new API will have to choose which permissions are allowed.

[jstritar, 2012-03-27 15:16:09]

http://codereview.chromium.org/9839105/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_apitest.cc (right):

http://codereview.chromium.org/9839105/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_apitest.cc:316:
*CommandLine::ForCurrentProcess() = previous_command_line_;
Think this is okay to do? The documentation says that command line is mutable
but not thread-safe, and I haven't seen other usages like this.

http://codereview.chromium.org/9839105/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_apitest.cc:318:
ASSERT_TRUE(WaitForExtensionLoadError());
How about using extension_file_util::LoadExtension since we're having trouble
with WaitForExtensionLoadError? I'm not sure what the issue is with the load
errors on windows.

http://codereview.chromium.org/9839105/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_apitest.h (right):

http://codereview.chromium.org/9839105/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_apitest.h:182: void VerifyPermissions(const
FilePath& extension_path);
On 2012/03/26 15:19:57, miket wrote:
> On 2012/03/26 13:41:51, jstritar wrote:
> > Do these VerifyPermissions tests really get us much? Just wondering if it
> needs
> > to be in every platform api test.
> 
> That's a fair point. It's a bit of a belt-and-suspenders approach, giving us
> another opportunity to catch that someone forgot to declare the permissions if
> they happen to copy/paste an existing apitest.cc to a new API. But it's a
pretty
> heavyweight way to do it. How about this multi-step approach instead:
> 
> 1. I'll add a comment into each existing description of a platform-app API
> reminding the author to take a look at the permissions model.
> 
> 2. Then I'll refactor the usage of VerifyPermissions into a separate test
that's
> not associated with any particular API, so its purpose (making sure
permissions
> are enforced) is clear.
> 
> 3. Finally, asargent and I will continue our quest to let authors declare
> permissions in the IDL. Then most of this problem will go away, because anyone
> writing a new API will have to choose which permissions are allowed.

We could also override ExtensionBrowserTest::RunExtensionTestImpl to assert that
extension->is_platform_app()? (We'd have to change the signature or expose
another method so the PlatformAppApiTest has access to Extension*)

http://codereview.chromium.org/9839105/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/9839105/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_browsertest.cc:430: int before =
extension_load_errors_observed_;
Is there a specific reason you need to count the errors here?

[miket_OOO, 2012-03-28 16:53:41]

Jon, thanks for your comments. As discussed in person, I'm scaling back the
extent of this CL because I found a way to get rid of much of the browser test
stuff and replace it with simpler unit tests. One inline comment.

[miket_OOO, 2012-03-28 17:28:47]

http://codereview.chromium.org/9839105/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/9839105/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_browsertest.cc:430: int before =
extension_load_errors_observed_;
On 2012/03/27 15:16:09, jstritar wrote:
> Is there a specific reason you need to count the errors here?

I'm following the idiom established in the other WaitFor[...] methods, but it
appears to be so that we can confirm that the reason RegisterAndWait() exited
was that it received the expected notification (versus a timeout, for example).