Ash Flag on Linux: Modified and Renamed Factory Files

Ash Flag on Linux: Modified and Renamed Factory Files

Working towards making the USE_ASH flag work on Linux.
Changed ash factories
- browser_non_client_frame_view_factory_ash
- immersive_mode_controller_factory_ash
into more general 'view' factories, so that they can be included in USE_ASH=False builds. Ash related code in these new factory files is excluded by #if-defines for non-ash builds.

BUG=565641
Committed: https://crrev.com/e9240f028b0c02aeee9f886b01e88e16d8066fd7
Cr-Commit-Position: refs/heads/master@{#363957}

[wierichs, 2015-12-03 05:04:35]

wierichs@google.com changed reviewers:
+ benwells@chromium.org

[wierichs, 2015-12-03 09:58:35]

Description was changed from

==========
Ash flag working on Linux: Modified factory files
==========

to

==========
Ash flag on Linux: Modified factory files
==========

[wierichs, 2015-12-03 23:45:04]

Description was changed from

==========
Ash flag on Linux: Modified factory files
==========

to

==========
Ash Flag on Linux: Modified and Renamed Factory Files

Changed ash factories
- browser_non_client_frame_view_factory_ash
- immersive_mode_controller_factory_ash
into more general 'view' factories, so that they can be included in
USE_ASH=False builds. Ash related code in these new factory files is excluded by
#if-defines for non-ash builds.

BUG=565641
==========

[wierichs, 2015-12-03 23:45:55]

Description was changed from

==========
Ash Flag on Linux: Modified and Renamed Factory Files

Changed ash factories
- browser_non_client_frame_view_factory_ash
- immersive_mode_controller_factory_ash
into more general 'view' factories, so that they can be included in
USE_ASH=False builds. Ash related code in these new factory files is excluded by
#if-defines for non-ash builds.

BUG=565641
==========

to

==========
Ash Flag on Linux: Modified and Renamed Factory Files

Working towards making the USE_ASH flag work on Linux.
Changed ash factories
- browser_non_client_frame_view_factory_ash
- immersive_mode_controller_factory_ash
into more general 'view' factories, so that they can be included in
USE_ASH=False builds. Ash related code in these new factory files is excluded by
#if-defines for non-ash builds.

BUG=565641
==========

[benwells, 2015-12-04 00:42:37]

(still going on this review, but forwarding on this bit for now...)

https://codereview.chromium.org/1498733003/diff/1/chrome/browser/ui/views/fra...
File
chrome/browser/ui/views/frame/browser_non_client_frame_view_factory_views.cc
(right):

https://codereview.chromium.org/1498733003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view_factory_views.cc:9:
#include "chrome/browser/ui/views/frame/browser_view.h"
This included should go first.

https://codereview.chromium.org/1498733003/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (left):

https://codereview.chromium.org/1498733003/diff/1/chrome/chrome_browser_ui.gy...
chrome/chrome_browser_ui.gypi:2109: 'browser/ui/views/ash/tab_scrubber.h',
Some of these changes are unrelated to the factory stuff, so you should split
them otu into the other 'all the trivial stuff' CL.

[benwells, 2015-12-07 05:03:23]

https://codereview.chromium.org/1498733003/diff/20001/chrome/browser/ui/views...
File
chrome/browser/ui/views/frame/browser_non_client_frame_view_factory_views.cc
(right):

https://codereview.chromium.org/1498733003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_factory_views.cc:24:
#if defined(OS_CHROMEOS) && defined(USE_ASH)
This isn't logically equivalent - windows and linux should be able to create
BrowserNonClientFrameViewAsh if USE_ASH is defined.

As pointed out on the other CL by estade@, host_desktop_type and
DESKTOP_TYPE_NATIVE should always be defined, so you don't need to change this
so dramatically. You should be able to just wrap the
BrowserNonClientFrameViewAsh* creation at the end of the original version in #if
USE_ASH, and add something like NOTREACHED(); return nullptr in the #else (or
something like that).

https://codereview.chromium.org/1498733003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/immersive_mode_controller_factory_views.cc
(right):

https://codereview.chromium.org/1498733003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/immersive_mode_controller_factory_views.cc:6:
#include "chrome/browser/ui/views/frame/immersive_mode_controller_ash.h"
This _ash include should also be put into a #if USE_ASH block, and not included
in the build if ash isn't turned on.

[wierichs, 2015-12-07 23:49:39]

https://codereview.chromium.org/1498733003/diff/20001/chrome/browser/ui/views...
File
chrome/browser/ui/views/frame/browser_non_client_frame_view_factory_views.cc
(right):

https://codereview.chromium.org/1498733003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_factory_views.cc:24:
#if defined(OS_CHROMEOS) && defined(USE_ASH)
On 2015/12/07 05:03:22, benwells wrote:
> This isn't logically equivalent - windows and linux should be able to create
> BrowserNonClientFrameViewAsh if USE_ASH is defined.
> 
> As pointed out on the other CL by estade@, host_desktop_type and
> DESKTOP_TYPE_NATIVE should always be defined, so you don't need to change this
> so dramatically. You should be able to just wrap the
> BrowserNonClientFrameViewAsh* creation at the end of the original version in
#if
> USE_ASH, and add something like NOTREACHED(); return nullptr in the #else (or
> something like that).

Done.

https://codereview.chromium.org/1498733003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/immersive_mode_controller_factory_views.cc
(right):

https://codereview.chromium.org/1498733003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/immersive_mode_controller_factory_views.cc:6:
#include "chrome/browser/ui/views/frame/immersive_mode_controller_ash.h"
On 2015/12/07 05:03:23, benwells wrote:
> This _ash include should also be put into a #if USE_ASH block, and not
included
> in the build if ash isn't turned on.

Done.

[benwells, 2015-12-08 01:48:06]

lgtm

lgtm with one change removed

https://codereview.chromium.org/1498733003/diff/40001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (left):

https://codereview.chromium.org/1498733003/diff/40001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:1338:
'browser/ui/views/frame/native_browser_frame_factory_desktop_ash.cc',
This file is moving from linux desktop, to all ash.

I don't think that is right .... we might need a new group for linux ash if we
want to keep linux ash compiling ... or something - looking at that file it is
pretty trivial and a little bit confusing.

To move forward with this CL, I think you should remove this change and do it in
a separate patch.

[wierichs, 2015-12-08 07:24:00]

wierichs@google.com changed reviewers:
+ thestig@chromium.org

[wierichs, 2015-12-08 07:24:01]

+thestig for owner's review.

[Lei Zhang, 2015-12-08 09:02:03]

lgtm

[wierichs, 2015-12-09 01:49:39]

The CQ bit was checked by wierichs@google.com

[wierichs, 2015-12-09 01:49:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/1498733003/#ps60001
(title: "Put ash file back into linux in gyp")

[wierichs, 2015-12-09 01:49:46]

The CQ bit was unchecked by wierichs@google.com

[wierichs, 2015-12-09 01:54:17]

The CQ bit was checked by wierichs@google.com

[commit-bot: I haz the power, 2015-12-09 01:58:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1498733003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1498733003/60001

[commit-bot: I haz the power, 2015-12-09 03:06:13]

Description was changed from

==========
Ash Flag on Linux: Modified and Renamed Factory Files

Working towards making the USE_ASH flag work on Linux.
Changed ash factories
- browser_non_client_frame_view_factory_ash
- immersive_mode_controller_factory_ash
into more general 'view' factories, so that they can be included in
USE_ASH=False builds. Ash related code in these new factory files is excluded by
#if-defines for non-ash builds.

BUG=565641
==========

to

==========
Ash Flag on Linux: Modified and Renamed Factory Files

Working towards making the USE_ASH flag work on Linux.
Changed ash factories
- browser_non_client_frame_view_factory_ash
- immersive_mode_controller_factory_ash
into more general 'view' factories, so that they can be included in
USE_ASH=False builds. Ash related code in these new factory files is excluded by
#if-defines for non-ash builds.

BUG=565641
==========

[commit-bot: I haz the power, 2015-12-09 03:06:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-09 03:07:10]

Description was changed from

==========
Ash Flag on Linux: Modified and Renamed Factory Files

Working towards making the USE_ASH flag work on Linux.
Changed ash factories
- browser_non_client_frame_view_factory_ash
- immersive_mode_controller_factory_ash
into more general 'view' factories, so that they can be included in
USE_ASH=False builds. Ash related code in these new factory files is excluded by
#if-defines for non-ash builds.

BUG=565641
==========

to

==========
Ash Flag on Linux: Modified and Renamed Factory Files

Working towards making the USE_ASH flag work on Linux.
Changed ash factories
- browser_non_client_frame_view_factory_ash
- immersive_mode_controller_factory_ash
into more general 'view' factories, so that they can be included in
USE_ASH=False builds. Ash related code in these new factory files is excluded by
#if-defines for non-ash builds.

BUG=565641

Committed: https://crrev.com/e9240f028b0c02aeee9f886b01e88e16d8066fd7
Cr-Commit-Position: refs/heads/master@{#363957}
==========

[commit-bot: I haz the power, 2015-12-09 03:07:11]

Patchset 4 (id:??) landed as
https://crrev.com/e9240f028b0c02aeee9f886b01e88e16d8066fd7
Cr-Commit-Position: refs/heads/master@{#363957}