Issue 1062163004: Plugins: Move Prerender tests to PPAPI and some refactoring.

Issue 1062163004: Plugins: Move Prerender tests to PPAPI and some refactoring. (Closed)

Created:
5 years, 8 months ago by tommycli

Modified:
5 years, 8 months ago

Reviewers:
Lei Zhang, raymes, Paweł Hajdan Jr.

CC:
cbentzel+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, davidben+watch_chromium.org, gavinp+prer_chromium.org, jam, tburkard+watch_chromium.org

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

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Plugins: Move Prerender tests to PPAPI and some refactoring. BUG=475688, 103496, 100514, 306715 Committed: https://crrev.com/6f53f63dba3428a2dd166018ebed294d10105403 Cr-Commit-Position: refs/heads/master@{#325746}

Patch Set 8 : simplify and fix #

Patch Set 12 : remove stale export #

Patch Set 16 : make move clear #

Patch Set 20 : #

Created: 5 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+103 lines, -225 lines)			Patch
M	chrome/browser/apps/guest_view/web_view_browsertest.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	3 chunks	+2 lines, -25 lines	0 comments	Download
M	chrome/browser/prerender/prerender_browsertest.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	4 chunks	+20 lines, -41 lines	0 comments	Download
M	chrome/test/data/prerender/plugin_delay_load.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13	2 chunks	+7 lines, -4 lines	0 comments	Download
A +	chrome/test/data/prerender/plugin_never_load.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	2 chunks	+10 lines, -6 lines	0 comments	Download
D	chrome/test/data/prerender/prerender_plugin_power_saver.html	View		1 chunk	+0 lines, -29 lines	0 comments	Download
M	chrome/test/ppapi/ppapi_browsertest.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/test/ppapi/ppapi_test.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	2 chunks	+7 lines, -17 lines	0 comments	Download
M	content/content_tests.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13	2 chunks	+3 lines, -0 lines	0 comments	Download
A +	content/public/test/ppapi_test_utils.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	1 chunk	+18 lines, -12 lines	0 comments	Download
A +	content/public/test/ppapi_test_utils.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	1 chunk	+32 lines, -11 lines	0 comments	Download
M	content/test/ppapi/ppapi_browsertest.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/ppapi/ppapi_test.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	2 chunks	+2 lines, -14 lines	0 comments	Download
M	ppapi/ppapi_shared.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+0 lines, -2 lines	0 comments	Download
M	ppapi/shared_impl/BUILD.gn	View	1	1 chunk	+0 lines, -2 lines	0 comments	Download
D	ppapi/shared_impl/test_harness_utils.h	View		1 chunk	+0 lines, -29 lines	0 comments	Download
D	ppapi/shared_impl/test_harness_utils.cc	View		1 chunk	+0 lines, -31 lines	0 comments	Download

Messages

Total messages: 39 (14 generated)

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

tommycli

dmichael: PTAL. How does this look for fixing the Prerender tests?

5 years, 8 months ago (2015-04-10 00:19:03 UTC) #2

dmichael (off chromium)

FYI I'm moving off of Chromium... punting this over to teravest@

5 years, 8 months ago (2015-04-10 16:53:12 UTC) #6

teravest

On 2015/04/10 16:53:12, dmichael (off chromium) wrote: > FYI I'm moving off of Chromium... punting ...

5 years, 8 months ago (2015-04-10 17:01:31 UTC) #7

tommycli

On 2015/04/10 17:01:31, teravest wrote: > On 2015/04/10 16:53:12, dmichael (off chromium) wrote: > > ...

5 years, 8 months ago (2015-04-10 18:25:15 UTC) #8

raymes

https://codereview.chromium.org/1062163004/diff/230001/chrome/test/data/prerender/plugin_delay_load.html File chrome/test/data/prerender/plugin_delay_load.html (right): https://codereview.chromium.org/1062163004/diff/230001/chrome/test/data/prerender/plugin_delay_load.html#newcode7 chrome/test/data/prerender/plugin_delay_load.html:7: a the postMessage() function on the plugin object. nit: ...

5 years, 8 months ago (2015-04-15 06:44:18 UTC) #13

tommycli

raymes: addressed your comments below jcivelli: review content/public/test/OWNERS file please Thanks! https://codereview.chromium.org/1062163004/diff/230001/chrome/test/data/prerender/plugin_delay_load.html File chrome/test/data/prerender/plugin_delay_load.html (right): ...

5 years, 8 months ago (2015-04-15 18:23:19 UTC) #15

raymes

lgtm https://codereview.chromium.org/1062163004/diff/230001/content/public/test/OWNERS File content/public/test/OWNERS (right): https://codereview.chromium.org/1062163004/diff/230001/content/public/test/OWNERS#newcode10 content/public/test/OWNERS:10: per-file ppapi*=teravest@chromium.org I'll still be helping in the ...

5 years, 8 months ago (2015-04-16 01:16:31 UTC) #16

tommycli

raymes: done phajdan.jr: PTAL content/public/test/OWNERS, thanks! https://codereview.chromium.org/1062163004/diff/230001/content/public/test/OWNERS File content/public/test/OWNERS (right): https://codereview.chromium.org/1062163004/diff/230001/content/public/test/OWNERS#newcode10 content/public/test/OWNERS:10: per-file ppapi*=teravest@chromium.org On ...

5 years, 8 months ago (2015-04-16 17:45:45 UTC) #18

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1062163004/270001

5 years, 8 months ago (2015-04-16 17:46:29 UTC) #21

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

5 years, 8 months ago (2015-04-16 20:00:25 UTC) #23

Paweł Hajdan Jr.

https://codereview.chromium.org/1062163004/diff/270001/content/public/test/OWNERS File content/public/test/OWNERS (right): https://codereview.chromium.org/1062163004/diff/270001/content/public/test/OWNERS#newcode8 content/public/test/OWNERS:8: per-file ppapi*=bbudge@chromium.org Why are these entries needed? https://codereview.chromium.org/1062163004/diff/270001/content/public/test/ppapi_test_utils.cc File ...

5 years, 8 months ago (2015-04-16 21:36:18 UTC) #24

tommycli

phajdan.jr: Was able to use your suggestions to move the file back to its original ...

5 years, 8 months ago (2015-04-16 22:18:51 UTC) #25

tommycli

phajdan.jr: Actually I made a mistake somewhere. Please hold off on taking another look until ...

5 years, 8 months ago (2015-04-16 22:25:24 UTC) #26

tommycli

phajdan.jr: Okay this is ready for your review now. As it turns out I could ...

5 years, 8 months ago (2015-04-16 23:35:43 UTC) #30

Paweł Hajdan Jr.

5 years, 8 months ago (2015-04-17 01:38:01 UTC) #31

https://codereview.chromium.org/1062163004/diff/270001/content/public/test/OW...
File content/public/test/OWNERS (right):

https://codereview.chromium.org/1062163004/diff/270001/content/public/test/OW...
content/public/test/OWNERS:8: per-file ppapi*=bbudge@chromium.org
On 2015/04/16 23:35:42, tommycli wrote:
> On 2015/04/16 22:18:51, tommycli wrote:
> > On 2015/04/16 21:36:18, Paweł Hajdan Jr. wrote:
> > > Why are these entries needed?
> > 
> > Done. I realized i could move the whole thing back to ppapi, so these
entries
> > are gone now.
> 
> Actually, I wasn't able to move it back. I added these entries because the
> original file had ppapi owners too. content/test/ppapi also has ppapi owners.

Please look at the code that's being moved - do you have convincing arguments
for these ~50 lines having specific OWNERS?

https://codereview.chromium.org/1062163004/diff/270001/content/public/test/pp...
File content/public/test/ppapi_test_utils.cc (right):

https://codereview.chromium.org/1062163004/diff/270001/content/public/test/pp...
content/public/test/ppapi_test_utils.cc:18: "FAILS_", "FLAKY_", "DISABLED_",
"SLOW_"};
On 2015/04/16 23:35:42, tommycli wrote:
> On 2015/04/16 22:18:51, tommycli wrote:
> > On 2015/04/16 21:36:18, Paweł Hajdan Jr. wrote:
> > > Why is this needed? Only DISABLED_ is a valid prefix these days.
> > > 
> > > Either way, it doesn't look ppapi-specific to me.
> > 
> > Done. In the latest patch, these lines are untouched.
> > 
> > I think it's only called within src/chrome/test/ppapi/ppapi_browsertest.cc
and
> > src/content/test/ppapi/ppapi_browsertest.cc.
> 
> This is the same as the file I moved it from.

Even if it's just a move, I'd like to keep content/public/test quality really
high.

There is just no such thing as FAILS_, FLAKY_, or SLOW_.

Doing any post-processing for DISABLED_ tests is suspicious but I can't stop
you. I just don't think any such logic belongs in generic test utilities.

tommycli

phajdan.jr: addressed your comments below. Thanks! https://codereview.chromium.org/1062163004/diff/270001/content/public/test/OWNERS File content/public/test/OWNERS (right): https://codereview.chromium.org/1062163004/diff/270001/content/public/test/OWNERS#newcode8 content/public/test/OWNERS:8: per-file ppapi*=bbudge@chromium.org On ...

5 years, 8 months ago (2015-04-17 20:02:14 UTC) #32

phajdan.jr: addressed your comments below. Thanks!

https://codereview.chromium.org/1062163004/diff/270001/content/public/test/OW...
File content/public/test/OWNERS (right):

https://codereview.chromium.org/1062163004/diff/270001/content/public/test/OW...
content/public/test/OWNERS:8: per-file ppapi*=bbudge@chromium.org
On 2015/04/17 01:38:01, Paweł Hajdan Jr. wrote:
> On 2015/04/16 23:35:42, tommycli wrote:
> > On 2015/04/16 22:18:51, tommycli wrote:
> > > On 2015/04/16 21:36:18, Paweł Hajdan Jr. wrote:
> > > > Why are these entries needed?
> > > 
> > > Done. I realized i could move the whole thing back to ppapi, so these
> entries
> > > are gone now.
> > 
> > Actually, I wasn't able to move it back. I added these entries because the
> > original file had ppapi owners too. content/test/ppapi also has ppapi
owners.
> 
> Please look at the code that's being moved - do you have convincing arguments
> for these ~50 lines having specific OWNERS?

Done. I didn't realize you were recommending the removal of these lines.

https://codereview.chromium.org/1062163004/diff/270001/content/public/test/pp...
File content/public/test/ppapi_test_utils.cc (right):

https://codereview.chromium.org/1062163004/diff/270001/content/public/test/pp...
content/public/test/ppapi_test_utils.cc:18: "FAILS_", "FLAKY_", "DISABLED_",
"SLOW_"};
On 2015/04/17 01:38:01, Paweł Hajdan Jr. wrote:
> On 2015/04/16 23:35:42, tommycli wrote:
> > On 2015/04/16 22:18:51, tommycli wrote:
> > > On 2015/04/16 21:36:18, Paweł Hajdan Jr. wrote:
> > > > Why is this needed? Only DISABLED_ is a valid prefix these days.
> > > > 
> > > > Either way, it doesn't look ppapi-specific to me.
> > > 
> > > Done. In the latest patch, these lines are untouched.
> > > 
> > > I think it's only called within src/chrome/test/ppapi/ppapi_browsertest.cc
> and
> > > src/content/test/ppapi/ppapi_browsertest.cc.
> > 
> > This is the same as the file I moved it from.
> 
> Even if it's just a move, I'd like to keep content/public/test quality really
> high.
> 
> There is just no such thing as FAILS_, FLAKY_, or SLOW_.
> 
> Doing any post-processing for DISABLED_ tests is suspicious but I can't stop
> you. I just don't think any such logic belongs in generic test utilities.

Done.

Paweł Hajdan Jr.

LGTM I'd still prefer StripTestPrefixes to be moved out of content/public/test, but I'm fine either ...

5 years, 8 months ago (2015-04-17 22:37:52 UTC) #33

tommycli

On 2015/04/17 22:37:52, Paweł Hajdan Jr. wrote: > LGTM > > I'd still prefer StripTestPrefixes ...

5 years, 8 months ago (2015-04-17 23:09:04 UTC) #34

commit-bot: I haz the power

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

5 years, 8 months ago (2015-04-17 23:14:26 UTC) #37

commit-bot: I haz the power

5 years, 8 months ago (2015-04-18 00:34:39 UTC) #39

Message was sent while issue was closed.

Patchset 20 (id:??) landed as
https://crrev.com/6f53f63dba3428a2dd166018ebed294d10105403
Cr-Commit-Position: refs/heads/master@{#325746}

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