MacViews: Split ViewEventTestBase into platform-specific files.

MacViews: Split ViewEventTestBase into platform-specific files.

I couldn't bring myself to add another #ifdef to
view_event_test_base.cc, so this splits out the platform-specific stuff
and paves the way to get interactive_ui_tests compiling on MacViews.

This is done by adding a ViewEventTestPlatFormPart interface, which is
implemented for (views platforms): chromeos, ash, and mac.

There was also code for a non-ash aura platform, but all build
configurations that would have used it are broken, so it's just rot and
is removed.

BUG=399191
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288966

[tapted, 2014-08-05 14:00:26]

Hi sky, another orthogonal chunk from the big picture in crrev/433403002 . PTAL
when you get a chance.

https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
File chrome/test/base/chrome_views_test_helper_aura.cc (right):

https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
chrome/test/base/chrome_views_test_helper_aura.cc:15: class
ChromeViewsTestHelperAura : public ChromeViewsTestHelper {
This is the black sheep of the family.. I don't think there's bot coverage for
this. I checked this file compiles on linux, but chrome itself doesn't compile
with `aura && !ash`, so I'm not sure we want to keep this file at all.

[sky, 2014-08-05 21:54:35]

I'll take a look at this in more detail tomorrow. Do to the move I
don't have my machine up and running, which makes looking at this
difficult.

On Tue, Aug 5, 2014 at 7:00 AM,  <tapted@chromium.org> wrote:
> Reviewers: sky,
>
> Message:
> Hi sky, another orthogonal chunk from the big picture in crrev/433403002 .
> PTAL
> when you get a chance.
>
>
>
https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
> File chrome/test/base/chrome_views_test_helper_aura.cc (right):
>
>
https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
> chrome/test/base/chrome_views_test_helper_aura.cc:15: class
> ChromeViewsTestHelperAura : public ChromeViewsTestHelper {
> This is the black sheep of the family.. I don't think there's bot
> coverage for this. I checked this file compiles on linux, but chrome
> itself doesn't compile with `aura && !ash`, so I'm not sure we want to
> keep this file at all.
>
> Description:
> MacViews: Split ViewEventTestBase into platform-specific files.
>
> I couldn't bring myself to add another #ifdef to
> view_event_test_base.cc, so this splits out the platform-specific stuff
> and paves the way to get interactive_ui_tests compiling on MacViews.
>
> BUG=399191
>
> Please review this at https://codereview.chromium.org/443553003/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+278, -92 lines):
>   M chrome/chrome_tests.gypi
>   A chrome/test/base/chrome_views_test_helper.h
>   A chrome/test/base/chrome_views_test_helper_ash.cc
>   A chrome/test/base/chrome_views_test_helper_aura.cc
>   A chrome/test/base/chrome_views_test_helper_chromeos.cc
>   A chrome/test/base/chrome_views_test_helper_mac.mm
>   M chrome/test/base/view_event_test_base.h
>   M chrome/test/base/view_event_test_base.cc
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-08-06 17:37:44]

https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
File chrome/test/base/chrome_views_test_helper.h (right):

https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
chrome/test/base/chrome_views_test_helper.h:18: class ChromeViewsTestHelper {
Can we name this better to indicate what it is for. Maybe something like
ViewsTestHelperInit or ViewsTestHelperPlatformInit?

https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
chrome/test/base/chrome_views_test_helper.h:27: virtual gfx::NativeWindow
PlatformSetUp(
Do we really need the two virtual methods? Can't Create() take the parameters
you have in PlatformSetUp() and the destructor do the tear down?

https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
File chrome/test/base/chrome_views_test_helper_aura.cc (right):

https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
chrome/test/base/chrome_views_test_helper_aura.cc:15: class
ChromeViewsTestHelperAura : public ChromeViewsTestHelper {
On 2014/08/05 14:00:26, tapted wrote:
> This is the black sheep of the family.. I don't think there's bot coverage for
> this. I checked this file compiles on linux, but chrome itself doesn't compile
> with `aura && !ash`, so I'm not sure we want to keep this file at all.

Maybe check the ozone build? If that doesn't use it, then go ahead and nuke the
file. No sense in checking a file that won't be built and is just going to get
stale.

[tapted, 2014-08-11 12:04:17]

Thanks sky! PTAL

(and sorry for the latency - was OOO with sketchy internet)

https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
File chrome/test/base/chrome_views_test_helper.h (right):

https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
chrome/test/base/chrome_views_test_helper.h:18: class ChromeViewsTestHelper {
On 2014/08/06 17:37:44, sky wrote:
> Can we name this better to indicate what it is for. Maybe something like
> ViewsTestHelperInit or ViewsTestHelperPlatformInit?

I ended up with `ViewEventTestPlatFormPart*` - let me know what you think. I
think it's a good fit with the pattern in BrowserProcessPlatformPart.  (I
started using ..Init but it was feeling too much like a function name).

(in case it's useful, File moves are in patchset 4, so you can see most of the
diff for these files in a patchset 2:3 diff).

https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
chrome/test/base/chrome_views_test_helper.h:27: virtual gfx::NativeWindow
PlatformSetUp(
On 2014/08/06 17:37:44, sky wrote:
> Do we really need the two virtual methods? Can't Create() take the parameters
> you have in PlatformSetUp() and the destructor do the tear down?

Done (mostly). There was code in view_event_test_base.cc between teardown and
the destructor before, but tests seem happy with the steps being merged (I was
initially worried they wouldn't be).

I still needed a `GetContext` function. I've left it as a virtual member
function, but a static function on the PlatformPart base class could also work.

https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
File chrome/test/base/chrome_views_test_helper_aura.cc (right):

https://codereview.chromium.org/443553003/diff/1/chrome/test/base/chrome_view...
chrome/test/base/chrome_views_test_helper_aura.cc:15: class
ChromeViewsTestHelperAura : public ChromeViewsTestHelper {
On 2014/08/06 17:37:44, sky wrote:
> On 2014/08/05 14:00:26, tapted wrote:
> > This is the black sheep of the family.. I don't think there's bot coverage
for
> > this. I checked this file compiles on linux, but chrome itself doesn't
compile
> > with `aura && !ash`, so I'm not sure we want to keep this file at all.
> 
> Maybe check the ozone build? If that doesn't use it, then go ahead and nuke
the
> file. No sense in checking a file that won't be built and is just going to get
> stale.

Nuked. I think the ozone bot we most care about (linux_ecs_ozone) would be more
correctly be called embedded/chromecast bots -- they are aura, but not
toolkit-views, so none of this is hit (view_event_test_base.cc gyp-def'd out).

There are some freshly appearing chromeos_ozone bots which I tried out, but
interactive_ui_tests has never passed there (and they'll get the chromeos
version anyway).

[sky, 2014-08-11 16:14:34]

LGTM

[tapted, 2014-08-12 00:54:29]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-08-12 01:04:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tapted@chromium.org/443553003/120001

[commit-bot: I haz the power, 2014-08-12 02:01:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-12 14:50:16]

Change committed as 288966