Move {Restore,Store}FocusedView to NativeWidgetAura::On{Activated,LostActive}

Move {Restore,Store}FocusedView to NativeWidgetAura::On{Activated,LostActive}.

BUG=104361
TEST=Manual


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

[mazda, 2011-12-06 04:24:06]

It turned out it was my fault...

[oshima, 2011-12-06 19:30:37]

LGTM. please remove the comment I mentioned.
alice, GetInitialFocus issue below may be what you're looking for.

http://codereview.chromium.org/8821004/diff/1/ui/views/widget/native_widget_a...
File ui/views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/8821004/diff/1/ui/views/widget/native_widget_a...
ui/views/widget/native_widget_aura.cc:413: !GetWidget()->SetInitialFocus())) {
This looks wrong. Could be the cause of crbug.com/105195.
alice, can you look into this?

http://codereview.chromium.org/8821004/diff/1/ui/views/widget/native_widget_a...
ui/views/widget/native_widget_aura.cc:556: // See description of
got_initial_focus_in_ for details on this.
please remove this as this is a copy from gtk and not applicable here.

[mazda, 2011-12-07 04:51:51]

I found this change sometimes causes another issue that makes omnibox
unfocusable though I haven't found a reproducible step yet.
I'm looking into it now and hold off on submitting this CL until the cause of
the focus issue is found.

Thanks,

http://codereview.chromium.org/8821004/diff/1/ui/views/widget/native_widget_a...
File ui/views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/8821004/diff/1/ui/views/widget/native_widget_a...
ui/views/widget/native_widget_aura.cc:556: // See description of
got_initial_focus_in_ for details on this.
On 2011/12/06 19:30:37, oshima wrote:
> please remove this as this is a copy from gtk and not applicable here.

Done.

[oshima, 2011-12-07 06:19:12]

Ok, if that's the case, I can take over this bug as I'm also looking at
omnibox focus issue. One issue is that both location bar and textfield are
focusable and it's causing some issue in focus traversal and I'm working on
fix.

- oshima

On Tue, Dec 6, 2011 at 8:51 PM, <mazda@chromium.org> wrote:

> I found this change sometimes causes another issue that makes omnibox
> unfocusable though I haven't found a reproducible step yet.
> I'm looking into it now and hold off on submitting this CL until the cause
> of
> the focus issue is found.
>
> Thanks,
>
>
> http://codereview.chromium.**org/8821004/diff/1/ui/views/**
>
widget/native_widget_aura.cc<http://codereview.chromium.org/8821004/diff/1/ui/views/widget/native_widget_aura.cc>
> File ui/views/widget/native_widget_**aura.cc (right):
>
> http://codereview.chromium.**org/8821004/diff/1/ui/views/**
>
widget/native_widget_aura.cc#**newcode556<http://codereview.chromium.org/8821004/diff/1/ui/views/widget/native_widget_aura.cc#newcode556>
> ui/views/widget/native_widget_**aura.cc:556: // See description of
>
> got_initial_focus_in_ for details on this.
> On 2011/12/06 19:30:37, oshima wrote:
>
>> please remove this as this is a copy from gtk and not applicable here.
>>
>
> Done.
>
>
http://codereview.chromium.**org/8821004/<http://codereview.chromium.org/8821...
>

[mazda, 2011-12-07 08:47:04]

Thank you for your offer.

I tried another approach and moved RestoreFocusedView to OnActivated and
StoreFocusedView to OnLostActive respectively.
This at least fixes issue 104361 and does not cause the issue I mentioned in the
previous message.
Could you take another look?

If this does not look good or does not fix the omnibox focus issue, could you
take over the issue?

Thanks,

On 2011/12/07 06:19:12, oshima wrote:
> Ok, if that's the case, I can take over this bug as I'm also looking at
> omnibox focus issue. One issue is that both location bar and textfield are
> focusable and it's causing some issue in focus traversal and I'm working on
> fix.
> 
> - oshima
> 
> On Tue, Dec 6, 2011 at 8:51 PM, <mailto:mazda@chromium.org> wrote:
> 
> > I found this change sometimes causes another issue that makes omnibox
> > unfocusable though I haven't found a reproducible step yet.
> > I'm looking into it now and hold off on submitting this CL until the cause
> > of
> > the focus issue is found.
> >
> > Thanks,
> >
> >
> > http://codereview.chromium.**org/8821004/diff/1/ui/views/**
> >
>
widget/native_widget_aura.cc<http://codereview.chromium.org/8821004/diff/1/ui/views/widget/native_widget_aura.cc>
> > File ui/views/widget/native_widget_**aura.cc (right):
> >
> > http://codereview.chromium.**org/8821004/diff/1/ui/views/**
> >
>
widget/native_widget_aura.cc#**newcode556<http://codereview.chromium.org/8821004/diff/1/ui/views/widget/native_widget_aura.cc#newcode556>
> > ui/views/widget/native_widget_**aura.cc:556: // See description of
> >
> > got_initial_focus_in_ for details on this.
> > On 2011/12/06 19:30:37, oshima wrote:
> >
> >> please remove this as this is a copy from gtk and not applicable here.
> >>
> >
> > Done.
> >
> >
>
http://codereview.chromium.**org/8821004/%3Chttp://codereview.chromium.org/88...>
> >

[oshima, 2011-12-07 17:56:38]

On 2011/12/07 08:47:04, mazda wrote:
> Thank you for your offer.
> 
> I tried another approach and moved RestoreFocusedView to OnActivated and
> StoreFocusedView to OnLostActive respectively.
> This at least fixes issue 104361 and does not cause the issue I mentioned in
the
> previous message.
> Could you take another look?
> 
> If this does not look good or does not fix the omnibox focus issue, could you
> take over the issue?

I'm not 100% sure if this is right thing. What was the issue you had with your
original CL?


> 
> Thanks,
> 
> On 2011/12/07 06:19:12, oshima wrote:
> > Ok, if that's the case, I can take over this bug as I'm also looking at
> > omnibox focus issue. One issue is that both location bar and textfield are
> > focusable and it's causing some issue in focus traversal and I'm working on
> > fix.
> > 
> > - oshima
> > 
> > On Tue, Dec 6, 2011 at 8:51 PM, <mailto:mazda@chromium.org> wrote:
> > 
> > > I found this change sometimes causes another issue that makes omnibox
> > > unfocusable though I haven't found a reproducible step yet.
> > > I'm looking into it now and hold off on submitting this CL until the cause
> > > of
> > > the focus issue is found.
> > >
> > > Thanks,
> > >
> > >
> > > http://codereview.chromium.**org/8821004/diff/1/ui/views/**
> > >
> >
>
widget/native_widget_aura.cc<http://codereview.chromium.org/8821004/diff/1/ui/views/widget/native_widget_aura.cc>
> > > File ui/views/widget/native_widget_**aura.cc (right):
> > >
> > > http://codereview.chromium.**org/8821004/diff/1/ui/views/**
> > >
> >
>
widget/native_widget_aura.cc#**newcode556<http://codereview.chromium.org/8821004/diff/1/ui/views/widget/native_widget_aura.cc#newcode556>
> > > ui/views/widget/native_widget_**aura.cc:556: // See description of
> > >
> > > got_initial_focus_in_ for details on this.
> > > On 2011/12/06 19:30:37, oshima wrote:
> > >
> > >> please remove this as this is a copy from gtk and not applicable here.
> > >>
> > >
> > > Done.
> > >
> > >
> >
>
http://codereview.chromium.**org/8821004/%253Chttp://codereview.chromium.org/...>
> > >

[oshima, 2011-12-07 17:56:38]

On 2011/12/07 08:47:04, mazda wrote:
> Thank you for your offer.
> 
> I tried another approach and moved RestoreFocusedView to OnActivated and
> StoreFocusedView to OnLostActive respectively.
> This at least fixes issue 104361 and does not cause the issue I mentioned in
the
> previous message.
> Could you take another look?
> 
> If this does not look good or does not fix the omnibox focus issue, could you
> take over the issue?

I'm not 100% sure if this is right thing. What was the issue you had with your
original CL?


> 
> Thanks,
> 
> On 2011/12/07 06:19:12, oshima wrote:
> > Ok, if that's the case, I can take over this bug as I'm also looking at
> > omnibox focus issue. One issue is that both location bar and textfield are
> > focusable and it's causing some issue in focus traversal and I'm working on
> > fix.
> > 
> > - oshima
> > 
> > On Tue, Dec 6, 2011 at 8:51 PM, <mailto:mazda@chromium.org> wrote:
> > 
> > > I found this change sometimes causes another issue that makes omnibox
> > > unfocusable though I haven't found a reproducible step yet.
> > > I'm looking into it now and hold off on submitting this CL until the cause
> > > of
> > > the focus issue is found.
> > >
> > > Thanks,
> > >
> > >
> > > http://codereview.chromium.**org/8821004/diff/1/ui/views/**
> > >
> >
>
widget/native_widget_aura.cc<http://codereview.chromium.org/8821004/diff/1/ui/views/widget/native_widget_aura.cc>
> > > File ui/views/widget/native_widget_**aura.cc (right):
> > >
> > > http://codereview.chromium.**org/8821004/diff/1/ui/views/**
> > >
> >
>
widget/native_widget_aura.cc#**newcode556<http://codereview.chromium.org/8821004/diff/1/ui/views/widget/native_widget_aura.cc#newcode556>
> > > ui/views/widget/native_widget_**aura.cc:556: // See description of
> > >
> > > got_initial_focus_in_ for details on this.
> > > On 2011/12/06 19:30:37, oshima wrote:
> > >
> > >> please remove this as this is a copy from gtk and not applicable here.
> > >>
> > >
> > > Done.
> > >
> > >
> >
>
http://codereview.chromium.**org/8821004/%253Chttp://codereview.chromium.org/...>
> > >

[oshima, 2011-12-08 21:05:23]

Ben, can you review this for mazda-san?

[oshima, 2011-12-08 21:06:03]

On 2011/12/08 21:05:23, oshima wrote:
> Ben, can you review this for mazda-san?

I updated the bug why this needs to be in activation. just FYI.

[Ben Goodger (Google), 2011-12-08 21:06:57]

lgtm

[commit-bot: I haz the power, 2011-12-08 21:08:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/8821004/7001

[commit-bot: I haz the power, 2011-12-08 21:23:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/8821004/7001

[commit-bot: I haz the power, 2011-12-08 23:02:37]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 113663, job name
was 8821004-7001 (previous was lost) (previous was lost) (retry) (previous was
lost).

[commit-bot: I haz the power, 2011-12-08 23:38:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/8821004/7001

[commit-bot: I haz the power, 2011-12-09 02:45:25]

Change committed as 113741