Add place holder to move gfx::Display/Screen to ui/display

Add place holder to move gfx::Display/Screen to ui/display

I tried to move them and update includes, but it was just too many files to change at once. I'll move the class after all other files are migrated.

I covered ui/chromeos as a first target

BUG=597761
Committed: https://crrev.com/ef5fb8988b9d4a6539fcb9366ebafe204d02dc8e
Cr-Commit-Position: refs/heads/master@{#389557}

[oshima, 2016-04-22 21:58:45]

Patchset #2 (id:20001) has been deleted

[oshima, 2016-04-22 21:58:55]

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

[oshima, 2016-04-22 21:58:59]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-04-22 21:59:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1913613002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1913613002/40001

[oshima, 2016-04-22 22:07:47]

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

[commit-bot: I haz the power, 2016-04-22 22:08:26]

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

[oshima, 2016-04-22 23:05:13]

Patchset #1 (id:40001) has been deleted

[oshima, 2016-04-22 23:06:36]

Description was changed from

==========
Move display

BUG=
==========

to

==========
Add place holder to move gfx::Display/Screen to ui/display

convered ui/chromeos as a first target

BUG=597761
==========

[oshima, 2016-04-22 23:06:36]

oshima@chromium.org changed reviewers:
+ robliao@chromium.org

[oshima, 2016-04-22 23:09:51]

Patchset #1 (id:60001) has been deleted

[oshima, 2016-04-22 23:11:04]

Description was changed from

==========
Add place holder to move gfx::Display/Screen to ui/display

convered ui/chromeos as a first target

BUG=597761
==========

to

==========
Add place holder to move gfx::Display/Screen to ui/display

I tried to move them and update includes, but it was just too many files to
change at once. I'll move the class after all other files are migrated.

I convered ui/chromeos as a first target

BUG=597761
==========

[oshima, 2016-04-22 23:11:17]

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

[commit-bot: I haz the power, 2016-04-22 23:12:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1913613002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1913613002/80001

[robliao, 2016-04-22 23:13:56]

Description was changed from

==========
Add place holder to move gfx::Display/Screen to ui/display

I tried to move them and update includes, but it was just too many files to
change at once. I'll move the class after all other files are migrated.

I convered ui/chromeos as a first target

BUG=597761
==========

to

==========
Add place holder to move gfx::Display/Screen to ui/display

I tried to move them and update includes, but it was just too many files to
change at once. I'll move the class after all other files are migrated.

I covered ui/chromeos as a first target

BUG=597761
==========

[robliao, 2016-04-22 23:50:59]

The using type definition has the impact of not being able to forward declare
display::Display, display::DisplayObserver, and display::Screen during the
transition.

Is the plan to sweep through all the files to correct the declarations?

[commit-bot: I haz the power, 2016-04-23 00:16:10]

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

[commit-bot: I haz the power, 2016-04-23 00:16:11]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-04-23 00:44:33]

On 2016/04/23 00:16:11, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

As an alternative, wrapping the types appears to work. As an example,
display::Display can wrap gfx::Display and then have a temporary
display::Display -> gfx::Display _implicit_ conversion works on my end, allowing
the gradual propagation of types from source to consumer.
Implicit conversions are permitted by the style guide as long as the "project
leads" are okay with it.

Once gfx::Display references are gone, the last step would be to move the
gfx::Display implementation to display::Display and then remove the implicit
conversion.

[oshima, 2016-04-23 00:45:25]

On 2016/04/22 23:50:59, robliao wrote:
> The using type definition has the impact of not being able to forward declare
> display::Display, display::DisplayObserver, and display::Screen during the
> transition.
> 
> Is the plan to sweep through all the files to correct the declarations?

You can use using for forward decl, so you can with more typing. 

namespace gfx {
class Display;
}
namespace display {
using Display = gfx::Display;
}

[robliao, 2016-04-23 00:50:54]

On 2016/04/23 00:45:25, oshima wrote:
> On 2016/04/22 23:50:59, robliao wrote:
> > The using type definition has the impact of not being able to forward
declare
> > display::Display, display::DisplayObserver, and display::Screen during the
> > transition.
> > 
> > Is the plan to sweep through all the files to correct the declarations?
> 
> You can use using for forward decl, so you can with more typing. 
> 
> namespace gfx {
> class Display;
> }
> namespace display {
> using Display = gfx::Display;
> }

Right, but once we move gfx::Display to display::Display, we'll need to update
all 
using Display = gfx::Display to
class Display;

The wrapper method appears to have promise and allows forward declarations to be
used similar to today.

[robliao, 2016-04-23 01:02:24]

On 2016/04/23 00:50:54, robliao wrote:
> On 2016/04/23 00:45:25, oshima wrote:
> > On 2016/04/22 23:50:59, robliao wrote:
> > > The using type definition has the impact of not being able to forward
> declare
> > > display::Display, display::DisplayObserver, and display::Screen during the
> > > transition.
> > > 
> > > Is the plan to sweep through all the files to correct the declarations?
> > 
> > You can use using for forward decl, so you can with more typing. 
> > 
> > namespace gfx {
> > class Display;
> > }
> > namespace display {
> > using Display = gfx::Display;
> > }
> 
> Right, but once we move gfx::Display to display::Display, we'll need to update
> all 
> using Display = gfx::Display to
> class Display;
> 
> The wrapper method appears to have promise and allows forward declarations to
be
> used similar to today.

Discussed offline and two passes for forward declares is acknowledged,
continuing the review.

[robliao, 2016-04-23 01:15:36]

Looks generally okay. A few comments.

https://codereview.chromium.org/1913613002/diff/80001/ui/chromeos/DEPS
File ui/chromeos/DEPS (right):

https://codereview.chromium.org/1913613002/diff/80001/ui/chromeos/DEPS#newcode9
ui/chromeos/DEPS:9: "+ui/display",
There may need to be a corresponding build change. ui_chromeos currently does
not depend on //ui/display

https://codereview.chromium.org/1913613002/diff/80001/ui/display/manager/disp...
File ui/display/manager/display_layout.h (right):

https://codereview.chromium.org/1913613002/diff/80001/ui/display/manager/disp...
ui/display/manager/display_layout.h:29: }
Nit: This can combine with the namespace below.

https://codereview.chromium.org/1913613002/diff/80001/ui/display/screen.h
File ui/display/screen.h (right):

https://codereview.chromium.org/1913613002/diff/80001/ui/display/screen.h#new...
ui/display/screen.h:12: class Display;
Are these forward declares necessary?

https://codereview.chromium.org/1913613002/diff/80001/ui/display/screen.h#new...
ui/display/screen.h:19: using DisplayObserver = gfx::DisplayObserver;
The Display and DisplayObserver types should be removed.

Consumers of screen.h should directly include display.h and display_observer.h
or forward declare them.

[oshima, 2016-04-23 01:30:44]

https://codereview.chromium.org/1913613002/diff/80001/ui/chromeos/DEPS
File ui/chromeos/DEPS (right):

https://codereview.chromium.org/1913613002/diff/80001/ui/chromeos/DEPS#newcode9
ui/chromeos/DEPS:9: "+ui/display",
On 2016/04/23 01:15:36, robliao wrote:
> There may need to be a corresponding build change. ui_chromeos currently does
> not depend on //ui/display

Done. Thank you for the catch. It's probably pulling dep from other public_deps,
but explicit is better.

https://codereview.chromium.org/1913613002/diff/80001/ui/display/manager/disp...
File ui/display/manager/display_layout.h (right):

https://codereview.chromium.org/1913613002/diff/80001/ui/display/manager/disp...
ui/display/manager/display_layout.h:29: }
On 2016/04/23 01:15:36, robliao wrote:
> Nit: This can combine with the namespace below.

Done.

https://codereview.chromium.org/1913613002/diff/80001/ui/display/screen.h
File ui/display/screen.h (right):

https://codereview.chromium.org/1913613002/diff/80001/ui/display/screen.h#new...
ui/display/screen.h:19: using DisplayObserver = gfx::DisplayObserver;
On 2016/04/23 01:15:36, robliao wrote:
> The Display and DisplayObserver types should be removed.
> 
> Consumers of screen.h should directly include display.h and display_observer.h
> or forward declare them.

These are because Screen has (and needs) forward decl of these classes. They do
not have to be the real consumer.
For example, consider defining a subclass of screen like this:

class MyClass : public display::Screen {
  void AddObserver(display::DisplayObserver* observer) override;
  display::Display GetPrimaryDisplay() overrride;
}

[robliao, 2016-04-23 01:40:43]

https://codereview.chromium.org/1913613002/diff/80001/ui/display/screen.h
File ui/display/screen.h (right):

https://codereview.chromium.org/1913613002/diff/80001/ui/display/screen.h#new...
ui/display/screen.h:12: class Display;
On 2016/04/23 01:15:36, robliao wrote:
> Are these forward declares necessary?

This should probably be handled by screen.h based off of the below.

https://codereview.chromium.org/1913613002/diff/80001/ui/display/screen.h#new...
ui/display/screen.h:19: using DisplayObserver = gfx::DisplayObserver;
On 2016/04/23 01:30:44, oshima wrote:
> On 2016/04/23 01:15:36, robliao wrote:
> > The Display and DisplayObserver types should be removed.
> > 
> > Consumers of screen.h should directly include display.h and
display_observer.h
> > or forward declare them.
> 
> These are because Screen has (and needs) forward decl of these classes. They
do
> not have to be the real consumer.
> For example, consider defining a subclass of screen like this:
> 
> class MyClass : public display::Screen {
>   void AddObserver(display::DisplayObserver* observer) override;
>   display::Display GetPrimaryDisplay() overrride;
> }

Noted. Just making sure we're not missing an opportunity to fix up files that
perform transitive includes.

https://codereview.chromium.org/1913613002/diff/100001/ui/chromeos/BUILD.gn
File ui/chromeos/BUILD.gn (right):

https://codereview.chromium.org/1913613002/diff/100001/ui/chromeos/BUILD.gn#n...
ui/chromeos/BUILD.gn:59: "//ui/display",
ui_chromeos.gyp needs this too.

[oshima, 2016-04-23 04:50:04]

https://codereview.chromium.org/1913613002/diff/80001/ui/display/screen.h
File ui/display/screen.h (right):

https://codereview.chromium.org/1913613002/diff/80001/ui/display/screen.h#new...
ui/display/screen.h:12: class Display;
On 2016/04/23 01:40:43, robliao wrote:
> On 2016/04/23 01:15:36, robliao wrote:
> > Are these forward declares necessary?
> 
> This should probably be handled by screen.h based off of the below.

Done.

https://codereview.chromium.org/1913613002/diff/100001/ui/chromeos/BUILD.gn
File ui/chromeos/BUILD.gn (right):

https://codereview.chromium.org/1913613002/diff/100001/ui/chromeos/BUILD.gn#n...
ui/chromeos/BUILD.gn:59: "//ui/display",
On 2016/04/23 01:40:43, robliao wrote:
> ui_chromeos.gyp needs this too.

Done.

[oshima, 2016-04-23 04:50:20]

Patchset #3 (id:120001) has been deleted

[oshima, 2016-04-23 04:50:31]

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

[commit-bot: I haz the power, 2016-04-23 04:50:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1913613002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1913613002/140001

[commit-bot: I haz the power, 2016-04-23 05:48:13]

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

[commit-bot: I haz the power, 2016-04-23 05:48:14]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-04-25 17:15:49]

lgtm

[oshima, 2016-04-25 18:27:35]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2016-04-25 18:28:02]

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

[commit-bot: I haz the power, 2016-04-25 20:44:17]

Description was changed from

==========
Add place holder to move gfx::Display/Screen to ui/display

I tried to move them and update includes, but it was just too many files to
change at once. I'll move the class after all other files are migrated.

I covered ui/chromeos as a first target

BUG=597761
==========

to

==========
Add place holder to move gfx::Display/Screen to ui/display

I tried to move them and update includes, but it was just too many files to
change at once. I'll move the class after all other files are migrated.

I covered ui/chromeos as a first target

BUG=597761
==========

[commit-bot: I haz the power, 2016-04-25 20:44:19]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2016-04-25 20:46:46]

Description was changed from

==========
Add place holder to move gfx::Display/Screen to ui/display

I tried to move them and update includes, but it was just too many files to
change at once. I'll move the class after all other files are migrated.

I covered ui/chromeos as a first target

BUG=597761
==========

to

==========
Add place holder to move gfx::Display/Screen to ui/display

I tried to move them and update includes, but it was just too many files to
change at once. I'll move the class after all other files are migrated.

I covered ui/chromeos as a first target

BUG=597761

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

[commit-bot: I haz the power, 2016-04-25 20:46:48]

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