Listen to Display reconfiguration and notify DisplayObservers on Mac.

Listen to Display reconfiguration and notify DisplayObservers on Mac.

This is including some refactoring with the DestopScreenX11 implementation.

TBR=sky@chromium.org (for BUILD.gn changes because brettw@ doens't formally own it)

BUG=122863, 162827
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284227

[mlamouri (slow - plz ping), 2014-06-24 18:39:46]

erg@ and oshima@, could you have a look at this?

[mlamouri (slow - plz ping), 2014-06-24 18:41:03]

On 2014/06/24 18:39:46, Mounir Lamouri wrote:
> erg@ and oshima@, could you have a look at this?

Note: I will have to wait until tomorrow morning to try the code on a real
machine.

[mlamouri (slow - plz ping), 2014-06-25 13:18:16]

I forgot to plug things up to the RWHVMac. This is now done and tested on a real
machine.

PTAL.

[Elliot Glaysher, 2014-06-26 18:11:43]

Please note that when you add multiple people to a code review, this usually
just means that each person ends up assuming the other person will do the
review.

If you have to have multiple people on a review, mentioning something like "erg:
c/b/u/v, oshima: ui/" is really helpful in getting a response. As is, I don't
know what you want each of us to review. (And am unsure what code here I'm
supposed to know about in this patch.)

[mlamouri (slow - plz ping), 2014-06-26 22:21:40]

On 2014/06/26 at 18:11:43, erg wrote:
> Please note that when you add multiple people to a code review, this usually
just means that each person ends up assuming the other person will do the
review.
> 
> If you have to have multiple people on a review, mentioning something like
"erg: c/b/u/v, oshima: ui/" is really helpful in getting a response. As is, I
don't know what you want each of us to review. (And am unsure what code here I'm
supposed to know about in this patch.)

Oh, sorry about that. I usually do that but for some reason, I forgot here.

erg@, could you have a look at ui/views/* changes and a quick look at
ui/gfx/screen_display_observer_delegate.*
oshima@, could you have a look at the changes in ui/gfx/*.

[Elliot Glaysher, 2014-06-27 18:08:47]

It looks like the part you want me to review just moves stuff out.

lgtm if oshima is fine with it.

[mlamouri (slow - plz ping), 2014-07-01 14:18:13]

On 2014/06/27 at 18:08:47, erg wrote:
> It looks like the part you want me to review just moves stuff out.
> 
> lgtm if oshima is fine with it.

oshima@, can you have a look at this? :)

[oshima, 2014-07-08 20:41:33]

I'll review this by EOD. (sorry I was OOO last week)

[oshima, 2014-07-09 17:12:51]

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_display_ob...
File ui/gfx/screen_display_observer_delegate.cc (right):

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_display_ob...
ui/gfx/screen_display_observer_delegate.cc:29: bool found = false;
can you use std::find_if ? I usualyl prefer loop over std::find, but I think
this method will be simpler with std::find.

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_display_ob...
File ui/gfx/screen_display_observer_delegate.h (right):

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_display_ob...
ui/gfx/screen_display_observer_delegate.h:15: class
ScreenDisplayObserverDelegate {
DisplayChangeNotifier would be better name. The name "Delegate" in chromium is
used for a class that uses delegate pattern.

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_display_ob...
ui/gfx/screen_display_observer_delegate.h:31: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_mac.mm
File ui/gfx/screen_mac.mm (right):

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_mac.mm#new...
ui/gfx/screen_mac.mm:196: configure_timer_.reset(new
base::OneShotTimer<ScreenMac>());
Any reason not to use RepeatingTimer?

[mlamouri (slow - plz ping), 2014-07-10 12:54:34]

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_display_ob...
File ui/gfx/screen_display_observer_delegate.cc (right):

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_display_ob...
ui/gfx/screen_display_observer_delegate.cc:29: bool found = false;
On 2014/07/09 17:12:51, oshima wrote:
> can you use std::find_if ? I usualyl prefer loop over std::find, but I think
> this method will be simpler with std::find.

Unfortunately, std::find() will compare the items with a given value, in that
case, it would check if the displays are equal. I'm checking if there ids are
equal so it wouldn't work that well unless we add an operator== on display using
the id... I'm not sure that would be a great idea.

An alternative would be to use std::find_if but in the current state of C++ in
Chromium, we might not be able to do it in a nicer way that this loop (ie. we
can't use anonymous functions).

Let me know if I misunderstood your comment.

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_display_ob...
File ui/gfx/screen_display_observer_delegate.h (right):

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_display_ob...
ui/gfx/screen_display_observer_delegate.h:15: class
ScreenDisplayObserverDelegate {
On 2014/07/09 17:12:51, oshima wrote:
> DisplayChangeNotifier would be better name. The name "Delegate" in chromium is
> used for a class that uses delegate pattern.

Done.

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_display_ob...
ui/gfx/screen_display_observer_delegate.h:31: };
On 2014/07/09 17:12:51, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_mac.mm
File ui/gfx/screen_mac.mm (right):

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_mac.mm#new...
ui/gfx/screen_mac.mm:196: configure_timer_.reset(new
base::OneShotTimer<ScreenMac>());
On 2014/07/09 17:12:51, oshima wrote:
> Any reason not to use RepeatingTimer?

There is no repeating pattern here. The timer is only used to coalesce events
(see comment) because HandleDisplayReconfiguration will be called multiple times
and we are reading the new values to detect what has changed, we want to make
sure that we do so only when all the calls went trough. The timer allow use to
do that. It is not polling, which is what RepeatingTimer would do, right?

[mlamouri (slow - plz ping), 2014-07-10 12:57:58]

jam@chromium.org: Please review changes in:
 +content/browser/renderer_host/render_widget_host_view_mac.*

[oshima, 2014-07-10 18:11:58]

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_display_ob...
File ui/gfx/screen_display_observer_delegate.cc (right):

https://codereview.chromium.org/341983008/diff/70001/ui/gfx/screen_display_ob...
ui/gfx/screen_display_observer_delegate.cc:29: bool found = false;
On 2014/07/10 12:54:34, Mounir Lamouri wrote:
> On 2014/07/09 17:12:51, oshima wrote:
> > can you use std::find_if ? I usualyl prefer loop over std::find, but I think
> > this method will be simpler with std::find.
> 
> Unfortunately, std::find() will compare the items with a given value, in that
> case, it would check if the displays are equal. I'm checking if there ids are
> equal so it wouldn't work that well unless we add an operator== on display
using
> the id... I'm not sure that would be a great idea.
> 
> An alternative would be to use std::find_if but in the current state of C++ in
> Chromium, we might not be able to do it in a nicer way that this loop (ie. we
> can't use anonymous functions).
> 
> Let me know if I misunderstood your comment.

Yes, I meant std:find_if (sorry I mentioned find_if, but omitted _if for the
rest of my comment)

https://codereview.chromium.org/382833002/diff/20001/ui/gfx/display_change_no...

This is easier to read IMO.

Can you also add unit test for notifier?

https://codereview.chromium.org/341983008/diff/90001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/341983008/diff/90001/ui/gfx/gfx.gyp#newcode258
ui/gfx/gfx.gyp:258: 'screen_display_observer_delegate.h',
this needs to be updated.

[mlamouri (slow - plz ping), 2014-07-11 13:47:43]

I've added some unit tests and did various cleanups.

PTAL.

https://codereview.chromium.org/341983008/diff/90001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/341983008/diff/90001/ui/gfx/gfx.gyp#newcode258
ui/gfx/gfx.gyp:258: 'screen_display_observer_delegate.h',
On 2014/07/10 18:11:58, oshima wrote:
> this needs to be updated.

Done.

[oshima, 2014-07-11 19:13:25]

lg just a few nits

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
File ui/gfx/display_change_notifier.cc (right):

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
ui/gfx/display_change_notifier.cc:25: const Display& display_;
It's error phone to have reference in member. Just use Display (or id)

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
File ui/gfx/display_change_notifier_unittest.cc (right):

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
ui/gfx/display_change_notifier_unittest.cc:20: #endif // 0
remove this?

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
ui/gfx/display_change_notifier_unittest.cc:26: , display_removed_(0)
move ","s to previous line. Or you can just run "git cl format" and
I won't complain style nits :)

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
ui/gfx/display_change_notifier_unittest.cc:66: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
ui/gfx/display_change_notifier_unittest.cc:68: namespace {
Don't put tests in anonymous namespace because tests names should be unique (as
you should be able to specify using --gtest_filter=).

Just put it on gfx (am also fine to put it in global ns).

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
ui/gfx/display_change_notifier_unittest.cc:97: "Observers can only be added
once!");
Null/Twice/Unknowns are covered by ObserverList's test so it's not necessary. I
don't mind keeping them if you want though.

[mlamouri (slow - plz ping), 2014-07-13 15:20:40]

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
File ui/gfx/display_change_notifier.cc (right):

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
ui/gfx/display_change_notifier.cc:25: const Display& display_;
On 2014/07/11 19:13:25, oshima wrote:
> It's error phone to have reference in member. Just use Display (or id)

Done.

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
File ui/gfx/display_change_notifier_unittest.cc (right):

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
ui/gfx/display_change_notifier_unittest.cc:20: #endif // 0
On 2014/07/11 19:13:25, oshima wrote:
> remove this?

Done.

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
ui/gfx/display_change_notifier_unittest.cc:26: , display_removed_(0)
On 2014/07/11 19:13:25, oshima wrote:
> move ","s to previous line. Or you can just run "git cl format" and
> I won't complain style nits :)

Done.

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
ui/gfx/display_change_notifier_unittest.cc:66: };
On 2014/07/11 19:13:25, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
ui/gfx/display_change_notifier_unittest.cc:68: namespace {
On 2014/07/11 19:13:25, oshima wrote:
> Don't put tests in anonymous namespace because tests names should be unique
(as
> you should be able to specify using --gtest_filter=).
> 
> Just put it on gfx (am also fine to put it in global ns).

Done.

https://codereview.chromium.org/341983008/diff/130001/ui/gfx/display_change_n...
ui/gfx/display_change_notifier_unittest.cc:97: "Observers can only be added
once!");
On 2014/07/11 19:13:25, oshima wrote:
> Null/Twice/Unknowns are covered by ObserverList's test so it's not necessary.
I
> don't mind keeping them if you want though.

ObserverList's tests should indeed cover that but DisplayChangeNotifier's
consumers might rely on such behaviour and ObserverList's behaviour might
change. These tests don't cost us and have the potential to trigger real
problems I think.

[mlamouri (slow - plz ping), 2014-07-13 15:28:17]

brettw@, could you review the BUILD.gn change?

[oshima, 2014-07-14 14:37:00]

lgtm

[brettw, 2014-07-14 19:41:19]

BUILD.gn LGTM. Feel free to add/remove files from lists in .gn files without
getting a separate GN review.

[jam, 2014-07-15 00:42:36]

On 2014/07/10 12:57:58, Mounir Lamouri wrote:
> mailto:jam@chromium.org: Please review changes in:
>  +content/browser/renderer_host/render_widget_host_view_mac.*

I'm not familiar at all with the mac code, so I'm not a good reviewer. I would
have put Avi since he's an owner, but since he's ooo, how about you get a review
from a person familiar with Mac and then I can rubberstamp (IM me)

[mlamouri (slow - plz ping), 2014-07-15 12:24:36]

I don't think the content/browser/ changes are that much Cocoa specific.
oshima@, would you be comfortable reviewing them so jam@ can rubber-stamp?

[oshima, 2014-07-15 16:54:41]

On 2014/07/15 12:24:36, Mounir Lamouri wrote:
> I don't think the content/browser/ changes are that much Cocoa specific.
> oshima@, would you be comfortable reviewing them so jam@ can rubber-stamp?

LGTM. The change in RWHVMac does the same thing as RWHVAura does to update the
ScreenInfo when the display metrics change.

[mlamouri (slow - plz ping), 2014-07-16 11:25:24]

On 2014/07/15 16:54:41, oshima wrote:
> On 2014/07/15 12:24:36, Mounir Lamouri wrote:
> > I don't think the content/browser/ changes are that much Cocoa specific.
> > oshima@, would you be comfortable reviewing them so jam@ can rubber-stamp?
> 
> LGTM. The change in RWHVMac does the same thing as RWHVAura does to update the
> ScreenInfo when the display metrics change.

jam@, PTAL?

[jam, 2014-07-16 18:28:53]

lgtm

[mlamouri (slow - plz ping), 2014-07-18 14:56:34]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-07-18 14:57:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/341983008/290001

[mlamouri (slow - plz ping), 2014-07-18 15:28:40]

The CQ bit was unchecked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-07-18 15:28:41]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-07-18 15:29:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/341983008/290001

[commit-bot: I haz the power, 2014-07-18 22:36:15]

Change committed as 284227