|
|
Created:
4 years, 11 months ago by tapted Modified:
4 years, 11 months ago CC:
chromium-reviews, jennb, jianli, Dmitry Titov, dcheng, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMac: Remove a case where panels may become key when an autofill popup closes
Since panels have a promoted -[NSWindow level], OSX prefers them to
browser windows when it needs to activate a window. One case this
happens is whenever an autofill popup is dismissed (e.g. when tabbing
between text fields on a web page).
Sometimes, a panel window may be activated in this case since it thinks
it was the "previous" key window, when actually it was a browser window.
The "previous" key window case doesn't really seem to add much. So
remove it. It means that a panel can lose focus (and not immediately
regain it) when another panel window is closed, but that seems
preferable to tabbing into a Hangouts chat window instead of a password
field when a username autofill popup is dismissed. (Note it doesn't
happen if a *browser* window is closed, since the native close button
doesn't activate a window, but the non-native hangouts close button
does).
The "allow previous" logic was included in r127517 (to fix
http://crbug.com/112038). Closing hangouts windows seems to be the only
use case it supports, and I don't think that use case will be missed.
BUG=546509, 112038
Committed: https://crrev.com/5b81d65c7382b3b855cbc5bd26a15cc68ef4c5e5
Cr-Commit-Position: refs/heads/master@{#369347}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove poopie code from chrome_browser_application_mac.mm #
Messages
Total messages: 28 (14 generated)
Description was changed from ========== Mac: Remove a case where panels may become key when an autofill popup closes Since panels have a promoted -[NSWindow level], OSX prefers them to browser windows when it needs to activate a window. One case this happens is whenever an autofill popup is dismissed (e.g. when tabbing betwween text fields on a web page). Sometimes, a panel window may be activated in this case since it thinks it was the "previous" key window, when actually it was a browser window. The "previous" key window case doesn't really seem to add much. So remove it. It might mean that a panel can lose focus (and not immediately regain it) when another Chrome window is closed, but that seems preferable to tabbing into a Hangouts chat window instead of a password field when a username autofill popup is dismissed. BUG=575274 ========== to ========== Mac: Remove a case where panels may become key when an autofill popup closes Since panels have a promoted -[NSWindow level], OSX prefers them to browser windows when it needs to activate a window. One case this happens is whenever an autofill popup is dismissed (e.g. when tabbing betwween text fields on a web page). Sometimes, a panel window may be activated in this case since it thinks it was the "previous" key window, when actually it was a browser window. The "previous" key window case doesn't really seem to add much. So remove it. It might mean that a panel can lose focus (and not immediately regain it) when another Chrome window is closed, but that seems preferable to tabbing into a Hangouts chat window instead of a password field when a username autofill popup is dismissed. BUG=575274, 112038 ==========
Description was changed from ========== Mac: Remove a case where panels may become key when an autofill popup closes Since panels have a promoted -[NSWindow level], OSX prefers them to browser windows when it needs to activate a window. One case this happens is whenever an autofill popup is dismissed (e.g. when tabbing betwween text fields on a web page). Sometimes, a panel window may be activated in this case since it thinks it was the "previous" key window, when actually it was a browser window. The "previous" key window case doesn't really seem to add much. So remove it. It might mean that a panel can lose focus (and not immediately regain it) when another Chrome window is closed, but that seems preferable to tabbing into a Hangouts chat window instead of a password field when a username autofill popup is dismissed. BUG=575274, 112038 ========== to ========== Mac: Remove a case where panels may become key when an autofill popup closes Since panels have a promoted -[NSWindow level], OSX prefers them to browser windows when it needs to activate a window. One case this happens is whenever an autofill popup is dismissed (e.g. when tabbing between text fields on a web page). Sometimes, a panel window may be activated in this case since it thinks it was the "previous" key window, when actually it was a browser window. The "previous" key window case doesn't really seem to add much. So remove it. It might mean that a panel can lose focus (and not immediately regain it) when another Chrome window is closed, but that seems preferable to tabbing into a Hangouts chat window instead of a password field when a username autofill popup is dismissed. BUG=575274, 112038 ==========
Description was changed from ========== Mac: Remove a case where panels may become key when an autofill popup closes Since panels have a promoted -[NSWindow level], OSX prefers them to browser windows when it needs to activate a window. One case this happens is whenever an autofill popup is dismissed (e.g. when tabbing between text fields on a web page). Sometimes, a panel window may be activated in this case since it thinks it was the "previous" key window, when actually it was a browser window. The "previous" key window case doesn't really seem to add much. So remove it. It might mean that a panel can lose focus (and not immediately regain it) when another Chrome window is closed, but that seems preferable to tabbing into a Hangouts chat window instead of a password field when a username autofill popup is dismissed. BUG=575274, 112038 ========== to ========== Mac: Remove a case where panels may become key when an autofill popup closes Since panels have a promoted -[NSWindow level], OSX prefers them to browser windows when it needs to activate a window. One case this happens is whenever an autofill popup is dismissed (e.g. when tabbing between text fields on a web page). Sometimes, a panel window may be activated in this case since it thinks it was the "previous" key window, when actually it was a browser window. The "previous" key window case doesn't really seem to add much. So remove it. It might mean that a panel can lose focus (and not immediately regain it) when another Chrome window is closed, but that seems preferable to tabbing into a Hangouts chat window instead of a password field when a username autofill popup is dismissed. BUG=546509, 112038 ==========
Description was changed from ========== Mac: Remove a case where panels may become key when an autofill popup closes Since panels have a promoted -[NSWindow level], OSX prefers them to browser windows when it needs to activate a window. One case this happens is whenever an autofill popup is dismissed (e.g. when tabbing between text fields on a web page). Sometimes, a panel window may be activated in this case since it thinks it was the "previous" key window, when actually it was a browser window. The "previous" key window case doesn't really seem to add much. So remove it. It might mean that a panel can lose focus (and not immediately regain it) when another Chrome window is closed, but that seems preferable to tabbing into a Hangouts chat window instead of a password field when a username autofill popup is dismissed. BUG=546509, 112038 ========== to ========== Mac: Remove a case where panels may become key when an autofill popup closes Since panels have a promoted -[NSWindow level], OSX prefers them to browser windows when it needs to activate a window. One case this happens is whenever an autofill popup is dismissed (e.g. when tabbing between text fields on a web page). Sometimes, a panel window may be activated in this case since it thinks it was the "previous" key window, when actually it was a browser window. The "previous" key window case doesn't really seem to add much. So remove it. It means that a panel can lose focus (and not immediately regain it) when another panel window is closed, but that seems preferable to tabbing into a Hangouts chat window instead of a password field when a username autofill popup is dismissed. (Note it doesn't happen if a *browser* window is closed, since the native close button doesn't activate a window, but the non-native hangouts close button does). The "allow previous" logic was included in r127517 (to fix http://crbug.com/112038). Closing hangouts windows seems to be the only use case it supports, and I don't think that use case will be missed. BUG=546509, 112038 ==========
tapted@chromium.org changed reviewers: + shess@chromium.org
Hi Scott, could you please take a look? Maybe you can think of a use-case I missed... (also you reviewed the original code 4 years ago :p). I have a pretty low bar for reverting this if any problems crop up, since this code is very fragile and tricky to maintain (and will likely be removed on OSX - http://crbug.com/467808 ). There's a bit more info in b/26431075 as well. Thanks!
shess: ping?
https://codereview.chromium.org/1575743003/diff/1/chrome/browser/ui/cocoa/pan... File chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm (left): https://codereview.chromium.org/1575743003/diff/1/chrome/browser/ui/cocoa/pan... chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm:98: [app previousKeyWindow] == self || AFAICT, the only reason |previousKeyWindows_| in chrome_browser_application_mac.mm exists is to support this check. Removing all of that makes me a little nervous (what if there are interesting non-obvious side effects?), but not removing all of that also makes me nervous. In the bug comment #3, it sounds like the sequence should be: 1) key window is new tab, previous is unknown. 2) key window is chat window, previous is #1's tab. 3.1) key window is #1's tab again, previous is #2's chat window. 3.2) key window is autocomplete popup, previous is #1's tab (the login page). AFAICT, the problem at #4 is that when the key window is the autocomplete popup, the previous key window is the chat window, instead of #1's tab (well, or window thereof). That would imply that the code to update the previousKeyWindows_ vector isn't firing for some reason. OR, the problem at #4 is that the autocomplete popup doesn't become key window, but dismissing it does cause the search for the previous key window. In that case, the current key window is still the tab's window, and the previous key window is the chat window, and it should be re-selecting the current key window somehow. [OR this needs more thought, again. It's hard to get back into the flow of this kind of change, since it's complicated visible code interacting with complicated invisible (and undocumented) code.]
Patchset #2 (id:20001) has been deleted
On 2016/01/12 22:29:55, Scott Hess wrote: > https://codereview.chromium.org/1575743003/diff/1/chrome/browser/ui/cocoa/pan... > File chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm (left): > > https://codereview.chromium.org/1575743003/diff/1/chrome/browser/ui/cocoa/pan... > chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm:98: [app > previousKeyWindow] == self || > AFAICT, the only reason |previousKeyWindows_| in > chrome_browser_application_mac.mm exists is to support this check. Removing all > of that makes me a little nervous (what if there are interesting non-obvious > side effects?), but not removing all of that also makes me nervous. Removed the code in chrome_browser_application_mac.mm . There used to be more in the _removeWindow override (not involving previousKeyWindows_), but I found a nicer way to do that in r323157 -> https://codereview.chromium.org/1040993005/. I considered the possibility that that CL could be involved here, or regressed something - I had a close look and nothing stands out. > In the bug comment #3, it sounds like the sequence should be: > 1) key window is new tab, previous is unknown. > 2) key window is chat window, previous is #1's tab. > 3.1) key window is #1's tab again, previous is #2's chat window. > 3.2) key window is autocomplete popup, previous is #1's tab (the login page). > > AFAICT, the problem at #4 is that when the key window is the autocomplete popup, > the previous key window is the chat window, instead of #1's tab (well, or window > thereof). That would imply that the code to update the previousKeyWindows_ > vector isn't firing for some reason. > > OR, the problem at #4 is that the autocomplete popup doesn't become key window, > but dismissing it does cause the search for the previous key window. In that > case, the current key window is still the tab's window, and the previous key > window is the chat window, and it should be re-selecting the current key window > somehow. > > [OR this needs more thought, again. It's hard to get back into the flow of this > kind of change, since it's complicated visible code interacting with complicated > invisible (and undocumented) code.] Yeah - this previousKeyWindows_ stuff is hard to reason about - it has bitten me a number of times. I'm not sad to see it removed, and if the only thing we lose is a panel failing to re-activate itself in some cases when a window is closed, then I think that's fine. I can't think of anything else that would get tripped up, and I've tried lots of workflows with this patched in - everything seems fine. I don't think it's worth the effort to consider a fix that makes previousKeyWindows_ even harder to reason about.
LGTM (Looks Grand To Me, because "good" isn't quite right). Keep an eye out for other code to remove in this area. One of the biggest concerns I have is that some other unrelated feature could develop implicit dependencies on this kind of hackery. I don't see anything obvious that could regress things in the referenced CL, but it does look like the kind of thing which _could_ regress things. Especially moving the selection of key window around, it's pretty plausible that the obvious-to-be-next window in the old code may be slightly different than the obvious-to-be-next window in the new code. But *shrug*, for this kind of thing having someone yell at you means they are paying attention.
tapted@chromium.org changed reviewers: + mark@chromium.org
+mark for OWNERS in chrome_browser_application_mac.*
LGTM
The CQ bit was checked by tapted@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1575743003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575743003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by tapted@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1575743003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575743003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by tapted@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1575743003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575743003/40001
Message was sent while issue was closed.
Description was changed from ========== Mac: Remove a case where panels may become key when an autofill popup closes Since panels have a promoted -[NSWindow level], OSX prefers them to browser windows when it needs to activate a window. One case this happens is whenever an autofill popup is dismissed (e.g. when tabbing between text fields on a web page). Sometimes, a panel window may be activated in this case since it thinks it was the "previous" key window, when actually it was a browser window. The "previous" key window case doesn't really seem to add much. So remove it. It means that a panel can lose focus (and not immediately regain it) when another panel window is closed, but that seems preferable to tabbing into a Hangouts chat window instead of a password field when a username autofill popup is dismissed. (Note it doesn't happen if a *browser* window is closed, since the native close button doesn't activate a window, but the non-native hangouts close button does). The "allow previous" logic was included in r127517 (to fix http://crbug.com/112038). Closing hangouts windows seems to be the only use case it supports, and I don't think that use case will be missed. BUG=546509, 112038 ========== to ========== Mac: Remove a case where panels may become key when an autofill popup closes Since panels have a promoted -[NSWindow level], OSX prefers them to browser windows when it needs to activate a window. One case this happens is whenever an autofill popup is dismissed (e.g. when tabbing between text fields on a web page). Sometimes, a panel window may be activated in this case since it thinks it was the "previous" key window, when actually it was a browser window. The "previous" key window case doesn't really seem to add much. So remove it. It means that a panel can lose focus (and not immediately regain it) when another panel window is closed, but that seems preferable to tabbing into a Hangouts chat window instead of a password field when a username autofill popup is dismissed. (Note it doesn't happen if a *browser* window is closed, since the native close button doesn't activate a window, but the non-native hangouts close button does). The "allow previous" logic was included in r127517 (to fix http://crbug.com/112038). Closing hangouts windows seems to be the only use case it supports, and I don't think that use case will be missed. BUG=546509, 112038 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Mac: Remove a case where panels may become key when an autofill popup closes Since panels have a promoted -[NSWindow level], OSX prefers them to browser windows when it needs to activate a window. One case this happens is whenever an autofill popup is dismissed (e.g. when tabbing between text fields on a web page). Sometimes, a panel window may be activated in this case since it thinks it was the "previous" key window, when actually it was a browser window. The "previous" key window case doesn't really seem to add much. So remove it. It means that a panel can lose focus (and not immediately regain it) when another panel window is closed, but that seems preferable to tabbing into a Hangouts chat window instead of a password field when a username autofill popup is dismissed. (Note it doesn't happen if a *browser* window is closed, since the native close button doesn't activate a window, but the non-native hangouts close button does). The "allow previous" logic was included in r127517 (to fix http://crbug.com/112038). Closing hangouts windows seems to be the only use case it supports, and I don't think that use case will be missed. BUG=546509, 112038 ========== to ========== Mac: Remove a case where panels may become key when an autofill popup closes Since panels have a promoted -[NSWindow level], OSX prefers them to browser windows when it needs to activate a window. One case this happens is whenever an autofill popup is dismissed (e.g. when tabbing between text fields on a web page). Sometimes, a panel window may be activated in this case since it thinks it was the "previous" key window, when actually it was a browser window. The "previous" key window case doesn't really seem to add much. So remove it. It means that a panel can lose focus (and not immediately regain it) when another panel window is closed, but that seems preferable to tabbing into a Hangouts chat window instead of a password field when a username autofill popup is dismissed. (Note it doesn't happen if a *browser* window is closed, since the native close button doesn't activate a window, but the non-native hangouts close button does). The "allow previous" logic was included in r127517 (to fix http://crbug.com/112038). Closing hangouts windows seems to be the only use case it supports, and I don't think that use case will be missed. BUG=546509, 112038 Committed: https://crrev.com/5b81d65c7382b3b855cbc5bd26a15cc68ef4c5e5 Cr-Commit-Position: refs/heads/master@{#369347} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5b81d65c7382b3b855cbc5bd26a15cc68ef4c5e5 Cr-Commit-Position: refs/heads/master@{#369347} |