Issue 581283002: Add webview testAPIMethodExistence to app_shell_browsertests.

Issue 581283002: Add webview testAPIMethodExistence to app_shell_browsertests. (Closed)

Created:
6 years, 3 months ago by lfg

Modified:
6 years, 3 months ago

Reviewers:
Fady Samuel, Yoyo Zhou

CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, Fady Samuel, Xi Han, wjmaclean

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Add webview testAPIMethodExistence to app_shell_browsertests. This CL adds an app_shell_browsertest to verify the existence of the webview API. BUG=352290 Committed: https://crrev.com/03cc87ba6cd9f2533fb8c8104a9af9b57ffdb79f Cr-Commit-Position: refs/heads/master@{#296194}

Patch Set 1 #

Total comments: 26

Patch Set 2 : #

Patch Set 3 : Properly merging #

Patch Set 4 : #

Total comments: 1

Patch Set 5 : #

Created: 6 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+256 lines, -6 lines)			Patch
A	extensions/browser/guest_view/web_view/web_view_apitest.cc	View	1 2 3 4	1 chunk	+151 lines, -0 lines	0 comments	Download
M	extensions/shell/app_shell.gyp	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
A +	extensions/test/data/web_view/apitest/main.html	View	1	1 chunk	+4 lines, -4 lines	0 comments	Download
A	extensions/test/data/web_view/apitest/main.js	View	1 2 3	1 chunk	+100 lines, -0 lines	0 comments	Download
A +	extensions/test/data/web_view/apitest/manifest.json	View	1	2 chunks	+1 line, -3 lines	0 comments	Download
A +	extensions/test/data/web_view/apitest/test.js	View	1	0 chunks	+-1 lines, --1 lines	0 comments	Download

Messages

Total messages: 12 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

lfg

yoz@: PTAL, this CL depends on https://codereview.chromium.org/584513003/.

6 years, 3 months ago (2014-09-18 23:14:35 UTC) #2

Fady Samuel

https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_view/web_view/web_view_browsertests.cc File extensions/browser/guest_view/web_view/web_view_browsertests.cc (right): https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_view/web_view/web_view_browsertests.cc#newcode129 extensions/browser/guest_view/web_view/web_view_browsertests.cc:129: content::WebContents* embedder_web_contents = Can we add a DCHECK above ...

6 years, 3 months ago (2014-09-18 23:22:15 UTC) #4

Yoyo Zhou

https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_view/web_view/web_view_browsertests.cc File extensions/browser/guest_view/web_view/web_view_browsertests.cc (right): https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_view/web_view/web_view_browsertests.cc#newcode1 extensions/browser/guest_view/web_view/web_view_browsertests.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. ...

6 years, 3 months ago (2014-09-19 00:26:04 UTC) #5

lfg

Please, take another look. https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_view/web_view/web_view_browsertests.cc File extensions/browser/guest_view/web_view/web_view_browsertests.cc (right): https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_view/web_view/web_view_browsertests.cc#newcode1 extensions/browser/guest_view/web_view/web_view_browsertests.cc:1: // Copyright 2014 The Chromium ...

6 years, 3 months ago (2014-09-22 17:44:28 UTC) #6

Please, take another look.

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

https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_browsertests.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2014/09/19 00:26:03, Yoyo Zhou wrote:
> Even if you have multiple tests, this file should be named
> web_view_browsertest.cc. But various platforms don't like files with the same
> basename. Can you come up with something better (non-plural)?

What about web_view_apitest.cc ?

https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_browsertests.cc:23: class
TestGuestViewManager : public extensions::GuestViewManager {
On 2014/09/19 00:26:03, Yoyo Zhou wrote:
> This looks very similar to the other web_view_browsertest.cc. Is there a plan
to
> avoid this sort of redundancy? Add some TODOs here and there if necessary.

They are the same. I've added a TODO to merge them together. The idea is that as
soon as app_shell_browsertests is running on all platforms we should be able to
migrate most of the tests out of chrome browsertests.

https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_browsertests.cc:95:
TestGuestViewManager* test_guest_view_manager_;
On 2014/09/19 00:26:04, Yoyo Zhou wrote:
> scoped_ptr? (How does this get deleted?)

Good question, I copied this code from the other file and didn't really think
about it.

So, after some investigation, this function gets called from
GuestViewManager::FromBrowserContext, where it gets assigned to a linked_ptr
that takes care of deleting it on destruction.

It doesn't look like this is safe to return the same pointer more than once, as
the ownership is transferred, so I've added some comments and changed the
function to assert if its called twice.

https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_browsertests.cc:107: void
TestHelper(const std::string& test_name,
On 2014/09/19 00:26:03, Yoyo Zhou wrote:
> This name isn't very descriptive. It could be better, like RunTest.

Done.

https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_browsertests.cc:109: std::cout
<< "Launching test " << test_name << " from " << app_location
On 2014/09/19 00:26:03, Yoyo Zhou wrote:
> remove debug comments or change them to VLOG/DVLOG

Forgot those. Removed.

https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_browsertests.cc:117:
ASSERT_TRUE(loaded);
On 2014/09/19 00:26:03, Yoyo Zhou wrote:
> inline with previous line

Done.

https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_browsertests.cc:126: LOG(ERROR)
<< "UNABLE TO FIND EMBEDDER WEB CONTENTS.";
On 2014/09/19 00:26:04, Yoyo Zhou wrote:
> All caps doesn't seem necessary.

I've changed it to a DCHECK, as per fsamuel@ suggestion.

https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_browsertests.cc:129:
content::WebContents* embedder_web_contents =
On 2014/09/18 23:22:14, Fady Samuel wrote:
> Can we add a DCHECK above to verify there is exactly one app window?

Done.

https://codereview.chromium.org/581283002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_browsertests.cc:151:
IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestAPIMethodExistence) {
On 2014/09/19 00:26:03, Yoyo Zhou wrote:
> Shim looks like it's in a strange place in this name. Why is it here?

I originally copied from the chrome browsertests. Removed it now.

https://codereview.chromium.org/581283002/diff/1/extensions/test/data/web_vie...
File extensions/test/data/web_view/shim/main.js (right):

https://codereview.chromium.org/581283002/diff/1/extensions/test/data/web_vie...
extensions/test/data/web_view/shim/main.js:7: window.runTest =
function(testName) {
On 2014/09/18 23:22:14, Fady Samuel wrote:
> Can we move these utility functions to a common file?

I've added a TODO for this.

https://codereview.chromium.org/581283002/diff/1/extensions/test/data/web_vie...
extensions/test/data/web_view/shim/main.js:62: 'terminate'
On 2014/09/18 23:22:14, Fady Samuel wrote:
> Can we add more methods to this?

Done.

https://codereview.chromium.org/581283002/diff/1/extensions/test/data/web_vie...
extensions/test/data/web_view/shim/main.js:75: typeof
webview.contentWindow.postMessage);
On 2014/09/18 23:22:14, Fady Samuel wrote:
> nit: alignment

Done.

https://codereview.chromium.org/581283002/diff/1/extensions/test/data/web_vie...
File extensions/test/data/web_view/shim/manifest.json (right):

https://codereview.chromium.org/581283002/diff/1/extensions/test/data/web_vie...
extensions/test/data/web_view/shim/manifest.json:3: "version": "2",
On 2014/09/19 00:26:04, Yoyo Zhou wrote:
> should also have "manifest_version": 2 (I'm surprised that it works without
it)

Done.

Yoyo Zhou

LGTM https://codereview.chromium.org/581283002/diff/60001/extensions/browser/guest_view/web_view/web_view_apitest.cc File extensions/browser/guest_view/web_view/web_view_apitest.cc (right): https://codereview.chromium.org/581283002/diff/60001/extensions/browser/guest_view/web_view/web_view_apitest.cc#newcode93 extensions/browser/guest_view/web_view/web_view_apitest.cc:93: DCHECK(test_guest_view_manager_ == NULL); nit: can just write DCHECK(!test_guest_view_manager);

6 years, 3 months ago (2014-09-22 23:43:57 UTC) #8

commit-bot: I haz the power

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

6 years, 3 months ago (2014-09-23 15:42:05 UTC) #10

commit-bot: I haz the power

Committed patchset #5 (id:80001) as 3815019376e8a83f14494e2fdcc4ee74ac8db1e1

6 years, 3 months ago (2014-09-23 15:49:51 UTC) #11

commit-bot: I haz the power

6 years, 3 months ago (2014-09-23 15:50:37 UTC) #12

Message was sent while issue was closed.

Patchset 5 (id:??) landed as
https://crrev.com/03cc87ba6cd9f2533fb8c8104a9af9b57ffdb79f
Cr-Commit-Position: refs/heads/master@{#296194}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages