Introduces 'highlighted' property to candidate view.

Introduces 'highlighted' property to candidate view.

With my previous 'cleanup', the candidate view becomes a CustomButton
and the 'PRESSED' button status indicates the highlighted. I thought
this is good enough, but hovering also changes the button state.

Using a button state is a bad idea, so this CL adds a boolean flag
for highlighted status.

BUG=365959
R=oshima@chromium.org
TEST=manually, ash_unittests

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

[oshima, 2014-04-23 23:50:07]

https://codereview.chromium.org/258463002/diff/1/ash/ime/candidate_view.cc
File ash/ime/candidate_view.cc (right):

https://codereview.chromium.org/258463002/diff/1/ash/ime/candidate_view.cc#ne...
ash/ime/candidate_view.cc:225: SetHighlighted(true);
shouldn't this be 

SetHighlighted(state() == STATE_PRESSED)

?

[Jun Mukai, 2014-04-23 23:58:02]

https://codereview.chromium.org/258463002/diff/1/ash/ime/candidate_view.cc
File ash/ime/candidate_view.cc (right):

https://codereview.chromium.org/258463002/diff/1/ash/ime/candidate_view.cc#ne...
ash/ime/candidate_view.cc:225: SetHighlighted(true);
On 2014/04/23 23:50:07, oshima wrote:
> shouldn't this be 
> 
> SetHighlighted(state() == STATE_PRESSED)
> 
> ?

No, state() can be changed to 'HOVER' and then 'NORMAL' for mouse hovering. We
don't want to set the existing highlight to false in that case.

[oshima, 2014-04-24 00:05:37]

https://codereview.chromium.org/258463002/diff/1/ash/ime/candidate_view.cc
File ash/ime/candidate_view.cc (right):

https://codereview.chromium.org/258463002/diff/1/ash/ime/candidate_view.cc#ne...
ash/ime/candidate_view.cc:225: SetHighlighted(true);
On 2014/04/23 23:58:02, Jun Mukai wrote:
> On 2014/04/23 23:50:07, oshima wrote:
> > shouldn't this be 
> > 
> > SetHighlighted(state() == STATE_PRESSED)
> > 
> > ?
> 
> No, state() can be changed to 'HOVER' and then 'NORMAL' for mouse hovering. We
> don't want to set the existing highlight to false in that case.

Ah i see. Can you add test for this? It looks subtle and can be fragile to
changes.

[Jun Mukai, 2014-04-24 19:01:39]

https://codereview.chromium.org/258463002/diff/1/ash/ime/candidate_view.cc
File ash/ime/candidate_view.cc (right):

https://codereview.chromium.org/258463002/diff/1/ash/ime/candidate_view.cc#ne...
ash/ime/candidate_view.cc:225: SetHighlighted(true);
On 2014/04/24 00:05:37, oshima wrote:
> On 2014/04/23 23:58:02, Jun Mukai wrote:
> > On 2014/04/23 23:50:07, oshima wrote:
> > > shouldn't this be 
> > > 
> > > SetHighlighted(state() == STATE_PRESSED)
> > > 
> > > ?
> > 
> > No, state() can be changed to 'HOVER' and then 'NORMAL' for mouse hovering.
We
> > don't want to set the existing highlight to false in that case.
> 
> Ah i see. Can you add test for this? It looks subtle and can be fragile to
> changes.

Done.

[oshima, 2014-04-24 20:54:36]

lgtm with nits

https://codereview.chromium.org/258463002/diff/10001/ash/ime/candidate_view_u...
File ash/ime/candidate_view_unittest.cc (right):

https://codereview.chromium.org/258463002/diff/10001/ash/ime/candidate_view_u...
ash/ime/candidate_view_unittest.cc:21: const char* kDummyCandidates[] = {
nit: this adds static initializer.

const char* const kDummyCandidates[] ...

https://codereview.chromium.org/258463002/diff/10001/ash/ime/candidate_view_u...
ash/ime/candidate_view_unittest.cc:75: int GetHighlightedIndex(int*
highlighted_count) {
const {

https://codereview.chromium.org/258463002/diff/10001/ash/ime/candidate_view_u...
ash/ime/candidate_view_unittest.cc:95: LOG(ERROR) << "ButtonPressed is called
for non candidates";
Shouldn't this be ASSERT if it's error?

[Jun Mukai, 2014-04-24 21:14:22]

https://codereview.chromium.org/258463002/diff/10001/ash/ime/candidate_view_u...
File ash/ime/candidate_view_unittest.cc (right):

https://codereview.chromium.org/258463002/diff/10001/ash/ime/candidate_view_u...
ash/ime/candidate_view_unittest.cc:21: const char* kDummyCandidates[] = {
On 2014/04/24 20:54:37, oshima wrote:
> nit: this adds static initializer.
> 
> const char* const kDummyCandidates[] ...

Done.

https://codereview.chromium.org/258463002/diff/10001/ash/ime/candidate_view_u...
ash/ime/candidate_view_unittest.cc:75: int GetHighlightedIndex(int*
highlighted_count) {
On 2014/04/24 20:54:37, oshima wrote:
> const {

Done.

https://codereview.chromium.org/258463002/diff/10001/ash/ime/candidate_view_u...
ash/ime/candidate_view_unittest.cc:95: LOG(ERROR) << "ButtonPressed is called
for non candidates";
On 2014/04/24 20:54:37, oshima wrote:
> Shouldn't this be ASSERT if it's error?

ASSERT requires 'void' argument. Use DCHECK instead.

[Jun Mukai, 2014-04-24 21:14:40]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-04-24 21:45:09]

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

[commit-bot: I haz the power, 2014-04-25 03:37:46]

Change committed as 266123