Factor out activation tracking code in AppListControllerWin.

Factor out activation tracking code in AppListControllerWin.

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

[benwells, 2013-08-08 22:11:15]

https://codereview.chromium.org/22488007/diff/1/chrome/browser/ui/views/app_l...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/22488007/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:315: void
CheckTaskbarOrViewHasFocus() {
It would be great to factorize this monster. Although looking more closely it is
mostly comments :-/

https://codereview.chromium.org/22488007/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:503:
base::RepeatingTimer<AppListController> timer_;
Can this be removed now?

https://codereview.chromium.org/22488007/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:689:
activation_tracker_.reset(new ActivationTracker(current_view_,
Hmm, pity this guy's lifetime isn't managed by the view but I can't think of a
nice way to do that. Did you consider always having a tracker but updating its
view pointer? You could probably just update it in OnActivationChanged.

This code now has a few assumptions about when there will be a valid
activation_tracker_.

[koz (OOO until 15th September), 2013-08-08 23:42:25]

https://codereview.chromium.org/22488007/diff/1/chrome/browser/ui/views/app_l...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/22488007/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:315: void
CheckTaskbarOrViewHasFocus() {
On 2013/08/08 22:11:15, benwells wrote:
> It would be great to factorize this monster. Although looking more closely it
is
> mostly comments :-/

There might be a way to break it up, but I'm happy to just separate it out for
now.

https://codereview.chromium.org/22488007/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:503:
base::RepeatingTimer<AppListController> timer_;
On 2013/08/08 22:11:15, benwells wrote:
> Can this be removed now?

Done.

https://codereview.chromium.org/22488007/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:689:
activation_tracker_.reset(new ActivationTracker(current_view_,
On 2013/08/08 22:11:15, benwells wrote:
> Hmm, pity this guy's lifetime isn't managed by the view but I can't think of a
> nice way to do that. Did you consider always having a tracker but updating its
> view pointer? You could probably just update it in OnActivationChanged.
> 
> This code now has a few assumptions about when there will be a valid
> activation_tracker_.

The next change I have in mind is creating a class that manages showing and
hiding the view. It would own both current_view_ and activation_tracker_ and
keep them in sync. Hopefully this will make their relationship clearer / suggest
further improvements.

[benwells, 2013-08-09 00:11:49]

On 2013/08/08 23:42:25, koz wrote:
>
https://codereview.chromium.org/22488007/diff/1/chrome/browser/ui/views/app_l...
> File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):
> 
>
https://codereview.chromium.org/22488007/diff/1/chrome/browser/ui/views/app_l...
> chrome/browser/ui/views/app_list/app_list_controller_win.cc:315: void
> CheckTaskbarOrViewHasFocus() {
> On 2013/08/08 22:11:15, benwells wrote:
> > It would be great to factorize this monster. Although looking more closely
it
> is
> > mostly comments :-/
> 
> There might be a way to break it up, but I'm happy to just separate it out for
> now.
> 
>
https://codereview.chromium.org/22488007/diff/1/chrome/browser/ui/views/app_l...
> chrome/browser/ui/views/app_list/app_list_controller_win.cc:503:
> base::RepeatingTimer<AppListController> timer_;
> On 2013/08/08 22:11:15, benwells wrote:
> > Can this be removed now?
> 
> Done.
> 
>
https://codereview.chromium.org/22488007/diff/1/chrome/browser/ui/views/app_l...
> chrome/browser/ui/views/app_list/app_list_controller_win.cc:689:
> activation_tracker_.reset(new ActivationTracker(current_view_,
> On 2013/08/08 22:11:15, benwells wrote:
> > Hmm, pity this guy's lifetime isn't managed by the view but I can't think of
a
> > nice way to do that. Did you consider always having a tracker but updating
its
> > view pointer? You could probably just update it in OnActivationChanged.
> > 
> > This code now has a few assumptions about when there will be a valid
> > activation_tracker_.
> 
> The next change I have in mind is creating a class that manages showing and
> hiding the view. It would own both current_view_ and activation_tracker_ and
> keep them in sync. Hopefully this will make their relationship clearer /
suggest
> further improvements.

ok, lgtm then.

[commit-bot: I haz the power, 2013-08-09 06:39:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/22488007/2002

[commit-bot: I haz the power, 2013-08-09 06:44:49]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-08-10 01:10:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/22488007/25001

[commit-bot: I haz the power, 2013-08-10 14:10:04]

Change committed as 216849