Make RotationLockDefaultView inherit directly from ActionableView

Make RotationLockDefaultView inherit directly from ActionableView

RotationLockDefaultView was inheriting from TrayItemMore to be able to
use its layout. However, it did not use TrayItemMore's chevron or
detailed view transition. This patch changes RotationLockDefaultView to
inherit directly from ActionableView and use factory functions in
TrayPopupUtils to layout its contents.

This patch also fixes an issue with rotation lock row where its label
was not updated properly when rotation lock status was changed (e.g.
due to use of rotation shortcut).

This patch is a prerequisite for fixing correct theming and ripple
animation for non-clickable rows in the lock screen.

BUG=687689
TEST=TrayRotationLockTest.* in ash_unittests, manual

Review-Url: https://codereview.chromium.org/2696703005
Cr-Commit-Position: refs/heads/master@{#451139}
Committed: https://chromium.googlesource.com/chromium/src/+/39de08056d4d8cb6a4f2928b4c142f907c37e6b7

[mohsen, 2017-02-14 19:08:06]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 19:08:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2017-02-14 19:17:21]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 19:18:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2017-02-14 19:22:47]

mohsen@chromium.org changed reviewers:
+ stevenjb@chromium.org

[mohsen, 2017-02-14 19:22:48]

Please take a look...

[stevenjb, 2017-02-14 19:29:10]

stevenjb@chromium.org changed reviewers:
+ tdanderson@chromium.org

[stevenjb, 2017-02-14 19:29:11]

I am not the best person to review this any more.

tdanderson@ - Could you take a look, and why haven't we added you to
/ash/system/OWNERS yet?

cc:jamescook@

[commit-bot: I haz the power, 2017-02-14 20:07:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 20:07:45]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-02-14 20:43:25]

On 2017/02/14 19:29:11, stevenjb wrote:
> I am not the best person to review this any more.
> 
> tdanderson@ - Could you take a look, and why haven't we added you to
> /ash/system/OWNERS yet?
> 
> cc:jamescook@

tdanderson definitely deserves to be in //ash/system/OWNERS - that was an
oversight on my part. Sending https://codereview.chromium.org/2699493002 to add
him.

[mohsen, 2017-02-14 21:25:15]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 21:25:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2017-02-14 21:27:36]

stevenjb@: Thanks

tdanderson@: I'd missed one case when observing screen rotation controller which
is now fixed. Please take a look at the latest patch set...

[commit-bot: I haz the power, 2017-02-14 22:08:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 22:08:46]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2017-02-16 01:00:12]

Very nice, LGTM with one question/comment about observer removal.

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
File ash/system/chromeos/rotation/tray_rotation_lock.cc (left):

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
ash/system/chromeos/rotation/tray_rotation_lock.cc:188: if (!observing_shell_)
It seems that by removing these checks, your CL opens up the possibility of
attempting to remove the same observer twice (for instance, I think this can
happen on lines 168 and 193; are you sure you need both?) 

However it seems that RemoveObserver() is a no-op in such a case (i.e., doesn't
hit a DCHECK like in AddObserver()) but nevertheless it would be nice to remove
any calls that are never needed.

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
File ash/system/chromeos/rotation/tray_rotation_lock.cc (right):

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
ash/system/chromeos/rotation/tray_rotation_lock.cc:32: namespace {
nit: new line after lines 32 and 43

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
ash/system/chromeos/rotation/tray_rotation_lock.cc:44: }  // namespce
nit: 'namespace'

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
ash/system/chromeos/rotation/tray_rotation_lock.cc:59: // Stop observing
rotation locak status.
nit: 'lock'

[mohsen, 2017-02-16 04:14:09]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 04:15:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2017-02-16 04:15:17]

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
File ash/system/chromeos/rotation/tray_rotation_lock.cc (left):

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
ash/system/chromeos/rotation/tray_rotation_lock.cc:188: if (!observing_shell_)
On 2017/02/16 at 01:00:12, tdanderson wrote:
> It seems that by removing these checks, your CL opens up the possibility of
attempting to remove the same observer twice (for instance, I think this can
happen on lines 168 and 193; are you sure you need both?) 
> 
> However it seems that RemoveObserver() is a no-op in such a case (i.e.,
doesn't hit a DCHECK like in AddObserver()) but nevertheless it would be nice to
remove any calls that are never needed.

Yes, since the RemoveObserver() can handle multiple removals of an observer, the
manual bookkeeping is not needed here.

Both calls for removal of rotation observer (lines 188 and 192) are needed as
each of them can happen before/without the other one. For the shell observer, I
can't say for sure that both calls are needed, however, TrayRotationLockTest's
are failing. This might be because of how tests are set up, but, I'd rather stay
on the safe side and keep both calls.

WDYT?

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
File ash/system/chromeos/rotation/tray_rotation_lock.cc (right):

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
ash/system/chromeos/rotation/tray_rotation_lock.cc:32: namespace {
On 2017/02/16 at 01:00:12, tdanderson wrote:
> nit: new line after lines 32 and 43

Done.

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
ash/system/chromeos/rotation/tray_rotation_lock.cc:44: }  // namespce
On 2017/02/16 at 01:00:12, tdanderson wrote:
> nit: 'namespace'

Done.

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
ash/system/chromeos/rotation/tray_rotation_lock.cc:59: // Stop observing
rotation locak status.
On 2017/02/16 at 01:00:12, tdanderson wrote:
> nit: 'lock'

Done.

[commit-bot: I haz the power, 2017-02-16 06:16:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 06:16:37]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[tdanderson, 2017-02-16 19:20:59]

PS6 still LGTM

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
File ash/system/chromeos/rotation/tray_rotation_lock.cc (left):

https://codereview.chromium.org/2696703005/diff/50001/ash/system/chromeos/rot...
ash/system/chromeos/rotation/tray_rotation_lock.cc:188: if (!observing_shell_)
On 2017/02/16 04:15:17, mohsen wrote:
> On 2017/02/16 at 01:00:12, tdanderson wrote:
> > It seems that by removing these checks, your CL opens up the possibility of
> attempting to remove the same observer twice (for instance, I think this can
> happen on lines 168 and 193; are you sure you need both?) 
> > 
> > However it seems that RemoveObserver() is a no-op in such a case (i.e.,
> doesn't hit a DCHECK like in AddObserver()) but nevertheless it would be nice
to
> remove any calls that are never needed.
> 
> Yes, since the RemoveObserver() can handle multiple removals of an observer,
the
> manual bookkeeping is not needed here.
> 
> Both calls for removal of rotation observer (lines 188 and 192) are needed as
> each of them can happen before/without the other one. For the shell observer,
I
> can't say for sure that both calls are needed, however, TrayRotationLockTest's
> are failing. This might be because of how tests are set up, but, I'd rather
stay
> on the safe side and keep both calls.
> 
> WDYT?

Sounds fine by me, thanks.

[mohsen, 2017-02-16 20:39:59]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 20:41:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 22:00:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 22:00:29]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-02-16 22:01:35]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2017-02-16 22:01:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2696703005/#ps110001
(title: "Rebased")

[commit-bot: I haz the power, 2017-02-16 22:04:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 23:48:01]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1487282495360640,
"parent_rev": "3bc4fa6a738e393013822dbffaa25b02551803d3", "commit_rev":
"39de08056d4d8cb6a4f2928b4c142f907c37e6b7"}

[commit-bot: I haz the power, 2017-02-16 23:48:37]

Description was changed from

==========
Make RotationLockDefaultView inherit directly from ActionableView

RotationLockDefaultView was inheriting from TrayItemMore to be able to
use its layout. However, it did not use TrayItemMore's chevron or
detailed view transition. This patch changes RotationLockDefaultView to
inherit directly from ActionableView and use factory functions in
TrayPopupUtils to layout its contents.

This patch also fixes an issue with rotation lock row where its label
was not updated properly when rotation lock status was changed (e.g.
due to use of rotation shortcut).

This patch is a prerequisite for fixing correct theming and ripple
animation for non-clickable rows in the lock screen.

BUG=687689
TEST=TrayRotationLockTest.* in ash_unittests, manual
==========

to

==========
Make RotationLockDefaultView inherit directly from ActionableView

RotationLockDefaultView was inheriting from TrayItemMore to be able to
use its layout. However, it did not use TrayItemMore's chevron or
detailed view transition. This patch changes RotationLockDefaultView to
inherit directly from ActionableView and use factory functions in
TrayPopupUtils to layout its contents.

This patch also fixes an issue with rotation lock row where its label
was not updated properly when rotation lock status was changed (e.g.
due to use of rotation shortcut).

This patch is a prerequisite for fixing correct theming and ripple
animation for non-clickable rows in the lock screen.

BUG=687689
TEST=TrayRotationLockTest.* in ash_unittests, manual

Review-Url: https://codereview.chromium.org/2696703005
Cr-Commit-Position: refs/heads/master@{#451139}
Committed:
https://chromium.googlesource.com/chromium/src/+/39de08056d4d8cb6a4f2928b4c14...
==========

[commit-bot: I haz the power, 2017-02-16 23:48:38]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/39de08056d4d8cb6a4f2928b4c14...