Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1164033002:
Change focus status according to the change of window focus (Closed)
|
Created:
5 years, 6 months ago by Jaekyun Seok (inactive) Modified:
5 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, AKVT Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange focus status according to the change of window focus
On Android, View's focus status isn't changed when window focus is lost.
So we should update ContentViewCore's focus status according to the
change of window focus.
BUG=495547
Committed: https://crrev.com/98d5087ecd03ed36ec2e161102b71dad97a92b4c
Cr-Commit-Position: refs/heads/master@{#333446}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Apply jdduke@'s comments #
Total comments: 4
Patch Set 3 : Update native focus with focus status of View and Window #
Total comments: 11
Patch Set 4 : Update hasFocus() as well #
Total comments: 5
Patch Set 5 : Rename method #Messages
Total messages: 33 (5 generated)
jaekyun@chromium.org changed reviewers: + jdduke@chromium.org, tedchoc@chromium.org
jdduke@ and tedchoc@, please review this CL.
https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1639: if (mContainerView.hasFocus()) onFocusChanged(hasWindowFocus); I recall that this method is triggered when the user opens the dropdown toolbar menu. I don't think we want to blur() the host in that case, is that why you have the mContainerView.hasFocus() check?
https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1639: if (mContainerView.hasFocus()) onFocusChanged(hasWindowFocus); On 2015/06/03 22:46:56, jdduke wrote: > I recall that this method is triggered when the user opens the dropdown toolbar > menu. I don't think we want to blur() the host in that case, is that why you > have the mContainerView.hasFocus() check? The mContainerView.hasFocus() check isn't related to that; I used it to avoid unnecessary focus change. BTW, could you please elaborate more about the dropdown toolbar menu? And when I tested this CL I didn't see any blurring effect when window focus was lost; only a cursor's blinking stops.
On 2015/06/03 22:57:01, Jaekyun Seok wrote: > https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1639: > if (mContainerView.hasFocus()) onFocusChanged(hasWindowFocus); > On 2015/06/03 22:46:56, jdduke wrote: > > I recall that this method is triggered when the user opens the dropdown > toolbar > > menu. I don't think we want to blur() the host in that case, is that why you > > have the mContainerView.hasFocus() check? > > The mContainerView.hasFocus() check isn't related to that; I used it to avoid > unnecessary focus change. > > BTW, could you please elaborate more about the dropdown toolbar menu? > When you tap the toolbar icon to open the dropdown, I believe we get an onWindowFocusChanged call. I'm not sure if we want that to cause the web content to lose focus, but I'm not sure if there's a good way to distinguish between that happening and the more general multi-window loss of focus. > And when I tested this CL I didn't see any blurring effect when window focus was > lost; only a cursor's blinking stops. Oh, sorry, by "blur" I just mean that the window loses focus. For whatever historical reason, the term "blur" is used to indicate "unfocus" in Chrome code =/.
On 2015/06/03 22:59:55, jdduke wrote: > On 2015/06/03 22:57:01, Jaekyun Seok wrote: > > > https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... > > File > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > > (right): > > > > > https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1639: > > if (mContainerView.hasFocus()) onFocusChanged(hasWindowFocus); > > On 2015/06/03 22:46:56, jdduke wrote: > > > I recall that this method is triggered when the user opens the dropdown > > toolbar > > > menu. I don't think we want to blur() the host in that case, is that why you > > > have the mContainerView.hasFocus() check? > > > > The mContainerView.hasFocus() check isn't related to that; I used it to avoid > > unnecessary focus change. > > > > BTW, could you please elaborate more about the dropdown toolbar menu? > > > > When you tap the toolbar icon to open the dropdown, I believe we get an > onWindowFocusChanged call. I'm not sure if we want that to cause the web content > to lose focus, but I'm not sure if there's a good way to distinguish between > that happening and the more general multi-window loss of focus. I confirmed your recall was right. But keeping a focus doesn't seem right when opening the toolbar menu. For example, opening the toolbar menu doesn't hide the IME even though clicking a character doesn't work at this status. Do you think that this behavior is right? > > > And when I tested this CL I didn't see any blurring effect when window focus > was > > lost; only a cursor's blinking stops. > > Oh, sorry, by "blur" I just mean that the window loses focus. For whatever > historical reason, the term "blur" is used to indicate "unfocus" in Chrome code > =/.
On 2015/06/03 23:28:23, Jaekyun Seok wrote: > On 2015/06/03 22:59:55, jdduke wrote: > > On 2015/06/03 22:57:01, Jaekyun Seok wrote: > > > > > > https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... > > > File > > > > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > > > (right): > > > > > > > > > https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... > > > > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1639: > > > if (mContainerView.hasFocus()) onFocusChanged(hasWindowFocus); > > > On 2015/06/03 22:46:56, jdduke wrote: > > > > I recall that this method is triggered when the user opens the dropdown > > > toolbar > > > > menu. I don't think we want to blur() the host in that case, is that why > you > > > > have the mContainerView.hasFocus() check? > > > > > > The mContainerView.hasFocus() check isn't related to that; I used it to > avoid > > > unnecessary focus change. > > > > > > BTW, could you please elaborate more about the dropdown toolbar menu? > > > > > > > When you tap the toolbar icon to open the dropdown, I believe we get an > > onWindowFocusChanged call. I'm not sure if we want that to cause the web > content > > to lose focus, but I'm not sure if there's a good way to distinguish between > > that happening and the more general multi-window loss of focus. > > I confirmed your recall was right. But keeping a focus doesn't seem right when > opening the toolbar menu. > For example, opening the toolbar menu doesn't hide the IME even though clicking > a character doesn't work at this status. > > Do you think that this behavior is right? Well, there are more side effects than just dismissing the IME. For instance, this might clear the selection, and it will dismiss all popups (select popups, disambiguation popups, etc...). I'm not sure we want that (we could split off that logic if necessary). We also need to make sure the focus calls are balanced and well-ordered, e.g., can we get onWindowFocusChanged(false) then onFocusChanged(false)?
On 2015/06/03 23:46:52, jdduke wrote: > On 2015/06/03 23:28:23, Jaekyun Seok wrote: > > On 2015/06/03 22:59:55, jdduke wrote: > > > On 2015/06/03 22:57:01, Jaekyun Seok wrote: > > > > > > > > > > https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... > > > > File > > > > > > > > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... > > > > > > > > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1639: > > > > if (mContainerView.hasFocus()) onFocusChanged(hasWindowFocus); > > > > On 2015/06/03 22:46:56, jdduke wrote: > > > > > I recall that this method is triggered when the user opens the dropdown > > > > toolbar > > > > > menu. I don't think we want to blur() the host in that case, is that why > > you > > > > > have the mContainerView.hasFocus() check? > > > > > > > > The mContainerView.hasFocus() check isn't related to that; I used it to > > avoid > > > > unnecessary focus change. > > > > > > > > BTW, could you please elaborate more about the dropdown toolbar menu? > > > > > > > > > > When you tap the toolbar icon to open the dropdown, I believe we get an > > > onWindowFocusChanged call. I'm not sure if we want that to cause the web > > content > > > to lose focus, but I'm not sure if there's a good way to distinguish between > > > that happening and the more general multi-window loss of focus. > > > > I confirmed your recall was right. But keeping a focus doesn't seem right when > > opening the toolbar menu. > > For example, opening the toolbar menu doesn't hide the IME even though > clicking > > a character doesn't work at this status. > > > > Do you think that this behavior is right? > > Well, there are more side effects than just dismissing the IME. For instance, > this might clear the selection, and it will dismiss all popups (select popups, > disambiguation popups, etc...). I'm not sure we want that (we could split off > that logic if necessary). We also need to make sure the focus calls are balanced > and well-ordered, e.g., can we get onWindowFocusChanged(false) then > onFocusChanged(false)? I understood. I will dig more to find a better solution. And View's focus status isn't changed even when Window's focus status is changed according to http://developer.android.com/reference/android/view/View.html#onWindowFocusCh....
https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1639: if (mContainerView.hasFocus()) onFocusChanged(hasWindowFocus); One idea here would be to instead just call the native focus setter directly: if (mContainer.hasFocus()) { // *insert some note about how window and view focus are different but that the content view needs to know regardless* if (mNatieContentViewCore != 0) nativeSetFocus(mNativeContentViewCore, hasWindowFocus); }
On 2015/06/04 00:23:41, jdduke wrote: > https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1639: > if (mContainerView.hasFocus()) onFocusChanged(hasWindowFocus); > One idea here would be to instead just call the native focus setter directly: > > if (mContainer.hasFocus()) { > // *insert some note about how window and view focus are different but that > the content view needs to know regardless* > if (mNatieContentViewCore != 0) nativeSetFocus(mNativeContentViewCore, > hasWindowFocus); > } I will try this. Thanks!
PTAL. https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1639: if (mContainerView.hasFocus()) onFocusChanged(hasWindowFocus); On 2015/06/04 00:23:41, jdduke wrote: > One idea here would be to instead just call the native focus setter directly: > > if (mContainer.hasFocus()) { > // *insert some note about how window and view focus are different but that > the content view needs to know regardless* > if (mNatieContentViewCore != 0) nativeSetFocus(mNativeContentViewCore, > hasWindowFocus); > } Done.
Looks reasonable to me, Ted do you have any thoughts? https://codereview.chromium.org/1164033002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1648: For completeness can you add a javadoc to this method? * @see View#onFocusChanged(boolean) https://codereview.chromium.org/1164033002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1649: public void onFocusChanged(boolean gainFocus) { So, just to be clear, should this assertion hold? assert mContainerView.hasWindowFocus(); If so, would be nice to add that to be explicit about the assumptions.
On 2015/06/04 15:28:10, jdduke wrote: > Looks reasonable to me, Ted do you have any thoughts? Seems reasonable to me as well. Does blur have effects on anything like long press on links (contextual menus)? HTML5 date/color pickers? Do those get dismissed if we do stuff like this? We will lose window focus for any dialogs triggered by the web contents, but I want to make sure none of those automatically are dismissed on use defocusing the RenderWidgetHostViewAndroid. > > https://codereview.chromium.org/1164033002/diff/20001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/1164033002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1648: > > For completeness can you add a javadoc to this method? > > * @see View#onFocusChanged(boolean) > > https://codereview.chromium.org/1164033002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1649: > public void onFocusChanged(boolean gainFocus) { > So, just to be clear, should this assertion hold? > > assert mContainerView.hasWindowFocus(); > > If so, would be nice to add that to be explicit about the assumptions.
PTAL. I've modified codes to update native focus always with focus status of View and Window. https://codereview.chromium.org/1164033002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1648: On 2015/06/04 15:28:09, jdduke wrote: > For completeness can you add a javadoc to this method? > > * @see View#onFocusChanged(boolean) Done. https://codereview.chromium.org/1164033002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1649: public void onFocusChanged(boolean gainFocus) { On 2015/06/04 15:28:09, jdduke wrote: > So, just to be clear, should this assertion hold? > > assert mContainerView.hasWindowFocus(); > > If so, would be nice to add that to be explicit about the assumptions. onFocusChanged could be called even when an app is in the background if some other View requests focus. So we can't assert it. Instead I will record focus status on native, and avoid duplicated calls.
On 2015/06/04 17:41:35, Ted C wrote: > On 2015/06/04 15:28:10, jdduke wrote: > > Looks reasonable to me, Ted do you have any thoughts? > > Seems reasonable to me as well. Does blur have effects on anything like > long press on links (contextual menus)? HTML5 date/color pickers? > Do those get dismissed if we do stuff like this? I tried contextual menus and data/color pickers, but I didn't see any side effects. > > We will lose window focus for any dialogs triggered by the web contents, > but I want to make sure none of those automatically are dismissed on > use defocusing the RenderWidgetHostViewAndroid. I didn't see any unexpected dismiss of dialogs with this CL. > > > > > > https://codereview.chromium.org/1164033002/diff/20001/content/public/android/... > > File > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > > (right): > > > > > https://codereview.chromium.org/1164033002/diff/20001/content/public/android/... > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1648: > > > > For completeness can you add a javadoc to this method? > > > > * @see View#onFocusChanged(boolean) > > > > > https://codereview.chromium.org/1164033002/diff/20001/content/public/android/... > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1649: > > public void onFocusChanged(boolean gainFocus) { > > So, just to be clear, should this assertion hold? > > > > assert mContainerView.hasWindowFocus(); > > > > If so, would be nice to add that to be explicit about the assumptions.
jdduke@chromium.org changed reviewers: + boliu@chromium.org
Bo, is there anything special we need to consider about view and window focus changes with WebView? https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1639: boolean hasFocus = mContainerView.hasFocus() && mContainerView.hasWindowFocus(); I think this might break WebView, see the definition of ContentViewCore.hasFocus(). I'm a little wary of caching the focus state, as I believe the web contents can get created with or without focus. So we might want to update the semantics of ContentViewCore.hasFocus() to include the window focus, but again we'll need to make sure that plays nicely with WebView.
I think having WebContents focus be window focus && view focus should be fine for webview. No one is really a good judge of these things until something actually breaks.. but the golden rules is don't make things messier and more difficult to understand. https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:602: private boolean mHasFocusOnNative; why would native ever diverge from java? https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1639: boolean hasFocus = mContainerView.hasFocus() && mContainerView.hasWindowFocus(); On 2015/06/05 01:54:31, jdduke(OOO_until_June_8) wrote: > I think this might break WebView, see the definition of > ContentViewCore.hasFocus(). I'm a little wary of caching the focus state, as I > believe the web contents can get created with or without focus. So we might want > to update the semantics of ContentViewCore.hasFocus() to include the window > focus, but again we'll need to make sure that plays nicely with WebView. I agree that focus state in other areas of code should match WebContent's focus state. But I'm not sure if changing hasFocus is the best change. I mean in that case, should onWindowFocusChanged and onFocusChanged be merged as well to be consistent? CVC is weird that it's modeling both android View and WebContents, and it's sucking when those two don't agree. Maybe we should remove CVC.hasFocus and add a hasFocus method to java WebContents to clean this up. https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1640: if (mNativeContentViewCore != 0 && mHasFocusOnNative != hasFocus) { Oh is it just to save a jni call when things don't change? Is it that expensive? Because focus shouldn't change all that often, so it's probably not worth optimizing over it.
https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2645: return mContainerView.hasFocus(); So this could probably be "return mContainerView.hasFocus() && mContainerView.hasWindowFocus()", then we just unconditionally call setNativeFocus(hasFocus()) when we get a view/window focus change event?
PTAL. https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:602: private boolean mHasFocusOnNative; On 2015/06/05 05:15:50, boliu wrote: > why would native ever diverge from java? This is removed. https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1639: boolean hasFocus = mContainerView.hasFocus() && mContainerView.hasWindowFocus(); On 2015/06/05 05:15:51, boliu wrote: > On 2015/06/05 01:54:31, jdduke(OOO_until_June_8) wrote: > > I think this might break WebView, see the definition of > > ContentViewCore.hasFocus(). I'm a little wary of caching the focus state, as I > > believe the web contents can get created with or without focus. So we might > want > > to update the semantics of ContentViewCore.hasFocus() to include the window > > focus, but again we'll need to make sure that plays nicely with WebView. > > I agree that focus state in other areas of code should match WebContent's focus > state. But I'm not sure if changing hasFocus is the best change. I mean in that > case, should onWindowFocusChanged and onFocusChanged be merged as well to be > consistent? In Java side, there seem some different tasks which should be done respectively in onWindowFocusChanged and onFocusChanged. So I want to keep the logics as they are. > > CVC is weird that it's modeling both android View and WebContents, and it's > sucking when those two don't agree. > > Maybe we should remove CVC.hasFocus and add a hasFocus method to java > WebContents to clean this up. We could do that when CVC is refactored later. https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1640: if (mNativeContentViewCore != 0 && mHasFocusOnNative != hasFocus) { On 2015/06/05 05:15:51, boliu wrote: > Oh is it just to save a jni call when things don't change? Is it that expensive? > Because focus shouldn't change all that often, so it's probably not worth > optimizing over it. mHasFocusOnNative is removed. https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2645: return mContainerView.hasFocus(); On 2015/06/05 14:38:05, jdduke(OOO_until_June_8) wrote: > So this could probably be "return mContainerView.hasFocus() && > mContainerView.hasWindowFocus()", then we just unconditionally call > setNativeFocus(hasFocus()) when we get a view/window focus change event? Done.
ajith.v@samsung.com changed reviewers: + ajith.v@samsung.com
https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1639: boolean hasFocus = mContainerView.hasFocus() && mContainerView.hasWindowFocus(); On 2015/06/05 05:15:51, boliu wrote: > On 2015/06/05 01:54:31, jdduke(OOO_until_June_8) wrote: > > I think this might break WebView, see the definition of > > ContentViewCore.hasFocus(). I'm a little wary of caching the focus state, as I > > believe the web contents can get created with or without focus. So we might > want > > to update the semantics of ContentViewCore.hasFocus() to include the window > > focus, but again we'll need to make sure that plays nicely with WebView. > > I agree that focus state in other areas of code should match WebContent's focus > state. But I'm not sure if changing hasFocus is the best change. I mean in that > case, should onWindowFocusChanged and onFocusChanged be merged as well to be > consistent? > > CVC is weird that it's modeling both android View and WebContents, and it's > sucking when those two don't agree. > > Maybe we should remove CVC.hasFocus and add a hasFocus method to java > WebContents to clean this up. I too felt having a focus method in WebContents will simplify things further. We have to ensure WebContents's focus is set correct during focus change of CVC. Currently WebContents querying back to CVCImpl for checking the focus, which is not required. I assumes from renderer side, RWHVA focus change won't be triggered in any case. Please correct me if I am wrong in this statement. https://codereview.chromium.org/1164033002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2645: return mContainerView.hasFocus(); On 2015/06/05 14:38:05, jdduke(OOO_until_June_8) wrote: > So this could probably be "return mContainerView.hasFocus() && > mContainerView.hasWindowFocus()", then we just unconditionally call > setNativeFocus(hasFocus()) when we get a view/window focus change event? This is very much legitimate. We need to test our input types (date, month, color etc.) which creates new window on CVC.
lgtm, would be nice if we could test this but I'm not sure we have great exposure of web contents focus state. Please wait for Ted's approval. https://codereview.chromium.org/1164033002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1636: if (mNativeContentViewCore != 0) nativeSetFocus(mNativeContentViewCore, hasFocus()); Looks like the overhead here is 2 or 3 IPCs (RenderWidgetHost doesn't cache focus state). Probably not a big deal.
lgtm https://codereview.chromium.org/1164033002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1635: private void updateFocusOnNative() { I would call this updateNativeFocus. OnNative reads odd to me.
https://codereview.chromium.org/1164033002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2631: * @see View#hasFocus() This comment should be fixed
https://codereview.chromium.org/1164033002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1164033002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1635: private void updateFocusOnNative() { On 2015/06/08 16:19:33, Ted C wrote: > I would call this updateNativeFocus. OnNative reads odd to me. Done. https://codereview.chromium.org/1164033002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2631: * @see View#hasFocus() On 2015/06/08 16:20:54, boliu wrote: > This comment should be fixed Done.
I don't own this code. Don't have to wait for me :p
The CQ bit was checked by jaekyun@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, jdduke@chromium.org Link to the patchset: https://codereview.chromium.org/1164033002/#ps80001 (title: "Rename method")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1164033002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/98d5087ecd03ed36ec2e161102b71dad97a92b4c Cr-Commit-Position: refs/heads/master@{#333446}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1177483003/ by jaekyun@chromium.org. The reason for reverting is: This CL caused http://crbug.com/498631.. |
