Changes how to compute the id of a display.

Changes how to compute the id of a display.

Due to the discussion in crbug.com/240341, we prefer to use
the name field rather than the product_code field.

BUG=240341
R=oshima@chromium.org, marcheu@chromium.org
TEST=compilation passed

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

[oshima, 2013-05-20 20:29:39]

https://codereview.chromium.org/15368003/diff/1/chromeos/display/output_util.cc
File chromeos/display/output_util.cc (right):

https://codereview.chromium.org/15368003/diff/1/chromeos/display/output_util....
chromeos/display/output_util.cc:134: static_cast<uint32>(product_code) :
base::Hash(product_name);
May be we should mask the product_code with 0xFFFFC?
I think it's better to collide than having different code for the same display,
given that this code path should
be really rare. WDYT?

[marcheu, 2013-05-20 20:54:43]

https://codereview.chromium.org/15368003/diff/1/chromeos/display/output_util.cc
File chromeos/display/output_util.cc (right):

https://codereview.chromium.org/15368003/diff/1/chromeos/display/output_util....
chromeos/display/output_util.cc:134: static_cast<uint32>(product_code) :
base::Hash(product_name);
On 2013/05/20 20:29:40, oshima wrote:
> May be we should mask the product_code with 0xFFFFC?
> I think it's better to collide than having different code for the same
display,
> given that this code path should
> be really rare. WDYT?

I think it's fine either way. It's probably also fine (and lower impact) to
replace 9846 with 9845 when we encounter it...

[Jun Mukai, 2013-05-21 01:28:25]

https://codereview.chromium.org/15368003/diff/1/chromeos/display/output_util.cc
File chromeos/display/output_util.cc (right):

https://codereview.chromium.org/15368003/diff/1/chromeos/display/output_util....
chromeos/display/output_util.cc:134: static_cast<uint32>(product_code) :
base::Hash(product_name);
On 2013/05/20 20:54:43, marcheu wrote:
> On 2013/05/20 20:29:40, oshima wrote:
> > May be we should mask the product_code with 0xFFFFC?
> > I think it's better to collide than having different code for the same
> display,
> > given that this code path should
> > be really rare. WDYT?
> 
> I think it's fine either way. It's probably also fine (and lower impact) to
> replace 9846 with 9845 when we encounter it...

Hmm, I'm not sure what's the mask should be... is 0xFFFFFC good enough?  The
specific issue we've met is 9846/9845, but some other vendors may have some
other code pairs, right?
Indeed this is anyway really rare code path though... (as far as I see, only
internal displays don't have name fields).

[oshima, 2013-05-21 01:40:00]

https://codereview.chromium.org/15368003/diff/1/chromeos/display/output_util.cc
File chromeos/display/output_util.cc (right):

https://codereview.chromium.org/15368003/diff/1/chromeos/display/output_util....
chromeos/display/output_util.cc:134: static_cast<uint32>(product_code) :
base::Hash(product_name);
On 2013/05/21 01:28:25, Jun Mukai wrote:
> On 2013/05/20 20:54:43, marcheu wrote:
> > On 2013/05/20 20:29:40, oshima wrote:
> > > May be we should mask the product_code with 0xFFFFC?
> > > I think it's better to collide than having different code for the same
> > display,
> > > given that this code path should
> > > be really rare. WDYT?
> > 
> > I think it's fine either way. It's probably also fine (and lower impact) to
> > replace 9846 with 9845 when we encounter it...
> 
> Hmm, I'm not sure what's the mask should be... is 0xFFFFFC good enough?  The
> specific issue we've met is 9846/9845, but some other vendors may have some
> other code pairs, right?
> Indeed this is anyway really rare code path though... (as far as I see, only
> internal displays don't have name fields).

0xFFFC (sorry it was typo) produces 9844 for 9846/9845. I think
this is something we just need to address case by case
(there is no guarantee that name field doesn't change when a display
switches analog/display mode, for example). Since having
different ID for the same display is worse than having two
displays with the same ID, being conservative is better IMO.

That's being said, I don't feel so strongly about it, so existing
code is okay if you prefer.

[marcheu, 2013-05-21 01:42:34]

On 2013/05/21 01:28:25, Jun Mukai wrote:
>
https://codereview.chromium.org/15368003/diff/1/chromeos/display/output_util.cc
> File chromeos/display/output_util.cc (right):
> 
>
https://codereview.chromium.org/15368003/diff/1/chromeos/display/output_util....
> chromeos/display/output_util.cc:134: static_cast<uint32>(product_code) :
> base::Hash(product_name);
> On 2013/05/20 20:54:43, marcheu wrote:
> > On 2013/05/20 20:29:40, oshima wrote:
> > > May be we should mask the product_code with 0xFFFFC?
> > > I think it's better to collide than having different code for the same
> > display,
> > > given that this code path should
> > > be really rare. WDYT?
> > 
> > I think it's fine either way. It's probably also fine (and lower impact) to
> > replace 9846 with 9845 when we encounter it...
> 
> Hmm, I'm not sure what's the mask should be... is 0xFFFFFC good enough?  The
> specific issue we've met is 9846/9845, but some other vendors may have some
> other code pairs, right?

I don't know, I've never seen that before, so I'm not sure it's worth doing
something generic. Maybe it's worth adding a function for monitor-specific
quirks? With only one quirk for now...

> Indeed this is anyway really rare code path though... (as far as I see, only
> internal displays don't have name fields).

[Jun Mukai, 2013-05-21 21:19:10]

chatted with oshima offline, and I'm inclined to use a static fallback if the
product name string is missing.  Here is the justification:
- anyways it is a rare case: almost all displays have product name string
- even if a vendor is doing so, fallback to the same id is safer than to
different

wdyt?

Also fixed the code a bit for testing, and added test cases.

[oshima, 2013-05-21 21:53:31]

https://codereview.chromium.org/15368003/diff/12001/chromeos/display/output_u...
File chromeos/display/output_util.cc (right):

https://codereview.chromium.org/15368003/diff/12001/chromeos/display/output_u...
chromeos/display/output_util.cc:24: // product_name, and the index of the output
it is connected to.
keep this name consistent in the file, like product_name_code,
product_code_hash.

https://codereview.chromium.org/15368003/diff/12001/chromeos/display/output_u...
chromeos/display/output_util.cc:31: uint32 product_code,
ditto

https://codereview.chromium.org/15368003/diff/12001/chromeos/display/output_u...
chromeos/display/output_util.cc:151: // display name but distinguish them by
product code.
Actually, we can't use product code because we know it's not safe. It's better
to keep this way.

https://codereview.chromium.org/15368003/diff/12001/chromeos/display/output_u...
chromeos/display/output_util.cc:167: uint16* product_code,
and if that's the case, we no longer need to parse product_code?

[Jun Mukai, 2013-05-22 00:28:05]

https://codereview.chromium.org/15368003/diff/12001/chromeos/display/output_u...
File chromeos/display/output_util.cc (right):

https://codereview.chromium.org/15368003/diff/12001/chromeos/display/output_u...
chromeos/display/output_util.cc:24: // product_name, and the index of the output
it is connected to.
On 2013/05/21 21:53:31, oshima wrote:
> keep this name consistent in the file, like product_name_code,
> product_code_hash.

Done.

https://codereview.chromium.org/15368003/diff/12001/chromeos/display/output_u...
chromeos/display/output_util.cc:31: uint32 product_code,
On 2013/05/21 21:53:31, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/15368003/diff/12001/chromeos/display/output_u...
chromeos/display/output_util.cc:151: // display name but distinguish them by
product code.
On 2013/05/21 21:53:31, oshima wrote:
> Actually, we can't use product code because we know it's not safe. It's better
> to keep this way.

Done.

https://codereview.chromium.org/15368003/diff/12001/chromeos/display/output_u...
chromeos/display/output_util.cc:167: uint16* product_code,
On 2013/05/21 21:53:31, oshima wrote:
> and if that's the case, we no longer need to parse product_code?

Done.

[oshima, 2013-05-22 02:42:31]

lgtm

https://codereview.chromium.org/15368003/diff/17001/chromeos/display/output_u...
File chromeos/display/output_util.cc (right):

https://codereview.chromium.org/15368003/diff/17001/chromeos/display/output_u...
chromeos/display/output_util.cc:143: // Generates product specific value from
product_name. See crbug.com/240341
nit:
 .. from product_name instead of product_code.
would be better

[Jun Mukai, 2013-05-22 17:44:59]

https://codereview.chromium.org/15368003/diff/17001/chromeos/display/output_u...
File chromeos/display/output_util.cc (right):

https://codereview.chromium.org/15368003/diff/17001/chromeos/display/output_u...
chromeos/display/output_util.cc:143: // Generates product specific value from
product_name. See crbug.com/240341
On 2013/05/22 02:42:32, oshima wrote:
> nit:
>  .. from product_name instead of product_code.
> would be better

Done.

[commit-bot: I haz the power, 2013-05-22 17:45:26]

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

[commit-bot: I haz the power, 2013-05-23 11:50:00]

Change committed as 201756