Unify display ids between Display and PlatformDisplay.

Unify display ids between Display and PlatformDisplay.

Instead of having an int32_t and int64_t id for each display, unify them
to use a single int64_t id. The int64_t id is the actual display id and
is provided by lower level code, so it makes more sense to keep it.

BUG=625876
Committed: https://crrev.com/200f7a114aa28e831677db649f134e7346d825eb
Cr-Commit-Position: refs/heads/master@{#408632}

[kylechar, 2016-07-28 16:51:05]

kylechar@chromium.org changed reviewers:
+ msw@chromium.org

[kylechar, 2016-07-28 16:52:27]

Description was changed from

==========
Unify display ids between Display and PlatformDisplay.

Instead of having an int32_t and int64_t id for each display, unify them
to use a single int64_t id. The int64_t id is the actual display id and
it's provided by lower levels so use it instead.

BUG=625876
==========

to

==========
Unify display ids between Display and PlatformDisplay.

Instead of having an int32_t and int64_t id for each display, unify them
to use a single int64_t id. The int64_t id is the actual display id and
is provided by lower level code, so it makes more sense to keep it.

BUG=625876
==========

[msw, 2016-07-28 18:05:32]

nice! lgtm with a nit; I'll look again if you address it.

https://codereview.chromium.org/2189893004/diff/1/services/ui/ws/display.h
File services/ui/ws/display.h (right):

https://codereview.chromium.org/2189893004/diff/1/services/ui/ws/display.h#ne...
services/ui/ws/display.h:69: int64_t id() const { return
platform_display_->GetId(); }
nit: rename GetId, don't define here (optionally TODO now, follow up later)
(or move DefaultPlatformDisplay::id_ to PlatformDisplay and make the virtual
GetId into a simple accessor, if that's okay for TestPlatformDisplay)

[kylechar, 2016-07-28 18:49:22]

Made some changes you suggested. The tests definitely would have failed with
what I had before. PTAL.

https://codereview.chromium.org/2189893004/diff/1/services/ui/ws/display.h
File services/ui/ws/display.h (right):

https://codereview.chromium.org/2189893004/diff/1/services/ui/ws/display.h#ne...
services/ui/ws/display.h:69: int64_t id() const { return
platform_display_->GetId(); }
On 2016/07/28 18:05:32, msw wrote:
> nit: rename GetId, don't define here (optionally TODO now, follow up later)
> (or move DefaultPlatformDisplay::id_ to PlatformDisplay and make the virtual
> GetId into a simple accessor, if that's okay for TestPlatformDisplay)

Right, tests. They'll need unique ids now too so I've made it
PlatformDisplay::id_ and a simple accessor.

[msw, 2016-07-28 19:01:02]

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/display....
services/ui/ws/display.cc:75: int64_t Display::GetId() const {
optional nit: this seems like it'd be okay as a simple accessor.
(you may change this to |id()| and define in the header if desired)
(though, the |display_manager()| simple accessors are defined here)

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/platform...
File services/ui/ws/platform_display.h (right):

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/platform...
services/ui/ws/platform_display.h:55: PlatformDisplay(int64_t id);
Oh dang; I saw |set_factory_for_testing| and thought this was already an
abstract class, not a pure-virtual interface... I'm more hesitant to make this
change now, but feel free to proceed if you think it's okay. Feel free to
consult a more knowledgeable reviewer, like sky, rjkroege, or sadrul.

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/platform...
services/ui/ws/platform_display.h:104: };
nit: DISALLOW_COPY_AND_ASSIGN(PlatformDisplay); if not pure-virtual.

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/test_uti...
File services/ui/ws/test_utils.h (right):

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/test_uti...
services/ui/ws/test_utils.h:248: int64_t next_display_id = 1;
nit: trailing underscore

[kylechar, 2016-07-28 19:20:16]

Thanks msw!

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/display....
services/ui/ws/display.cc:75: int64_t Display::GetId() const {
On 2016/07/28 19:01:02, msw wrote:
> optional nit: this seems like it'd be okay as a simple accessor.
> (you may change this to |id()| and define in the header if desired)
> (though, the |display_manager()| simple accessors are defined here)

I'll probably leave it like this, it better conveys the ownership (and I won't
have to change anything it again if I end making PlatformDisplay pure virtual
again).

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/platform...
File services/ui/ws/platform_display.h (right):

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/platform...
services/ui/ws/platform_display.h:55: PlatformDisplay(int64_t id);
On 2016/07/28 19:01:02, msw wrote:
> Oh dang; I saw |set_factory_for_testing| and thought this was already an
> abstract class, not a pure-virtual interface... I'm more hesitant to make this
> change now, but feel free to proceed if you think it's okay. Feel free to
> consult a more knowledgeable reviewer, like sky, rjkroege, or sadrul.

I think sky is OOO but I'll talk with sadrul or rjkroege. It seems like it
should be okay, having an id is important now that we're using the
PlatformDisplay id for everything, so might as well implement it once.

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/platform...
services/ui/ws/platform_display.h:104: };
On 2016/07/28 19:01:02, msw wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(PlatformDisplay); if not pure-virtual.

Done.

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/test_uti...
File services/ui/ws/test_utils.h (right):

https://codereview.chromium.org/2189893004/diff/20001/services/ui/ws/test_uti...
services/ui/ws/test_utils.h:248: int64_t next_display_id = 1;
On 2016/07/28 19:01:02, msw wrote:
> nit: trailing underscore

Done.

[msw, 2016-07-28 19:22:21]

lgtm

[kylechar, 2016-07-28 19:28:08]

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

[commit-bot: I haz the power, 2016-07-28 19:28:34]

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

[kylechar, 2016-07-29 13:06:34]

Talking with sadrul@ he thought changing the class from pure virtual to abstract
should happen in a separate CL. So I've undone that and added an id_ field to
TestPlatformDisplay. Otherwise everything is unchanged.

[kylechar, 2016-07-29 13:06:40]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-07-29 13:06:41]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2189893004/#ps60001
(title: "Back to pure virtual.")

[commit-bot: I haz the power, 2016-07-29 13:06:55]

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

[commit-bot: I haz the power, 2016-07-29 13:57:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-29 13:58:41]

Description was changed from

==========
Unify display ids between Display and PlatformDisplay.

Instead of having an int32_t and int64_t id for each display, unify them
to use a single int64_t id. The int64_t id is the actual display id and
is provided by lower level code, so it makes more sense to keep it.

BUG=625876
==========

to

==========
Unify display ids between Display and PlatformDisplay.

Instead of having an int32_t and int64_t id for each display, unify them
to use a single int64_t id. The int64_t id is the actual display id and
is provided by lower level code, so it makes more sense to keep it.

BUG=625876

Committed: https://crrev.com/200f7a114aa28e831677db649f134e7346d825eb
Cr-Commit-Position: refs/heads/master@{#408632}
==========

[commit-bot: I haz the power, 2016-07-29 13:58:42]

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