Fix expected PlacementList sort order.

Fix expected PlacementList sort order.

We sort the displays IDs list by the least significant 8 bits of the IDs.
This list is used to fill in the placement list, however,
DisplayLayout::Validate() expects the list to be sorted using the full
display IDs. This resulted in false positives where we skip applying valid
layouts.

We also should never persist an invalid layout.

BUG=700020
TEST=manually, +test.

Review-Url: https://codereview.chromium.org/2814843007
Cr-Commit-Position: refs/heads/master@{#464654}
Committed: https://chromium.googlesource.com/chromium/src/+/d3d4ea43bd26fafa27ae723b7db6ada167034248

[afakhry, 2017-04-12 23:34:18]

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

[commit-bot: I haz the power, 2017-04-12 23:35:33]

Dry run: 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, 2017-04-12 23:42:13]

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

[commit-bot: I haz the power, 2017-04-12 23:42:14]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[afakhry, 2017-04-12 23:47:48]

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

[afakhry, 2017-04-12 23:48:28]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-04-12 23:48:45]

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

[afakhry, 2017-04-13 00:49:03]

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

[afakhry, 2017-04-13 00:49:04]

Oshima-san, please take a look at this change. Thanks!

https://codereview.chromium.org/2814843007/diff/20001/ui/display/display.h
File ui/display/display.h (right):

https://codereview.chromium.org/2814843007/diff/20001/ui/display/display.h#ne...
ui/display/display.h:26: DISPLAY_EXPORT bool CompareDisplayIds(int64_t id1,
int64_t id2);
This was moved from ui/display/manager/display_manager_utilities.h due to
dependency as we want to use in display_layout.

[commit-bot: I haz the power, 2017-04-13 00:56:32]

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

[commit-bot: I haz the power, 2017-04-13 00:56:33]

Dry run: This issue passed the CQ dry run.

[afakhry, 2017-04-14 00:55:37]

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

[commit-bot: I haz the power, 2017-04-14 00:56:18]

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

[afakhry, 2017-04-14 00:57:27]

Patchset #2 (id:40001) has been deleted

[afakhry, 2017-04-14 00:58:26]

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

[commit-bot: I haz the power, 2017-04-14 00:58:56]

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

[afakhry, 2017-04-14 00:59:17]

Oshima-san, as agreed, we don't allow to register invalid layouts, but revert to
a default one.

[oshima, 2017-04-14 01:19:41]

lgtm

[afakhry, 2017-04-14 01:26:54]

The CQ bit was unchecked by afakhry@chromium.org

[afakhry, 2017-04-14 01:26:56]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2017-04-14 01:27:07]

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, 2017-04-14 02:01:47]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492133216572370,
"parent_rev": "a1bd9cb4fe2557db40620f62766eb2e111b12de1", "commit_rev":
"d3d4ea43bd26fafa27ae723b7db6ada167034248"}

[commit-bot: I haz the power, 2017-04-14 02:03:13]

Description was changed from

==========
Fix expected PlacementList sort order.

We sort the displays IDs list by the least significant 8 bits of the IDs.
This list is used to fill in the placement list, however,
DisplayLayout::Validate() expects the list to be sorted using the full
display IDs. This resulted in false positives where we skip applying valid
layouts.

We also should never persist an invalid layout.

BUG=700020
TEST=manually, +test.
==========

to

==========
Fix expected PlacementList sort order.

We sort the displays IDs list by the least significant 8 bits of the IDs.
This list is used to fill in the placement list, however,
DisplayLayout::Validate() expects the list to be sorted using the full
display IDs. This resulted in false positives where we skip applying valid
layouts.

We also should never persist an invalid layout.

BUG=700020
TEST=manually, +test.

Review-Url: https://codereview.chromium.org/2814843007
Cr-Commit-Position: refs/heads/master@{#464654}
Committed:
https://chromium.googlesource.com/chromium/src/+/d3d4ea43bd26fafa27ae723b7db6...
==========

[commit-bot: I haz the power, 2017-04-14 02:03:14]

Committed patchset #2 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d3d4ea43bd26fafa27ae723b7db6...

[Nico, 2017-07-28 21:09:57]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2017-07-28 21:09:58]

https://codereview.chromium.org/2814843007/diff/60001/ui/display/display.cc
File ui/display/display.cc (right):

https://codereview.chromium.org/2814843007/diff/60001/ui/display/display.cc#n...
ui/display/display.cc:63: DCHECK_NE(id1, id2);
This DCHECK is not correct. You're passing this to std::sort() which can call
this with the same element for lhs and rhs
(https://stackoverflow.com/questions/38966516/should-sorting-algorithm-pass-sa...).
We're trying to move cros builds to libc++
(https://chromium-review.googlesource.com/c/591775/) and one test fails due to
this incorrect DCHECK. I suppose this should be

  if (id1 == id2) return false;

instead?