Cleanup webview app_shell_browsertests and add webview media permission tests in app_shell_browsert…

Cleanup webview app_shell_browsertests and add webview media permission tests in app_shell_browsertests.

* Splits web_view_apitest into .h/.cc so it can be reused by other tests.
* Adds webview media permission tests in app_shell_browsertests:
  - TestAllow
  - TestAllowAndThenDeny
  - TestAllowAsync
  - TestAllowTwice
  - TestCheck
  - TestDeny
  - TestDenyThenAllowThrows
  - TestDenyWithPreventDefault
  - TestNoListenersImplyDeny
  - TestNoPreventDefaultImpliesDeny

BUG=352293
Committed: https://crrev.com/12dec866ef80260531951251b31a4b4886bf8be3
Cr-Commit-Position: refs/heads/master@{#298481}

[lfg, 2014-10-03 19:35:54]

lfg@chromium.org changed reviewers:
+ lazyboy@chromium.org

[lfg, 2014-10-03 19:35:55]

PTAL

[lazyboy, 2014-10-06 16:21:21]

lgtm

https://chromiumcodereview.appspot.com/626093002/diff/1/extensions/browser/gu...
File extensions/browser/guest_view/web_view/web_view_apitest.cc (right):

https://chromiumcodereview.appspot.com/626093002/diff/1/extensions/browser/gu...
extensions/browser/guest_view/web_view/web_view_apitest.cc:159:
WebViewAPITest::WebViewAPITest() {
Move constructor at the very beginning.

https://chromiumcodereview.appspot.com/626093002/diff/1/extensions/test/data/...
File extensions/test/data/web_view/media_access/allow/embedder.js (left):

https://chromiumcodereview.appspot.com/626093002/diff/1/extensions/test/data/...
extensions/test/data/web_view/media_access/allow/embedder.js:7:
embedder.baseGuestURL = '';
I'd keep these two as they make things readable, i.e. one can assume there would
be "baseGuestURL" and "guestURL" defined under the "embedder" object, you can
add a comment saying "These will be filled in chrome.test.getConfig() below".

[lfg, 2014-10-06 16:53:18]

https://codereview.chromium.org/626093002/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/web_view/web_view_apitest.cc (right):

https://codereview.chromium.org/626093002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_apitest.cc:159:
WebViewAPITest::WebViewAPITest() {
On 2014/10/06 16:21:20, lazyboy wrote:
> Move constructor at the very beginning.

Done.

https://codereview.chromium.org/626093002/diff/1/extensions/test/data/web_vie...
File extensions/test/data/web_view/media_access/allow/embedder.js (left):

https://codereview.chromium.org/626093002/diff/1/extensions/test/data/web_vie...
extensions/test/data/web_view/media_access/allow/embedder.js:7:
embedder.baseGuestURL = '';
On 2014/10/06 16:21:21, lazyboy wrote:
> I'd keep these two as they make things readable, i.e. one can assume there
would
> be "baseGuestURL" and "guestURL" defined under the "embedder" object, you can
> add a comment saying "These will be filled in chrome.test.getConfig() below".

Done.

[lfg, 2014-10-06 16:53:48]

lfg@chromium.org changed reviewers:
+ yoz@chromium.org

[lfg, 2014-10-06 16:53:49]

yoz@chromium.org: PTAL

[Yoyo Zhou, 2014-10-06 22:38:33]

LGTM

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_apitest.h (right):

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_apitest.h:16: class
WebViewAPITest : public AppShellTest {
class documentation?

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_apitest.h:20: void
LaunchApp(const std::string& app_location);
Document these?

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc
(right):

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc:28:
const content::MediaResponseCallback& callback) OVERRIDE {
nit: lowercase override

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc:79:
base::StringPrintf("runTest('%s')", test_name.c_str())));
nit, this seems like the embedded JS should have a semicolon.

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_media_access_apitest.h
(right):

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_media_access_apitest.h:9: class
WebViewMediaAccessAPITest : public WebViewAPITest {
nit: this doesn't have to be its own header file if it's not included elsewhere.
Up to you though.

[lfg, 2014-10-06 23:17:23]

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_apitest.h (right):

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_apitest.h:16: class
WebViewAPITest : public AppShellTest {
On 2014/10/06 22:38:33, Yoyo Zhou wrote:
> class documentation?

Done.

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_apitest.h:20: void
LaunchApp(const std::string& app_location);
On 2014/10/06 22:38:33, Yoyo Zhou wrote:
> Document these?

Done.

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc
(right):

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc:28:
const content::MediaResponseCallback& callback) OVERRIDE {
On 2014/10/06 22:38:33, Yoyo Zhou wrote:
> nit: lowercase override

Done.

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc:79:
base::StringPrintf("runTest('%s')", test_name.c_str())));
On 2014/10/06 22:38:33, Yoyo Zhou wrote:
> nit, this seems like the embedded JS should have a semicolon.

ExecuteScript already adds the semicolon, but since it's not obvious, I changed
it to have the semicolon.

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_media_access_apitest.h
(right):

https://codereview.chromium.org/626093002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_media_access_apitest.h:9: class
WebViewMediaAccessAPITest : public WebViewAPITest {
On 2014/10/06 22:38:33, Yoyo Zhou wrote:
> nit: this doesn't have to be its own header file if it's not included
elsewhere.
> Up to you though.

Done.

[lfg, 2014-10-07 15:03:37]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2014-10-07 15:05:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/626093002/40001

[commit-bot: I haz the power, 2014-10-07 15:10:07]

Committed patchset #3 (id:40001) as df0af118193c556abcbc56933d4763e9509e4ac5

[commit-bot: I haz the power, 2014-10-07 15:10:43]

Patchset 3 (id:??) landed as
https://crrev.com/12dec866ef80260531951251b31a4b4886bf8be3
Cr-Commit-Position: refs/heads/master@{#298481}