MacViews: Fix gn check fail for mac_views_browser on Aura dependencies.

MacViews: Fix gn check fail for mac_views_browser on Aura dependencies.

GN doesn't know about preprocessor hash defines, so it fails on a gn check when
these hash defines are false. In this case, USE_AURA is false on macOS, so GN
doesn't add Aura dependencies as correctly specified in the BUILD files. But
when running `gn check`, it sees the Aura #includes, not knowing it's actually
wrapped in a preprocessor #ifdef. Make exceptions for these files by adding
// nogncheck comments to skip gn checks on the lines inside these #ifdefs.

BUG=644138

[Patti Lor, 2016-09-09 01:35:52]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 01:36:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2016-09-09 01:40:29]

Description was changed from

==========
Fix gn check fail for mac_views_browser constrained window Aura dependencies.

GN doesn't know about preprocessor hash defines, so it fails on a gn check when
these hash defines are false. In this case, USE_AURA is false on macOS, so GN
doesn't add Aura dependencies as correctly specified in the BUILD files. But
when running `gn check`, it sees the Aura #includes, not knowing it's actually
wrapped in a preprocessor #ifdef. Ignore gn checks on the lines inside these

BUG=644138
==========

to

==========
Fix gn check fail for mac_views_browser constrained window Aura dependencies.

GN doesn't know about preprocessor hash defines, so it fails on a gn check when
these hash defines are false. In this case, USE_AURA is false on macOS, so GN
doesn't add Aura dependencies as correctly specified in the BUILD files. But
when running `gn check`, it sees the Aura #includes, not knowing it's actually
wrapped in a preprocessor #ifdef. Ignore gn checks on the lines inside these
#ifdefs.

BUG=644138
==========

[commit-bot: I haz the power, 2016-09-09 03:08:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 03:08:14]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2016-09-16 06:14:06]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 06:14:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2016-09-16 06:17:47]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 06:17:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2016-09-16 06:32:08]

Description was changed from

==========
Fix gn check fail for mac_views_browser constrained window Aura dependencies.

GN doesn't know about preprocessor hash defines, so it fails on a gn check when
these hash defines are false. In this case, USE_AURA is false on macOS, so GN
doesn't add Aura dependencies as correctly specified in the BUILD files. But
when running `gn check`, it sees the Aura #includes, not knowing it's actually
wrapped in a preprocessor #ifdef. Ignore gn checks on the lines inside these
#ifdefs.

BUG=644138
==========

to

==========
MacViews: Fix gn check fail for mac_views_browser on Aura dependencies.

GN doesn't know about preprocessor hash defines, so it fails on a gn check when
these hash defines are false. In this case, USE_AURA is false on macOS, so GN
doesn't add Aura dependencies as correctly specified in the BUILD files. But
when running `gn check`, it sees the Aura #includes, not knowing it's actually
wrapped in a preprocessor #ifdef. Make exceptions for these files by adding
// nogncheck comments to skip gn checks on the lines inside these #ifdefs.

BUG=644138
==========

[Patti Lor, 2016-09-16 06:32:28]

Description was changed from

==========
MacViews: Fix gn check fail for mac_views_browser on Aura dependencies.

GN doesn't know about preprocessor hash defines, so it fails on a gn check when
these hash defines are false. In this case, USE_AURA is false on macOS, so GN
doesn't add Aura dependencies as correctly specified in the BUILD files. But
when running `gn check`, it sees the Aura #includes, not knowing it's actually
wrapped in a preprocessor #ifdef. Make exceptions for these files by adding
// nogncheck comments to skip gn checks on the lines inside these #ifdefs.

BUG=644138
==========

to

==========
MacViews: Fix gn check fail for mac_views_browser on Aura dependencies.

GN doesn't know about preprocessor hash defines, so it fails on a gn check when
these hash defines are false. In this case, USE_AURA is false on macOS, so GN
doesn't add Aura dependencies as correctly specified in the BUILD files. But
when running `gn check`, it sees the Aura #includes, not knowing it's actually
wrapped in a preprocessor #ifdef. Make exceptions for these files by adding
// nogncheck comments to skip gn checks on the lines inside these #ifdefs.

BUG=644138
==========

[commit-bot: I haz the power, 2016-09-16 09:23:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 09:23:42]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2016-09-19 01:24:18]

patricialor@chromium.org changed reviewers:
+ tapted@chromium.org

[Patti Lor, 2016-09-19 01:24:19]

Hi Trent, PTAL - this is a fix for all the targets using Aura and also happen to
be included in a mac_views_browser build failing 'gn check <outdir>'. I made a
mapping between each file with // nogncheck comments added and the place where
their Aura dependencies are included behind an if in the GN files:

content/browser/BUILD.gn:1771
 - browser_main_loop.cc
 - web_contents_view_guest.cc
 - web_contents_impl.cc

content/shell/BUILD.gn:321
 - layout_test_download_manager_delegate.cc
 - shell_download_manager_delegate.cc

ui/snapshot/BUILD.gn:56
 - screenshot_grabber.cc

ui/views/BUILD.gn:541
 - custom_button.cc
 - menu_host.cc
 - menu_controller.cc
 - widget_aura_utils.cc

ui/views/BUILD.gn:752
 - scoped_views_test_helper.cc
 - views_test_suite.cc
 - widget_unittest.cc

ui/views/BUILD.gn:936
 - custom_button_unittest.cc
 - menu_button_unittest.cc
 - menu_controller_unittest.cc

ui/views/examples/BUILD.gn:123
 - examples_main.cc

Hopefully it all checks out. I also made some changes to the some of the #ifdefs
to include USE_AURA as well, not sure if these are necessary since they
obviously work without them, but you can make the call on that. Thanks!

[tapted, 2016-09-19 03:23:37]

I started at the bottom, so read these comments backwards :). Then I stopped at
menu_host.cc - you might see nice fixes for the others. But the approach should
probably be:
 - investigate widget_unittest.cc (see comment)
 - Send to sky@ to see how he feels about // nogncheck

nogncheck should usually be considered a last-resort. For this stuff, we need to
strike a balance -- it might not be worth restructuring things in these files to
handle non-aura cases, since we eventually do want aura on Mac, and then most of
this becomes unnecessary.

https://codereview.chromium.org/2326933002/diff/40001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/2326933002/diff/40001/ui/views/controls/menu/...
ui/views/controls/menu/menu_host.cc:38: class PreMenuEventDispatchHandler :
public ui::EventHandler,
This should be moved to a file like
ui/views/controls/menu/pre_menu_event_dispatch_handler_aura.cc

https://codereview.chromium.org/2326933002/diff/40001/ui/views/test/scoped_vi...
File ui/views/test/scoped_views_test_helper.cc (right):

https://codereview.chromium.org/2326933002/diff/40001/ui/views/test/scoped_vi...
ui/views/test/scoped_views_test_helper.cc:33: ui::ContextFactory*
old_context_factory = nullptr;
This looks like it could easily be moved to ViewsTestHelperAura::SetUp() - this
block just needs to happen before the call to AuraTestHelper::SetUp(), and the
block below happens after.

https://codereview.chromium.org/2326933002/diff/40001/ui/views/views_test_sui...
File ui/views/views_test_suite.cc (right):

https://codereview.chromium.org/2326933002/diff/40001/ui/views/views_test_sui...
ui/views/views_test_suite.cc:19: #include <memory>
this is in the views_test_suite.h header, so redundant here

https://codereview.chromium.org/2326933002/diff/40001/ui/views/views_test_sui...
ui/views/views_test_suite.cc:50: env_ = aura::Env::CreateInstance();
Things using ViewsTestSuite probably also use ViewsTestHelper or AuraTestHelper,
both of which also use a unique_ptr<aura::Env>, but in a way that encapsulates
its use to aura-only files. It would be interesting to see if anything actually
relies on this... if it's only a couple of things, maybe they can be updated.
But if lots breaks, this should be kept. (an alternative could be to declare
something like

// views_test_suite.h
class ViewsTestSuite::PlatformPart {
 public:
  virtual ~PlatformPart() {}
  static unique_ptr<PlatformPart> Create();
 protected:
  PlatformPart() {} 
};

#if !defined(USE_AURA)
unique_ptr<ViewsTestSuite::PlatformPart> ViewsTestSuite::PlatformPart::Create()
{
  return MakeUnique(new PlatformPart);
}
#endif

views_test_suite_aura.cc
namespace {
class PlatformPartAura : public ViewsTestSuite::PlatformPart {
 / *etc */
};
}

unique_ptr<ViewsTestSuite::PlatformPart> ViewsTestSuite::PlatformPart::Create()
{
  return MakeUnique(new PlatformPartAura);
}



I think here, this would be a bit of overkill and // nogncheck is fine. However,
a similar pattern applied to the other cases could be an improvement to code
health.

https://codereview.chromium.org/2326933002/diff/40001/ui/views/widget/widget_...
File ui/views/widget/widget_aura_utils.h (right):

https://codereview.chromium.org/2326933002/diff/40001/ui/views/widget/widget_...
ui/views/widget/widget_aura_utils.h:9: #include "ui/wm/public/window_types.h" 
// nogncheck
yeah - this one doesn't feel right, since it's not inside an #ifdef guard.

I think the right fix for this is just to change bridged_native_widget.mm's
PositionWindowInScreenCoordinates() to switch based on Widget::InitParams::Type
so that it returns `true` for {FRAMELESS, POPUP, BUBBLE, DRAG}. The other types
should still be listed in the switch statement rather than having a default:
case - this helps mitigate the potential for the implementations to drift apart.

https://codereview.chromium.org/2326933002/diff/40001/ui/views/widget/widget_...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/2326933002/diff/40001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:40: #include "ui/aura/window_tree_host.h"  //
nogncheck
So the "alternative" to // nogncheck is to move the code that requires these
headers into a separate file. We should investigate all the cases in this CL -
it might not make sense to do that in all cases, but widget_unittest is probably
a good place to try it out. For example, the tests here that rely on these aura
headers might:
 - belong instead in native_widget_aura_unittest.cc, OR
 - sill be widget tests, but aura-specific so belong in a new file like
widget_aura_unittest.cc, OR
 - need to be enabled on Mac but are missing the right abstraction (e.g. a
generic interface in widget_test.h [similar to IsNativeWindowVisible()] that is
implemented in widget_test_mac.mm and widget_test_aura.cc