Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10409034:
Fixing activation problem: Pages which refresh themselves should not get focus (Closed)
|
Created:
8 years, 7 months ago by Mr4D (OOO till 08-26) Modified:
8 years, 7 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFixing activation problem: Pages which refresh themselves should not get focus
BUG=122392
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138488
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed first review #
Total comments: 10
Patch Set 3 : Fix build problem #Patch Set 4 : Second review addressed #
Total comments: 11
Patch Set 5 : Third review addressed #
Total comments: 2
Patch Set 6 : Addressed fourth review #Patch Set 7 : Addressed fourth review #Messages
Total messages: 16 (0 generated)
Sky, please have a close look at this one. This makes perfectly sense - but - it might have some unwanted consequences somewhere else. Thanks!
http://codereview.chromium.org/10409034/diff/1/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/10409034/diff/1/chrome/browser/ui/browser.cc#n... chrome/browser/ui/browser.cc:2699: // When using ash it is not desirable to switch to a new window when the Why is this specific to ash? Additionally I'm not sure if this would cause wierdness if the request is not user_initiated but the window has focus. http://codereview.chromium.org/10409034/diff/1/chrome/browser/ui/browser.cc#n... chrome/browser/ui/browser.cc:3419: #if defined(USE_ASH) Whats the motivation for this?
See again. Note: As stated in the first review - I am not entirely sure that this is the best way of doing this and therefore you should have a close look. Let me know if you see a better suitable way to do this. http://codereview.chromium.org/10409034/diff/1/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/10409034/diff/1/chrome/browser/ui/browser.cc#n... chrome/browser/ui/browser.cc:2699: // When using ash it is not desirable to switch to a new window when the user_initiated: the problem is that if the page refreshes itself it is not user activated. So we do not want that the focus gets set to the "dynamic page". ASH: Please correct me if I am wrong, but with Chrome itself we do not have the problem of multiple windows like there. http://codereview.chromium.org/10409034/diff/1/chrome/browser/ui/browser.cc#n... chrome/browser/ui/browser.cc:3419: #if defined(USE_ASH) When this flag gets set, the LoadURL call further down will - many calls later - also trigger a SetFocus to the window. The idea here is that we only set the focus to the page if we are not on the current tab. I followed the whole call chain down, but this seemed to be the best location for a decision since further down the call chain the knowledge about who was trying to show the page is gone. If you have a better way of doing this - please let me know!
Sky, addressed as discussed. Please have another look!
http://codereview.chromium.org/10409034/diff/3002/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/10409034/diff/3002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:186: #include "ui/aura/window.h" You shouldn't need this. http://codereview.chromium.org/10409034/diff/3002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:2703: if (!user_initiated && Combine this with the Focus() request since that's what it is intended for. Also, I think you want: if (contents_is_selected && user_initiated && window()->IsActive()) ...->Focus() http://codereview.chromium.org/10409034/diff/3002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3420: #if defined(USE_ASH) Again, I think you want this every where. http://codereview.chromium.org/10409034/diff/3002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3424: (GetSelectedTabContentsWrapper() == nav_params.source_contents && Can you use source instead of nav_aprams.source_contents ? http://codereview.chromium.org/10409034/diff/3002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3425: window()->GetNativeHandle()->HasFocus())) window()->IsActive()
Addressed. Please have another look! http://codereview.chromium.org/10409034/diff/3002/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/10409034/diff/3002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:186: #include "ui/aura/window.h" On 2012/05/21 16:42:50, sky wrote: > You shouldn't need this. Done. http://codereview.chromium.org/10409034/diff/3002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:2703: if (!user_initiated && On 2012/05/21 16:42:50, sky wrote: > Combine this with the Focus() request since that's what it is intended for. > Also, I think you want: > > if (contents_is_selected && user_initiated && window()->IsActive()) > ...->Focus() Done. http://codereview.chromium.org/10409034/diff/3002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3420: #if defined(USE_ASH) On 2012/05/21 16:42:50, sky wrote: > Again, I think you want this every where. Done. http://codereview.chromium.org/10409034/diff/3002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3424: (GetSelectedTabContentsWrapper() == nav_params.source_contents && On 2012/05/21 16:42:50, sky wrote: > Can you use source instead of nav_aprams.source_contents ? Done. http://codereview.chromium.org/10409034/diff/3002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3425: window()->GetNativeHandle()->HasFocus())) On 2012/05/21 16:42:50, sky wrote: > window()->IsActive() Done.
http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:2700: // In case of multiple windows we do not want to switch windows when this is This comment is confusing. http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:2703: if (!user_initiated && I think you missed a comment from last round: "Combine this with the Focus() request since that's what it is intended for." Basicaly, I'm saying combine this with the if on 2708: if (user_initiated && contents_is_selected && window()->IsActive() http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3421: // If the current tab is initiating this, we don't want to update. You comment doesn't match what you're doing. http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3422: if (nav_params.window_action != browser::NavigateParams::NO_ACTION || I don't understand this first check. The default is NO_ACTION, so this should never be true. http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3423: params.disposition != CURRENT_TAB || Why are you checking this?
Thanks for your thorough review! Addressed - please have another look! http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:2703: if (!user_initiated && Ahh. In that case I have misunderstood. Done. http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3421: // If the current tab is initiating this, we don't want to update. On 2012/05/21 17:29:15, sky wrote: > You comment doesn't match what you're doing. Done. http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3422: if (nav_params.window_action != browser::NavigateParams::NO_ACTION || Done. Yes, that is correct (forgotten to remove that). http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3423: params.disposition != CURRENT_TAB || Because when a new thing (Popup / Window / Tab) gets opened we want to show it - even if the (what ever) was not yet active. In case of an automated update a "CURRENT_TAB" gets passed down which matches our requirement. I added a comment. But please let me know if you feel that that might be too limiting.
http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3423: params.disposition != CURRENT_TAB || On 2012/05/21 18:18:37, Mr4D wrote: > Because when a new thing (Popup / Window / Tab) gets opened we want to show it - > even if the (what ever) was not yet active. In case of an automated update a > "CURRENT_TAB" gets passed down which matches our requirement. I added a comment. > But please let me know if you feel that that might be too limiting. I believe Navigate takes care of that for you. See the comment and implementation where it sets the window_action to SHOW. http://codereview.chromium.org/10409034/diff/12004/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/10409034/diff/12004/chrome/browser/ui/browser.... chrome/browser/ui/browser.cc:2702: if (user_initiated && contents_is_selected && window()->IsActive()) Shouldn' this be an or, eg: if (content_is_selected && (user_initiated || window->IsActive()) ?
Addressed. Please have another look. http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/10409034/diff/3004/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3423: params.disposition != CURRENT_TAB || Okay, I removed the special check for the current_tab since the latter function will set the window_action as described. http://codereview.chromium.org/10409034/diff/12004/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/10409034/diff/12004/chrome/browser/ui/browser.... chrome/browser/ui/browser.cc:2702: if (user_initiated && contents_is_selected && window()->IsActive()) Done. (Wow good catch - somehow didn't think straight when applying your previous request).
LGTM I'm adding Ben in case he can think of any cases this would break.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/10409034/13003
Try job failure for 10409034-13003 (previous was lost) (retry) on win_rel for step "sync_unit_tests". It's a second try, previously, step "sync_unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/10409034/13003
Change committed as 138488 |
