Implement GetAllDisplays() for the Mac screen backend

Implement GetAllDisplays() for the Mac screen backend

Reusing the details of the GetNumDisplays() implementation for returning
a vector of available displays. Reusing the existing code for actually
generating gfx::Display objects by converting to NSScreen and then
calling GetDisplayForScreen to be consistent with the behavior in other
parts of this file.

BUG=311132
R=oshima@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232955

[oshima, 2013-10-24 15:09:33]

nico should review mac impl.

[Nico, 2013-10-24 15:41:27]

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

https://codereview.chromium.org/40163002/diff/30001/ui/gfx/screen_mac.mm#newc...
ui/gfx/screen_mac.mm:131: NSDictionary* screenDeviceDescription = [screen
deviceDescription];
Can you build an id -> NSScreen map before this loop and then only do a lookup
here?

[Dominik Röttsches, 2013-10-25 13:18:43]

Thanks for your review, Nico.

On 2013/10/24 15:41:27, Nico wrote:
> https://codereview.chromium.org/40163002/diff/30001/ui/gfx/screen_mac.mm
> File ui/gfx/screen_mac.mm (right):
> 
>
https://codereview.chromium.org/40163002/diff/30001/ui/gfx/screen_mac.mm#newc...
> ui/gfx/screen_mac.mm:131: NSDictionary* screenDeviceDescription = [screen
> deviceDescription];
> Can you build an id -> NSScreen map before this loop and then only do a lookup
> here?

Patch updated according to your suggestion.

[Nico, 2013-10-25 18:16:48]

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

https://codereview.chromium.org/40163002/diff/110001/ui/gfx/screen_mac.mm#new...
ui/gfx/screen_mac.mm:122: typedef std::map<int64, NSScreen*>
screenIdsToScreensMap;
types start with capital letters in chromium style. (or don't use a typedef at
all)

https://codereview.chromium.org/40163002/diff/110001/ui/gfx/screen_mac.mm#new...
ui/gfx/screen_mac.mm:142: if (!(foundScreen == screenIdsToScreens.end())) {
You can write !(a == b) as a != b

https://codereview.chromium.org/40163002/diff/110001/ui/gfx/screen_mac.mm#new...
ui/gfx/screen_mac.mm:143:
displays.push_back(GetDisplayForScreen(foundScreen->second, false));
Won't one of these be the primary display? (see GetDisplayNearestPoint() for
similar code which does check this)

[Dominik Röttsches, 2013-10-30 16:47:23]

On 2013/10/25 18:16:48, Nico wrote:

>
https://codereview.chromium.org/40163002/diff/110001/ui/gfx/screen_mac.mm#new...
> ui/gfx/screen_mac.mm:143:
> displays.push_back(GetDisplayForScreen(foundScreen->second, false));
> Won't one of these be the primary display? (see GetDisplayNearestPoint() for
> similar code which does check this)

Yes - I find the usage of this boolean parameter a bit inconsistent in that
file, hence I suggest the following refactoring:
https://codereview.chromium.org/47073006 - if you're okay with that, I'll update
this patch after merging the other change.

[Nico, 2013-10-30 17:07:01]

Sounds good, ping this once it's rebased. Thanks!

On Wed, Oct 30, 2013 at 9:47 AM, <dominik.rottsches@intel.com> wrote:

> On 2013/10/25 18:16:48, Nico wrote:
>
>
> https://codereview.chromium.**org/40163002/diff/110001/ui/**
>
gfx/screen_mac.mm#newcode143<https://codereview.chromium.org/40163002/diff/110001/ui/gfx/screen_mac.mm#newcode143>
>
>> ui/gfx/screen_mac.mm:143:
>> displays.push_back(**GetDisplayForScreen(**foundScreen->second, false));
>> Won't one of these be the primary display? (see GetDisplayNearestPoint()
>> for
>> similar code which does check this)
>>
>
> Yes - I find the usage of this boolean parameter a bit inconsistent in that
> file, hence I suggest the following refactoring:
>
https://codereview.chromium.**org/47073006<https://codereview.chromium.org/47...
if you're okay with that, I'll update
> this patch after merging the other change.
>
>
>
https://codereview.chromium.**org/40163002/<https://codereview.chromium.org/4...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dominik Röttsches, 2013-11-01 13:33:09]

On 2013/10/30 17:07:01, Nico wrote:
> Sounds good, ping this once it's rebased. Thanks!

Ready for another round, rebased on top of
https://codereview.chromium.org/47073006

[Nico, 2013-11-01 21:45:47]

lgtm with style issues fixed

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

https://codereview.chromium.org/40163002/diff/180001/ui/gfx/screen_mac.mm#new...
ui/gfx/screen_mac.mm:124: ScreenIdsToScreensMap screenIdsToScreens;
name_variables_like_this_in_chromium_style in c++ functions
(nameThemLikeThisInObjCThoughYesItsABitOfAMess)

(yes, mouseLocation further up is wrong too)

https://codereview.chromium.org/40163002/diff/180001/ui/gfx/screen_mac.mm#new...
ui/gfx/screen_mac.mm:126: NSDictionary* screenDeviceDescription = [screen
deviceDescription];
screen_device_description

https://codereview.chromium.org/40163002/diff/180001/ui/gfx/screen_mac.mm#new...
ui/gfx/screen_mac.mm:127: int64 screenId = [[screenDeviceDescription
screen_id

[commit-bot: I haz the power, 2013-11-04 10:35:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/40163002/2...

[commit-bot: I haz the power, 2013-11-04 12:52:43]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-11-04 12:55:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/40163002/2...

[commit-bot: I haz the power, 2013-11-04 16:10:00]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-11-05 09:02:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/40163002/2...

[commit-bot: I haz the power, 2013-11-05 13:33:45]

Change committed as 232955