Fix errors in display resolution change notifications

Fix errors in display resolution change notifications

We used to prepare the resolution change notification after we change
the resolution by setting the mode. This resulted in erronous messages
reporting wrong resolutions or even failures that didn't happen, when
updating the displays (which triggers the actual creation of the
notification) happens synchronously when SetDisplayMode() is called.

This CL:
- Makes sure that always the notification data is prepared first
  before the display mode is set. If the display mode change fails, the
  prepared notification data is discarded.
- Makes sure updating the displays doesn't happen synchronously
  in SetDisplayMode() when there is a resolution change.

BUG=701389
TEST=manually

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

[afakhry, 2017-04-06 21:23:05]

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

[afakhry, 2017-04-06 21:23:06]

Steven, could you please take a look? Thanks!

[stevenjb, 2017-04-06 22:10:53]

https://codereview.chromium.org/2799963003/diff/1/chrome/browser/extensions/d...
File chrome/browser/extensions/display_info_provider_chromeos.cc (right):

https://codereview.chromium.org/2799963003/diff/1/chrome/browser/extensions/d...
chrome/browser/extensions/display_info_provider_chromeos.cc:347: //
crbug.com/701389.
This is all kind of awkward. It looks like we do this here because
DisplayManager doesn't have access to resolution_notification_controller?

Can we explain what all is going on here, e.g (I may have this wrong):
* We prepare an Ash notification here.
* If DisplayManager successfully sets the new display mode, it triggers the
actual notification (where?)
* If the user confirms the change (?) the notification calls StoreDisplayPrefs ?

[afakhry, 2017-04-07 00:04:58]

https://codereview.chromium.org/2799963003/diff/1/chrome/browser/extensions/d...
File chrome/browser/extensions/display_info_provider_chromeos.cc (right):

https://codereview.chromium.org/2799963003/diff/1/chrome/browser/extensions/d...
chrome/browser/extensions/display_info_provider_chromeos.cc:347: //
crbug.com/701389.
On 2017/04/06 22:10:53, stevenjb wrote:
> This is all kind of awkward. It looks like we do this here because
> DisplayManager doesn't have access to resolution_notification_controller?
> 
> Can we explain what all is going on here, e.g (I may have this wrong):
> * We prepare an Ash notification here.
> * If DisplayManager successfully sets the new display mode, it triggers the
> actual notification (where?)

ResolutionNotificationController::OnDisplayConfigurationChanged()

> * If the user confirms the change (?) the notification calls StoreDisplayPrefs
?

Yes, in ResolutionNotificationController::AcceptResolutionChange().

This should be documented in the doc for
ResolutionNotificationController::PrepareNotification() here:
https://cs.chromium.org/chromium/src/ash/display/resolution_notification_cont...

So in order to avoid repetition of docs, please let me know what's the degree of
details you want to include here.

[stevenjb, 2017-04-07 00:12:13]

On 2017/04/07 00:04:58, afakhry wrote:
>
https://codereview.chromium.org/2799963003/diff/1/chrome/browser/extensions/d...
> File chrome/browser/extensions/display_info_provider_chromeos.cc (right):
> 
>
https://codereview.chromium.org/2799963003/diff/1/chrome/browser/extensions/d...
> chrome/browser/extensions/display_info_provider_chromeos.cc:347: //
> crbug.com/701389.
> On 2017/04/06 22:10:53, stevenjb wrote:
> > This is all kind of awkward. It looks like we do this here because
> > DisplayManager doesn't have access to resolution_notification_controller?
> > 
> > Can we explain what all is going on here, e.g (I may have this wrong):
> > * We prepare an Ash notification here.
> > * If DisplayManager successfully sets the new display mode, it triggers the
> > actual notification (where?)
> 
> ResolutionNotificationController::OnDisplayConfigurationChanged()
> 
> > * If the user confirms the change (?) the notification calls
StoreDisplayPrefs
> ?
> 
> Yes, in ResolutionNotificationController::AcceptResolutionChange().
> 
> This should be documented in the doc for
> ResolutionNotificationController::PrepareNotification() here:
>
https://cs.chromium.org/chromium/src/ash/display/resolution_notification_cont...
> 
> So in order to avoid repetition of docs, please let me know what's the degree
of
> details you want to include here.

Feel free to reference that here, just please include some additional
explanation as to what is going on. I was uncertain and theoretically I am at
least passingly familiar with this code...

[afakhry, 2017-04-08 00:56:59]

Description was changed from

==========
Fix errors in display resolution change notifications

We used to prepare the resolution change notification after we change
the resolution by setting the mode. This resulted in erronous messages
reporting wrong resolutions or even failures that didn't happen.

BUG=701389
TEST=manually
==========

to

==========
Fix errors in display resolution change notifications

We used to prepare the resolution change notification after we change
the resolution by setting the mode. This resulted in erronous messages
reporting wrong resolutions or even failures that didn't happen, when
updating the displays (which triggers the actual creation of the
notification) happens synchronously when SetDisplayMode() is called.

This CL makes sure that always the notification data is prepared first
before the display mode is set. If the display mode change fails, the
prepared notification data is discarded.

It also makes sure updating the displays doesn't happen synchronously
in SetDisplayMode() when there is a resolution change.

BUG=701389
TEST=manually
==========

[afakhry, 2017-04-08 01:32:59]

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

[commit-bot: I haz the power, 2017-04-08 01:33:34]

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-08 01:36:33]

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

[commit-bot: I haz the power, 2017-04-08 01:36:35]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[afakhry, 2017-04-08 02:42:15]

Description was changed from

==========
Fix errors in display resolution change notifications

We used to prepare the resolution change notification after we change
the resolution by setting the mode. This resulted in erronous messages
reporting wrong resolutions or even failures that didn't happen, when
updating the displays (which triggers the actual creation of the
notification) happens synchronously when SetDisplayMode() is called.

This CL makes sure that always the notification data is prepared first
before the display mode is set. If the display mode change fails, the
prepared notification data is discarded.

It also makes sure updating the displays doesn't happen synchronously
in SetDisplayMode() when there is a resolution change.

BUG=701389
TEST=manually
==========

to

==========
Fix errors in display resolution change notifications

We used to prepare the resolution change notification after we change
the resolution by setting the mode. This resulted in erronous messages
reporting wrong resolutions or even failures that didn't happen, when
updating the displays (which triggers the actual creation of the
notification) happens synchronously when SetDisplayMode() is called.

This CL:
- Makes sure that always the notification data is prepared first
  before the display mode is set. If the display mode change fails, the
  prepared notification data is discarded.
- Makes sure updating the displays doesn't happen synchronously
  in SetDisplayMode() when there is a resolution change.
- Fixes the crash when we revert to an old resolution for an already
  removed display.

BUG=701389,709722
TEST=manually
==========

[afakhry, 2017-04-08 03:09:12]

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

[commit-bot: I haz the power, 2017-04-08 03:09:23]

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-08 03:10:23]

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

[afakhry, 2017-04-08 03:10:24]

+Oshima-san. Please take a look at the display related changes. Thanks!

[commit-bot: I haz the power, 2017-04-08 03:16:21]

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

[commit-bot: I haz the power, 2017-04-08 03:16:23]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[afakhry, 2017-04-10 16:32:34]

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

[commit-bot: I haz the power, 2017-04-10 16:33:00]

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-10 17:24:21]

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

[commit-bot: I haz the power, 2017-04-10 17:24:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stevenjb, 2017-04-10 17:33:52]

https://codereview.chromium.org/2799963003/diff/60001/ash/display/resolution_...
File ash/display/resolution_notification_controller.h (right):

https://codereview.chromium.org/2799963003/diff/60001/ash/display/resolution_...
ash/display/resolution_notification_controller.h:61: const base::Closure&
accept_callback) WARN_UNUSED_RESULT;
I like this change and the explanation, but should we move this into a helper
file, e.g. display_util? (Maybe keep PrepareNotification more-or-less as it
was?)

[oshima, 2017-04-10 17:46:18]

lgtm

https://codereview.chromium.org/2799963003/diff/60001/ash/display/resolution_...
File ash/display/resolution_notification_controller.h (right):

https://codereview.chromium.org/2799963003/diff/60001/ash/display/resolution_...
ash/display/resolution_notification_controller.h:61: const base::Closure&
accept_callback) WARN_UNUSED_RESULT;
On 2017/04/10 17:33:52, stevenjb wrote:
> I like this change and the explanation, but should we move this into a helper
> file, e.g. display_util? (Maybe keep PrepareNotification more-or-less as it
> was?)
> 

I don't have strong opinion, and leave it to you guys.

[oshima, 2017-04-10 17:46:19]

lgtm

[afakhry, 2017-04-10 23:26:21]

https://codereview.chromium.org/2799963003/diff/60001/ash/display/resolution_...
File ash/display/resolution_notification_controller.h (right):

https://codereview.chromium.org/2799963003/diff/60001/ash/display/resolution_...
ash/display/resolution_notification_controller.h:61: const base::Closure&
accept_callback) WARN_UNUSED_RESULT;
On 2017/04/10 17:46:18, oshima wrote:
> On 2017/04/10 17:33:52, stevenjb wrote:
> > I like this change and the explanation, but should we move this into a
helper
> > file, e.g. display_util? (Maybe keep PrepareNotification more-or-less as it
> > was?)
> > 
> 
> I don't have strong opinion, and leave it to you guys.

I also don't have a strong opinion about this, but one of the pros that I see
for keeping these changes here instead of display_util, is that now users are
forced to use this API correctly, no other way is provided.

Also, moving to display_util, we'll need to add a function to discard the
prepared notification data if SetDisplayMode() fails as opposed to keeping that
internal here.

Let me think about it more...

[stevenjb, 2017-04-10 23:33:43]

If oshima is OK with this as-is, lgtm. Maybe we can consider further cleanup in
a followup.

[afakhry, 2017-04-13 01:20:49]

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

[commit-bot: I haz the power, 2017-04-13 01:22:17]

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 01:22:34]

The CQ bit was unchecked by afakhry@chromium.org

[afakhry, 2017-04-13 01:23:37]

Description was changed from

==========
Fix errors in display resolution change notifications

We used to prepare the resolution change notification after we change
the resolution by setting the mode. This resulted in erronous messages
reporting wrong resolutions or even failures that didn't happen, when
updating the displays (which triggers the actual creation of the
notification) happens synchronously when SetDisplayMode() is called.

This CL:
- Makes sure that always the notification data is prepared first
  before the display mode is set. If the display mode change fails, the
  prepared notification data is discarded.
- Makes sure updating the displays doesn't happen synchronously
  in SetDisplayMode() when there is a resolution change.
- Fixes the crash when we revert to an old resolution for an already
  removed display.

BUG=701389,709722
TEST=manually
==========

to

==========
Fix errors in display resolution change notifications

We used to prepare the resolution change notification after we change
the resolution by setting the mode. This resulted in erronous messages
reporting wrong resolutions or even failures that didn't happen, when
updating the displays (which triggers the actual creation of the
notification) happens synchronously when SetDisplayMode() is called.

This CL:
- Makes sure that always the notification data is prepared first
  before the display mode is set. If the display mode change fails, the
  prepared notification data is discarded.
- Makes sure updating the displays doesn't happen synchronously
  in SetDisplayMode() when there is a resolution change.

BUG=701389       
TEST=manually
==========

[afakhry, 2017-04-13 01:25:02]

afakhry@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[afakhry, 2017-04-13 01:25:03]

Devlin for chrome/browser/extensions/display_info_provider_chromeos.cc. Thanks!

[Devlin, 2017-04-13 22:00:57]

chrome/browser/extensions/display_info_provider_chromeos.cc lgtm

[afakhry, 2017-04-13 22:31:37]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2017-04-13 22:31:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2799963003/#ps100001
(title: "Update commit message")

[commit-bot: I haz the power, 2017-04-13 22:32:51]

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-13 23:34:17]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492122697480440,
"parent_rev": "6fcee53ae0524e658915fbea9df843c1451e4c6e", "commit_rev":
"d5fd8d945a7e131d8dc9d64c6e6334f831d5cacc"}

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

Description was changed from

==========
Fix errors in display resolution change notifications

We used to prepare the resolution change notification after we change
the resolution by setting the mode. This resulted in erronous messages
reporting wrong resolutions or even failures that didn't happen, when
updating the displays (which triggers the actual creation of the
notification) happens synchronously when SetDisplayMode() is called.

This CL:
- Makes sure that always the notification data is prepared first
  before the display mode is set. If the display mode change fails, the
  prepared notification data is discarded.
- Makes sure updating the displays doesn't happen synchronously
  in SetDisplayMode() when there is a resolution change.

BUG=701389       
TEST=manually
==========

to

==========
Fix errors in display resolution change notifications

We used to prepare the resolution change notification after we change
the resolution by setting the mode. This resulted in erronous messages
reporting wrong resolutions or even failures that didn't happen, when
updating the displays (which triggers the actual creation of the
notification) happens synchronously when SetDisplayMode() is called.

This CL:
- Makes sure that always the notification data is prepared first
  before the display mode is set. If the display mode change fails, the
  prepared notification data is discarded.
- Makes sure updating the displays doesn't happen synchronously
  in SetDisplayMode() when there is a resolution change.

BUG=701389       
TEST=manually

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

[commit-bot: I haz the power, 2017-04-13 23:35:06]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d5fd8d945a7e131d8dc9d64c6e63...