Caches the display names in MultiDisplayManager.

Caches the display names in MultiDisplayManager.


BUG=159034
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165775

[oshima, 2012-11-02 06:33:56]

http://codereview.chromium.org/11369042/diff/1/ash/display/multi_display_mana...
File ash/display/multi_display_manager.cc (right):

http://codereview.chromium.org/11369042/diff/1/ash/display/multi_display_mana...
ash/display/multi_display_manager.cc:556: std::string result;
this isn't used?

http://codereview.chromium.org/11369042/diff/1/ash/display/multi_display_mana...
ash/display/multi_display_manager.cc:567: if (!HasInternalDisplay() ||
!IsInternalDisplayId(id))
is it simpler to set inernal display's name here rather than checking here and
GetDisplayNameFor?

http://codereview.chromium.org/11369042/diff/1/ash/system/chromeos/tray_displ...
File ash/system/chromeos/tray_display.cc (right):

http://codereview.chromium.org/11369042/diff/1/ash/system/chromeos/tray_displ...
ash/system/chromeos/tray_display.cc:71: SetVisible(false);
This state shouldn't happen, but if we ever failed to get
the name, we still should show this menu with something like
"Unknown Display" rather than hiding it.

[Jun Mukai, 2012-11-02 16:09:07]

https://codereview.chromium.org/11369042/diff/1/ash/display/multi_display_man...
File ash/display/multi_display_manager.cc (right):

https://codereview.chromium.org/11369042/diff/1/ash/display/multi_display_man...
ash/display/multi_display_manager.cc:556: std::string result;
On 2012/11/02 06:33:56, oshima wrote:
> this isn't used?

Done.

https://codereview.chromium.org/11369042/diff/1/ash/display/multi_display_man...
ash/display/multi_display_manager.cc:567: if (!HasInternalDisplay() ||
!IsInternalDisplayId(id))
On 2012/11/02 06:33:56, oshima wrote:
> is it simpler to set inernal display's name here rather than checking here and
> GetDisplayNameFor?

Done.

https://codereview.chromium.org/11369042/diff/1/ash/system/chromeos/tray_disp...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/11369042/diff/1/ash/system/chromeos/tray_disp...
ash/system/chromeos/tray_display.cc:71: SetVisible(false);
On 2012/11/02 06:33:56, oshima wrote:
> This state shouldn't happen, but if we ever failed to get
> the name, we still should show this menu with something like
> "Unknown Display" rather than hiding it.

Done.

[oshima, 2012-11-02 16:35:11]

lgtm with a nit

https://codereview.chromium.org/11369042/diff/5001/ash/display/multi_display_...
File ash/display/multi_display_manager.cc (right):

https://codereview.chromium.org/11369042/diff/5001/ash/display/multi_display_...
ash/display/multi_display_manager.cc:561: if (HasInternalDisplay() &&
IsInternalDisplayId(display.id())) {
IsInternalDisplayId is enough, i think?

[Jun Mukai, 2012-11-02 16:37:53]

https://codereview.chromium.org/11369042/diff/5001/ash/display/multi_display_...
File ash/display/multi_display_manager.cc (right):

https://codereview.chromium.org/11369042/diff/5001/ash/display/multi_display_...
ash/display/multi_display_manager.cc:561: if (HasInternalDisplay() &&
IsInternalDisplayId(display.id())) {
On 2012/11/02 16:35:11, oshima wrote:
> IsInternalDisplayId is enough, i think?

Done.

[Jun Mukai, 2012-11-02 16:38:43]

derat, can you review this?

[Daniel Erat, 2012-11-02 16:46:16]

https://codereview.chromium.org/11369042/diff/10001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/11369042/diff/10001/ash/ash_strings.grd#newco...
ash/ash_strings.grd:352: <message
name="IDS_ASH_STATUS_TRAY_UNKNOWN_DISPLAY_NAME" desc="The name of the display
used in the tray to show the current status but the system failed to obtain the
name.">
nit: "Label shown in tray for a display whose name is unknown."

https://codereview.chromium.org/11369042/diff/10001/ash/display/multi_display...
File ash/display/multi_display_manager.cc (right):

https://codereview.chromium.org/11369042/diff/10001/ash/display/multi_display...
ash/display/multi_display_manager.cc:132: std::vector<std::string>
MultiDisplayManager::GetExternalDisplayNames() const {
this method seems a bit tricky; it returns an ordered list of names but skips
over the internal display.  is there any way that you could change the caller so
it just passes the external display that it cares about to GetDisplayNameFor()
instead?

https://codereview.chromium.org/11369042/diff/10001/ash/display/multi_display...
ash/display/multi_display_manager.cc:387: std::string
MultiDisplayManager::GetDisplayNameFor(
nit (doesn't need to be addressed here): putting "For" on the end of this method
name is pretty uncommon in the chrome source that i've seen.  i'd rename this to
either GetDisplayName() or GetNameForDisplay()

https://codereview.chromium.org/11369042/diff/10001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/11369042/diff/10001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:65: string16 display_name =
(!display_names.empty()) ?
nit: remove extra parentheses around !display_names.empty()

[Jun Mukai, 2012-11-02 18:04:35]

https://codereview.chromium.org/11369042/diff/10001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/11369042/diff/10001/ash/ash_strings.grd#newco...
ash/ash_strings.grd:352: <message
name="IDS_ASH_STATUS_TRAY_UNKNOWN_DISPLAY_NAME" desc="The name of the display
used in the tray to show the current status but the system failed to obtain the
name.">
On 2012/11/02 16:46:17, Daniel Erat wrote:
> nit: "Label shown in tray for a display whose name is unknown."

Done.

https://codereview.chromium.org/11369042/diff/10001/ash/display/multi_display...
File ash/display/multi_display_manager.cc (right):

https://codereview.chromium.org/11369042/diff/10001/ash/display/multi_display...
ash/display/multi_display_manager.cc:132: std::vector<std::string>
MultiDisplayManager::GetExternalDisplayNames() const {
On 2012/11/02 16:46:17, Daniel Erat wrote:
> this method seems a bit tricky; it returns an ordered list of names but skips
> over the internal display.  is there any way that you could change the caller
so
> it just passes the external display that it cares about to GetDisplayNameFor()
> instead?

Thanks for the comment.  Removed this method and changed the code structure a
bit.

https://codereview.chromium.org/11369042/diff/10001/ash/display/multi_display...
ash/display/multi_display_manager.cc:387: std::string
MultiDisplayManager::GetDisplayNameFor(
On 2012/11/02 16:46:17, Daniel Erat wrote:
> nit (doesn't need to be addressed here): putting "For" on the end of this
method
> name is pretty uncommon in the chrome source that i've seen.  i'd rename this
to
> either GetDisplayName() or GetNameForDisplay()

Can we do that in another CL?  This is used in many places.

https://codereview.chromium.org/11369042/diff/10001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/11369042/diff/10001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:65: string16 display_name =
(!display_names.empty()) ?
On 2012/11/02 16:46:17, Daniel Erat wrote:
> nit: remove extra parentheses around !display_names.empty()

Done.

[Daniel Erat, 2012-11-02 18:59:04]

lgtm

https://codereview.chromium.org/11369042/diff/10001/ash/display/multi_display...
File ash/display/multi_display_manager.cc (right):

https://codereview.chromium.org/11369042/diff/10001/ash/display/multi_display...
ash/display/multi_display_manager.cc:387: std::string
MultiDisplayManager::GetDisplayNameFor(
On 2012/11/02 18:04:35, Jun Mukai wrote:
> On 2012/11/02 16:46:17, Daniel Erat wrote:
> > nit (doesn't need to be addressed here): putting "For" on the end of this
> method
> > name is pretty uncommon in the chrome source that i've seen.  i'd rename
this
> to
> > either GetDisplayName() or GetNameForDisplay()
> 
> Can we do that in another CL?  This is used in many places.

Sure, sounds fine.

[commit-bot: I haz the power, 2012-11-02 20:33:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/11369042/5

[commit-bot: I haz the power, 2012-11-02 22:26:54]

Change committed as 165775