keyboard: Update the visibility after tab-switch.

keyboard: Update the visibility after tab-switch.

BUG=70784
TEST=manually, see bug

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

[sadrul, 2011-01-25 18:10:48]

http://codereview.chromium.org/6277020/diff/1/chrome/browser/tab_contents/tab...
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/6277020/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents.h:1338: bool
selected_field_is_editable_;
This could also move to RenderWidgetHostView or RenderWidgetHostViewViews.
Opinions?

[bryeung, 2011-01-25 18:37:27]

Looks okay to me.

Please wait for sky to weigh in on where the selected_field_is_editable flag
should get cached.

[rjkroege, 2011-01-25 18:41:30]

LGTM.

[sky, 2011-01-25 19:15:21]

TabContents doesn't feel like quite the right place, but I think it's the best
place for now.

http://codereview.chromium.org/6277020/diff/1/chrome/browser/tab_contents/tab...
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/6277020/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents.h:1337: // Whether the selected field
in the tab is an editable field.
Should this be focused?

http://codereview.chromium.org/6277020/diff/1/chrome/browser/ui/touch/frame/t...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/6277020/diff/1/chrome/browser/ui/touch/frame/t...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:41:
browser_view->browser()->tabstrip_model()->AddObserver(this);
You need to remove this observer at some point.

[sadrul, 2011-01-25 21:11:50]

http://codereview.chromium.org/6277020/diff/1/chrome/browser/tab_contents/tab...
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/6277020/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents.h:1337: // Whether the selected field
in the tab is an editable field.
On 2011/01/25 19:15:21, sky wrote:
> Should this be focused?

Indeed. Changed the 'selected's into 'focused' to clarify the meaning.

http://codereview.chromium.org/6277020/diff/1/chrome/browser/ui/touch/frame/t...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/6277020/diff/1/chrome/browser/ui/touch/frame/t...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:41:
browser_view->browser()->tabstrip_model()->AddObserver(this);
On 2011/01/25 19:15:21, sky wrote:
> You need to remove this observer at some point.

Done.

[sky, 2011-01-25 21:37:23]

On second thought, you should really get Brett (added to reviewer line) or
Pinkerton to review the change to TabContents. They are more of an owner of
TabContents than I.

The other changes LGTM.

http://codereview.chromium.org/6277020/diff/8001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/6277020/diff/8001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.h:1338: bool
focused_field_is_editable_;
You need to initialize this in the constructor.

[sadrul, 2011-01-25 21:41:25]

OK. Thanks for your review.

[sky, 2011-01-25 23:05:31]

Brett, correct me if I'm wrong, but I think the right place to stash this state
is in TabContent's PropertyBag.

  -Scott

[sadrul, 2011-01-26 14:55:25]

On 2011/01/25 23:05:31, sky wrote:
> Brett, correct me if I'm wrong, but I think the right place to stash this
state
> is in TabContent's PropertyBag.

Seeing how TabContents::property_bag_ is used, it looks like it is intended to
be used by things that are not TabContents itself. Since in this case, the
focused-field-is-editable state is maintained by TabContents itself, perhaps
using property_bag_ isn't necessary?

[sky, 2011-01-26 15:54:43]

On Wed, Jan 26, 2011 at 6:55 AM,  <sadrul@chromium.org> wrote:
> On 2011/01/25 23:05:31, sky wrote:
>>
>> Brett, correct me if I'm wrong, but I think the right place to stash this
>
> state
>>
>> is in TabContent's PropertyBag.
>
> Seeing how TabContents::property_bag_ is used, it looks like it is intended
> to
> be used by things that are not TabContents itself. Since in this case, the
> focused-field-is-editable state is maintained by TabContents itself, perhaps
> using property_bag_ isn't necessary?

I was thinking the touch related classes would update the property
bag, not tab contents.

  -Scott

[sadrul, 2011-01-26 16:26:08]

Aha! I misunderstood, sorry.

Using the property-bag this way makes a lot of sense. I have made the change to
use the property bag. Please take another look!

[sky, 2011-01-26 16:40:07]

http://codereview.chromium.org/6277020/diff/13002/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/6277020/diff/13002/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:140:
GetFocusedStateAccessor()->SetProperty(
This code is problematic if focus changes on a background page. Is that
possible? perhaps through script?

[sadrul, 2011-01-26 17:43:10]

Made the changes. Please take another look.

http://codereview.chromium.org/6277020/diff/13002/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/6277020/diff/13002/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:140:
GetFocusedStateAccessor()->SetProperty(
On 2011/01/26 16:40:08, sky wrote:
> This code is problematic if focus changes on a background page. Is that
> possible? perhaps through script?

Indeed! I made the change to update the property-bag of the tab-contents that
triggered the focus-change notification, instead of the tab-contents currently
in focus. Thanks for catching this.

http://codereview.chromium.org/6277020/diff/22001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:153: }
Or should this be done from TabDetachedAt and TabReplacedAt?

[sky, 2011-01-26 18:07:43]

http://codereview.chromium.org/6277020/diff/22001/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/6277020/diff/22001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:142: TabContents*
source_tab = static_cast<TabContents*>(host->delegate());
Casts like this are dangerous. Could we change FOCUS_CHANGED_IN_PAGE to pass in
the TabContents instead of the RVH?

http://codereview.chromium.org/6277020/diff/22001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:153:
Source<TabContents>(source).ptr()->property_bag());
On 2011/01/26 17:43:10, sadrul wrote:
> Or should this be done from TabDetachedAt and TabReplacedAt?

This is better.

[brettw, 2011-01-26 20:18:06]

http://codereview.chromium.org/6277020/diff/22001/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/6277020/diff/22001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:142: TabContents*
source_tab = static_cast<TabContents*>(host->delegate());
On 2011/01/26 18:07:43, sky wrote:
> Casts like this are dangerous. Could we change FOCUS_CHANGED_IN_PAGE to pass
in
> the TabContents instead of the RVH?

I agree, we should never have this type of cast.

[sadrul, 2011-01-26 20:42:46]

On 2011/01/26 20:18:06, brettw wrote:
>
http://codereview.chromium.org/6277020/diff/22001/chrome/browser/ui/touch/fra...
> File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):
> 
>
http://codereview.chromium.org/6277020/diff/22001/chrome/browser/ui/touch/fra...
> chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:142: TabContents*
> source_tab = static_cast<TabContents*>(host->delegate());
> On 2011/01/26 18:07:43, sky wrote:
> > Casts like this are dangerous. Could we change FOCUS_CHANGED_IN_PAGE to pass
> in
> > the TabContents instead of the RVH?
> 
> I agree, we should never have this type of cast.

Noted. Changed to pass TC instead of RVH with FOCUS_CHANGED_IN_PAGE. Please take
another look.

(Sent to the trybots)

[sky, 2011-01-26 21:02:21]

LGTM