mac_views_browser: fixed interactive_ui_tests compilation.

mac_views_browser: fixed interactive_ui_tests compilation.

Splitting interactive_test_utils_mac.mm to
mac specific part - interactive_test_utils_mac.mm
and cocoa-views part - interactive_test_utils_cocoa.mm.

Splitting interactive_test_utils_views.cc to
interactive_test_utils_common_views.cc that can be used in mac tests
and interactive_test_utils_views.cc that used only in toolkit-views tests.

Disabled 3 tests files that are not ready for mac_views.

BUG=575036

[kirr, 2016-02-04 13:08:20]

kirr@yandex-team.ru changed reviewers:
+ jackhou@chromium.org, tapted@chromium.org

[tapted, 2016-02-05 05:11:11]

Thanks! This looks pretty good.

I just landed a fix for browser_tests in
https://codereview.chromium.org/1632323003/, but hadn't yet done much with
interactive_ui_tests. elly (+cc) had a different approach in
http://crrev.com/1647563002 too, but we agreed to try splitting files rather
than introducing a ~#define.

(also I'll drop jack from reviewers, since he no longer works on chrome).

https://codereview.chromium.org/1669793002/diff/1/chrome/browser/ui/views/loc...
File
chrome/browser/ui/views/location_bar/page_action_image_view_interactive_uitest.cc
(right):

https://codereview.chromium.org/1669793002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/page_action_image_view_interactive_uitest.cc:16:
#include "chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.h"
can this be removed now that interactive_test_utils.h provides it instead?

https://codereview.chromium.org/1669793002/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/chrome_browser_ui.gy...
chrome/chrome_browser_ui.gypi:2113:
'browser/ui/views/translate/translate_bubble_view.cc',
if these aren't part of the ChromeMac release (e.g. behind
chrome://flags/#mac-views-dialogs we shouldn't move them into
chrome_browser_ui_views_sources yet, since it might be adding unnecessary bloat
or carry other risks)

Can we exclude the corresponding test files instead? (or maybe we just need to
skip this bit of the delta for now)

https://codereview.chromium.org/1669793002/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1119: # Mac sources, except when mac_views_browser==1
nit: full stop after comment

https://codereview.chromium.org/1669793002/diff/1/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1719: 'conditions' : [
I've been putting a comment here, like

# The browser window can be views or Cocoa on Mac. Test accordingly.

Then this needs a corresponding BUILD.gn change in case we forget about it and
accidentally lose some test coverage

https://codereview.chromium.org/1669793002/diff/1/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1725:
'browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm',
Can this just move into chrome_interactive_ui_test_cocoa_sources?

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
File chrome/test/base/interactive_test_utils.h (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils.h:135: // Returns the center of |view|
in screen coordinates.
nit: blank line before

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
File chrome/test/base/interactive_test_utils_cocoa.mm (left):

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_cocoa.mm:11: #include "base/logging.h"
nit: keep this -- there are still DCHECKs below and the files "own" header
(interactive_test_utils.h) isn't providing it

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
File chrome/test/base/interactive_test_utils_cocoa.mm (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_cocoa.mm:7: #include <Carbon/Carbon.h>
nit: I think this was only used for TransformProcessType, so can probably be
removed too

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_cocoa.mm:96: 
nit: remove blank line

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
File chrome/test/base/interactive_test_utils_common_views.cc (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_common_views.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
nit: (c) 2012 -> 2016

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_common_views.cc:4: 
perhaps a comment in here about this file's purpose. E.g.

// Methods compiled on all toolkit-views platforms (including Mac).

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_common_views.cc:7: #include
"ui/base/test/ui_controls.h"
nit: I think this can be removed since interactive_test_utils.h provides it

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
File chrome/test/base/interactive_test_utils_views.cc (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_views.cc:10: #include
"ui/base/test/ui_controls.h"
nit: remove

[tapted, 2016-02-05 05:11:27]

Description was changed from

==========
mac_views_browser: fixed interactive_ui_tests compilation.

Splitting interactive_test_utils_mac.mm to
mac specific part - interactive_test_utils_mac.mm
and cocoa-views part - interactive_test_utils_cocoa.mm.

Splitting interactive_test_utils_views.cc to
interactive_test_utils_common_views.cc that can be used in mac tests
and interactive_test_utils_views.cc that used only in toolkit-views tests.

Disabled 3 tests files that are not ready for mac_views.

BUG=575036
==========

to

==========
mac_views_browser: fixed interactive_ui_tests compilation.

Splitting interactive_test_utils_mac.mm to
mac specific part - interactive_test_utils_mac.mm
and cocoa-views part - interactive_test_utils_cocoa.mm.

Splitting interactive_test_utils_views.cc to
interactive_test_utils_common_views.cc that can be used in mac tests
and interactive_test_utils_views.cc that used only in toolkit-views tests.

Disabled 3 tests files that are not ready for mac_views.

BUG=575036
==========

[tapted, 2016-02-05 05:11:27]

tapted@chromium.org changed reviewers:
- jackhou@chromium.org

[kirr, 2016-02-05 13:26:24]

Made changes in GN build.
Fixed review issues.

https://codereview.chromium.org/1669793002/diff/1/chrome/browser/ui/views/loc...
File
chrome/browser/ui/views/location_bar/page_action_image_view_interactive_uitest.cc
(right):

https://codereview.chromium.org/1669793002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/page_action_image_view_interactive_uitest.cc:16:
#include "chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.h"
On 2016/02/05 05:11:11, tapted wrote:
> can this be removed now that interactive_test_utils.h provides it instead?

Done.

https://codereview.chromium.org/1669793002/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/chrome_browser_ui.gy...
chrome/chrome_browser_ui.gypi:2113:
'browser/ui/views/translate/translate_bubble_view.cc',
On 2016/02/05 05:11:11, tapted wrote:
> if these aren't part of the ChromeMac release (e.g. behind
> chrome://flags/#mac-views-dialogs we shouldn't move them into
> chrome_browser_ui_views_sources yet, since it might be adding unnecessary
bloat
> or carry other risks)
> 
> Can we exclude the corresponding test files instead? (or maybe we just need to
> skip this bit of the delta for now)

Done.
Seem like it was needless.
Thanks.

https://codereview.chromium.org/1669793002/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1119: # Mac sources, except when mac_views_browser==1
On 2016/02/05 05:11:11, tapted wrote:
> nit: full stop after comment

Done.

https://codereview.chromium.org/1669793002/diff/1/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1719: 'conditions' : [
On 2016/02/05 05:11:11, tapted wrote:
> I've been putting a comment here, like
> 
> # The browser window can be views or Cocoa on Mac. Test accordingly.
> 
> Then this needs a corresponding BUILD.gn change in case we forget about it and
> accidentally lose some test coverage

Done.

https://codereview.chromium.org/1669793002/diff/1/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1725:
'browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm',
On 2016/02/05 05:11:11, tapted wrote:
> Can this just move into chrome_interactive_ui_test_cocoa_sources?

Done. I wanted to make these strings remarkable (to show that we need to
refactor tests).

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
File chrome/test/base/interactive_test_utils.h (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils.h:135: // Returns the center of |view|
in screen coordinates.
On 2016/02/05 05:11:11, tapted wrote:
> nit: blank line before

Done.

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
File chrome/test/base/interactive_test_utils_cocoa.mm (left):

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_cocoa.mm:11: #include "base/logging.h"
On 2016/02/05 05:11:11, tapted wrote:
> nit: keep this -- there are still DCHECKs below and the files "own" header
> (interactive_test_utils.h) isn't providing it

Done.

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
File chrome/test/base/interactive_test_utils_cocoa.mm (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_cocoa.mm:96: 
On 2016/02/05 05:11:11, tapted wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
File chrome/test/base/interactive_test_utils_common_views.cc (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_common_views.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2016/02/05 05:11:11, tapted wrote:
> nit: (c) 2012 -> 2016

Done.

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_common_views.cc:4: 
On 2016/02/05 05:11:11, tapted wrote:
> perhaps a comment in here about this file's purpose. E.g.
> 
> // Methods compiled on all toolkit-views platforms (including Mac).

Done.

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_common_views.cc:7: #include
"ui/base/test/ui_controls.h"
On 2016/02/05 05:11:11, tapted wrote:
> nit: I think this can be removed since interactive_test_utils.h provides it

Done.

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
File chrome/test/base/interactive_test_utils_views.cc (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_views.cc:10: #include
"ui/base/test/ui_controls.h"
On 2016/02/05 05:11:11, tapted wrote:
> nit: remove

Done.

[tapted, 2016-02-07 23:15:30]

lgtm with a few more nits fixed - thanks!

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
File chrome/test/base/interactive_test_utils_cocoa.mm (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_cocoa.mm:7: #include <Carbon/Carbon.h>
On 2016/02/05 05:11:11, tapted wrote:
> nit: I think this was only used for TransformProcessType, so can probably be
> removed too

ping?

https://codereview.chromium.org/1669793002/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1669793002/diff/20001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:1122: # Cocoa depended tests.
nit: remove comment && sort. I don't think
browser_action_button_interactive_uitest needs special treatment -- it's tightly
coupled to the Cocoa-based browser window, so when mac_views_browser substitutes
a toolkit-views based browser the test won't be needed

https://codereview.chromium.org/1669793002/diff/20001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1669793002/diff/20001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:497: if (mac_views_browser) {
nit: comment before `The browser window can be views or Cocoa on Mac. Test
accordingly.` (Hopefully Soon (tm) the .gyp version will go away...)

https://codereview.chromium.org/1669793002/diff/20001/chrome/test/base/intera...
File chrome/test/base/interactive_test_utils_common_views.cc (right):

https://codereview.chromium.org/1669793002/diff/20001/chrome/test/base/intera...
chrome/test/base/interactive_test_utils_common_views.cc:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
nit: no (c)

[kirr, 2016-02-08 14:40:43]

On 2016/02/07 23:15:30, tapted wrote:
> lgtm with a few more nits fixed - thanks!
> 
>
https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
> File chrome/test/base/interactive_test_utils_cocoa.mm (right):
> 
>
https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
> chrome/test/base/interactive_test_utils_cocoa.mm:7: #include <Carbon/Carbon.h>
> On 2016/02/05 05:11:11, tapted wrote:
> > nit: I think this was only used for TransformProcessType, so can probably be
> > removed too
> 
> ping?
> 
> https://codereview.chromium.org/1669793002/diff/20001/chrome/chrome_tests.gypi
> File chrome/chrome_tests.gypi (right):
> 
>
https://codereview.chromium.org/1669793002/diff/20001/chrome/chrome_tests.gyp...
> chrome/chrome_tests.gypi:1122: # Cocoa depended tests.
> nit: remove comment && sort. I don't think
> browser_action_button_interactive_uitest needs special treatment -- it's
tightly
> coupled to the Cocoa-based browser window, so when mac_views_browser
substitutes
> a toolkit-views based browser the test won't be needed
> 
> https://codereview.chromium.org/1669793002/diff/20001/chrome/test/BUILD.gn
> File chrome/test/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1669793002/diff/20001/chrome/test/BUILD.gn#ne...
> chrome/test/BUILD.gn:497: if (mac_views_browser) {
> nit: comment before `The browser window can be views or Cocoa on Mac. Test
> accordingly.` (Hopefully Soon (tm) the .gyp version will go away...)
> 
>
https://codereview.chromium.org/1669793002/diff/20001/chrome/test/base/intera...
> File chrome/test/base/interactive_test_utils_common_views.cc (right):
> 
>
https://codereview.chromium.org/1669793002/diff/20001/chrome/test/base/intera...
> chrome/test/base/interactive_test_utils_common_views.cc:1: // Copyright (c)
2016
> The Chromium Authors. All rights reserved.
> nit: no (c)

Sorry I'm on vacation now.
My colleague mblsha@yandex-team.ru agreed to finish this CL in two days.

[themblsha, 2016-02-08 17:08:57]

mblsha@yandex-team.ru changed reviewers:
+ mblsha@yandex-team.ru

[themblsha, 2016-02-08 17:08:58]

kirr@ is out on vacation and asked me to fix the remaining nits so it could be
merged this week.

I've created a new CL here with everything fixed:
https://codereview.chromium.org/1678163002/

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
File chrome/test/base/interactive_test_utils_cocoa.mm (right):

https://codereview.chromium.org/1669793002/diff/1/chrome/test/base/interactiv...
chrome/test/base/interactive_test_utils_cocoa.mm:7: #include <Carbon/Carbon.h>
On 2016/02/07 23:15:30, tapted wrote:
> On 2016/02/05 05:11:11, tapted wrote:
> > nit: I think this was only used for TransformProcessType, so can probably be
> > removed too
> 
> ping?

You're correct, it compiles without that header just fine.

https://codereview.chromium.org/1669793002/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1669793002/diff/20001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:1122: # Cocoa depended tests.
On 2016/02/07 23:15:30, tapted wrote:
> nit: remove comment && sort. I don't think
> browser_action_button_interactive_uitest needs special treatment -- it's
tightly
> coupled to the Cocoa-based browser window, so when mac_views_browser
substitutes
> a toolkit-views based browser the test won't be needed

Done.

https://codereview.chromium.org/1669793002/diff/20001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1669793002/diff/20001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:497: if (mac_views_browser) {
On 2016/02/07 23:15:30, tapted wrote:
> nit: comment before `The browser window can be views or Cocoa on Mac. Test
> accordingly.` (Hopefully Soon (tm) the .gyp version will go away...)

Done.

https://codereview.chromium.org/1669793002/diff/20001/chrome/test/base/intera...
File chrome/test/base/interactive_test_utils_common_views.cc (right):

https://codereview.chromium.org/1669793002/diff/20001/chrome/test/base/intera...
chrome/test/base/interactive_test_utils_common_views.cc:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
On 2016/02/07 23:15:30, tapted wrote:
> nit: no (c)

Done. It appears that the (c) omission is relatively recent, as lots of files
still have it intact.