Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
10 years, 1 month ago by altimofeev Modified:
9 years, 7 months ago Reviewers:
Finnur, DaveMoore, Ben Goodger (Google) CC:
chromium-reviews, ben+cc_chromium.org Visibility:
Public. |
DescriptionFixed focus requesting for the findbar.
BUG=chromium-os:8829
TEST=Run chromium. Use wrench menu to open 'Find' window. Notice that
focus is inplace.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66427
Patch Set 1 #Patch Set 2 : DCHECKs added #
Total comments: 8
Patch Set 3 : nits #
Total comments: 8
Patch Set 4 : nits + restoring #Patch Set 5 : extra request #Patch Set 6 : the nits #
Total comments: 8
Patch Set 7 : nits #Patch Set 8 : merge #
Created: 10 years, 1 month ago
Messages
Total messages: 15 (0 generated)
drive-by... http://codereview.chromium.org/5009001/diff/2001/chrome/browser/ui/views/find... File chrome/browser/ui/views/find_bar_view.cc (right): http://codereview.chromium.org/5009001/diff/2001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:227: DCHECK(find_text_); This is redundant and just adds cruft to debug builds. The code is going to crash on the next line anyway, you don't need a DCHECK to tell you that. http://codereview.chromium.org/5009001/diff/2001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:231: // has it's own focus tracker, so it will focus correct view on close. nit: I count four instances of the word 'the' being missing here (view, wrench menu, wrench menu, view). http://codereview.chromium.org/5009001/diff/2001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:232: DCHECK(find_text_->GetFocusManager()); Same here (don't need the DCHECK) http://codereview.chromium.org/5009001/diff/2001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:233: find_text_->GetFocusManager()->StoreFocusedView(); Make sure the following use case works (I think you haven't broken it, but just to make sure): 1) With two tabs open, open find window (ctrl-F or wrench) 2) Set focus to the omnibox 3) switch to the other tab 4) switch back. Focus should not be restored to the Find box. It should be restored to where focus was when you left the tab.
Thanks for the comments. http://codereview.chromium.org/5009001/diff/2001/chrome/browser/ui/views/find... File chrome/browser/ui/views/find_bar_view.cc (right): http://codereview.chromium.org/5009001/diff/2001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:227: DCHECK(find_text_); On 2010/11/15 14:49:12, Finnur wrote: > This is redundant and just adds cruft to debug builds. The code is going to > crash on the next line anyway, you don't need a DCHECK to tell you that. Done. http://codereview.chromium.org/5009001/diff/2001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:231: // has it's own focus tracker, so it will focus correct view on close. On 2010/11/15 14:49:12, Finnur wrote: > nit: I count four instances of the word 'the' being missing here (view, wrench > menu, wrench menu, view). Thanks, will try to improve my grammar. http://codereview.chromium.org/5009001/diff/2001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:232: DCHECK(find_text_->GetFocusManager()); On 2010/11/15 14:49:12, Finnur wrote: > Same here (don't need the DCHECK) Done. http://codereview.chromium.org/5009001/diff/2001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:233: find_text_->GetFocusManager()->StoreFocusedView(); On 2010/11/15 14:49:12, Finnur wrote: > Make sure the following use case works (I think you haven't broken it, but just > to make sure): > > 1) With two tabs open, open find window (ctrl-F or wrench) > 2) Set focus to the omnibox > 3) switch to the other tab > 4) switch back. > > Focus should not be restored to the Find box. It should be restored to where > focus was when you left the tab. I have tried your use case. The focus behaves as it should.
> I have tried your use case. The focus behaves as it should. Excellent. A few more minor nits... :) http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... File chrome/browser/ui/views/find_bar_view.cc (right): http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:228: // Storing is needed here because the view that has a focus before the wrench nit: 'a focus' implies there might be more than one focus (focai?) :) active at any given point. That's not the case. I would just drop the 'a' here since the meaning here is 'the focus'. http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:229: // menu activation will get the focus just after the wrench menu is closed. tiny-nit: 'focus' is enough, 'the focus' is not needed here. :) http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:230: // FindBar has it's own focus tracker, so it will focus a correct view on nit: 'The FindBar' nit: 'the correct view' http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:231: // the close. nit: 'the' is not appropriate here (the pendulum has swung the other way now). :)
Also note that the try bots complained about TabsRememberFocusFindInPage crashing. On 2010/11/15 16:20:30, Finnur wrote: > > I have tried your use case. The focus behaves as it should. > > Excellent. A few more minor nits... :) > > http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... > File chrome/browser/ui/views/find_bar_view.cc (right): > > http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... > chrome/browser/ui/views/find_bar_view.cc:228: // Storing is needed here because > the view that has a focus before the wrench > nit: 'a focus' implies there might be more than one focus (focai?) :) active at > any given point. That's not the case. I would just drop the 'a' here since the > meaning here is 'the focus'. > > http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... > chrome/browser/ui/views/find_bar_view.cc:229: // menu activation will get the > focus just after the wrench menu is closed. > tiny-nit: 'focus' is enough, 'the focus' is not needed here. :) > > http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... > chrome/browser/ui/views/find_bar_view.cc:230: // FindBar has it's own focus > tracker, so it will focus a correct view on > nit: 'The FindBar' > nit: 'the correct view' > > http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... > chrome/browser/ui/views/find_bar_view.cc:231: // the close. > nit: 'the' is not appropriate here (the pendulum has swung the other way now). > :)
> Also note that the try bots complained about TabsRememberFocusFindInPage crashing. Also I have found another use case, that didn't work. Focus wasn't returned to the previous owner on close (see the comments in the code). So an additional request was added to fix it. The fixes were added for OS_CHROMEOS only because in Linux, Windows ans Mac there is no problem with the focusing (Linux's FindBar implementation uses Gtk more directly). http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... File chrome/browser/ui/views/find_bar_view.cc (right): http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:228: // Storing is needed here because the view that has a focus before the wrench On 2010/11/15 16:20:30, Finnur wrote: > nit: 'a focus' implies there might be more than one focus (focai?) :) active at > any given point. That's not the case. I would just drop the 'a' here since the > meaning here is 'the focus'. Done. http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:229: // menu activation will get the focus just after the wrench menu is closed. On 2010/11/15 16:20:30, Finnur wrote: > tiny-nit: 'focus' is enough, 'the focus' is not needed here. :) Done. http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:230: // FindBar has it's own focus tracker, so it will focus a correct view on On 2010/11/15 16:20:30, Finnur wrote: > nit: 'The FindBar' > nit: 'the correct view' Done. http://codereview.chromium.org/5009001/diff/8001/chrome/browser/ui/views/find... chrome/browser/ui/views/find_bar_view.cc:231: // the close. On 2010/11/15 16:20:30, Finnur wrote: > nit: 'the' is not appropriate here (the pendulum has swung the other way now). > :) Done.
LGTM
http://codereview.chromium.org/5009001/diff/16001/chrome/browser/ui/views/fin... File chrome/browser/ui/views/find_bar_view.cc (right): http://codereview.chromium.org/5009001/diff/16001/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:232: // Restore focus to allow the find bar's external focus tracker save the view nit: save -> to save. http://codereview.chromium.org/5009001/diff/16001/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:234: // menu has got the focus). nit: got -> gotten. You can also use 'received'. http://codereview.chromium.org/5009001/diff/16001/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:236: // Request focus for the textfield. nit: this comment is redundant. http://codereview.chromium.org/5009001/diff/16001/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:243: // Request focus again since call to StoreFocusedView unfocuses the view. nit: call -> the call.
http://codereview.chromium.org/5009001/diff/16001/chrome/browser/ui/views/fin... File chrome/browser/ui/views/find_bar_view.cc (right): http://codereview.chromium.org/5009001/diff/16001/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:232: // Restore focus to allow the find bar's external focus tracker save the view On 2010/11/16 17:08:28, Finnur wrote: > nit: save -> to save. Done. http://codereview.chromium.org/5009001/diff/16001/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:234: // menu has got the focus). On 2010/11/16 17:08:28, Finnur wrote: > nit: got -> gotten. You can also use 'received'. I have settled on 'received'. http://codereview.chromium.org/5009001/diff/16001/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:236: // Request focus for the textfield. On 2010/11/16 17:08:28, Finnur wrote: > nit: this comment is redundant. Done. http://codereview.chromium.org/5009001/diff/16001/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:243: // Request focus again since call to StoreFocusedView unfocuses the view. On 2010/11/16 17:08:28, Finnur wrote: > nit: call -> the call. Done.
LGTM
Was any attempt to understand the bug in views made before this workaround was inserted? Why is this fix necessary on Linux only? Does the focus manager behave differently? Why?
1/ I tried to explain the reasons in the comments. Have you got any questions about the comments? 2/ This fix is (was?) necessary only for the views realization of the find bar. Chrome on linux used gtk directly, not via views, so it perfectly works with gtk wrench menu. Same for Windows and Mac OS. CrOS was the only one who used views realization with gtk menu. On Thu, Feb 3, 2011 at 8:28 PM, <ben@chromium.org> wrote: > Was any attempt to understand the bug in views made before this workaround > was > inserted? Why is this fix necessary on Linux only? Does the focus manager > behave > differently? Why? > > http://codereview.chromium.org/5009001/ >
Your comment does not explain why this functionality differs between OS_WIN and OS_CHROMEOS. See my original email for questions. You explain the circumstance in which the workaround is needed but not what the underlying sequence of events is. -Ben On Fri, Feb 4, 2011 at 1:45 AM, Alexey Timofeev <altimofeev@chromium.org>wrote: > 1/ I tried to explain the reasons in the comments. Have you got any > questions about the comments? > 2/ This fix is (was?) necessary only for the views realization of the find > bar. Chrome on linux used gtk directly, not via views, so it perfectly works > with gtk wrench menu. Same for Windows and Mac OS. CrOS was the only one who > used views realization with gtk menu. > > On Thu, Feb 3, 2011 at 8:28 PM, <ben@chromium.org> wrote: > >> Was any attempt to understand the bug in views made before this workaround >> was >> inserted? Why is this fix necessary on Linux only? Does the focus manager >> behave >> differently? Why? >> >> http://codereview.chromium.org/5009001/ >> > >
No further thoughts? -Ben On Fri, Feb 4, 2011 at 7:26 AM, Ben Goodger (Google) <ben@chromium.org>wrote: > Your comment does not explain why this functionality differs between OS_WIN > and OS_CHROMEOS. See my original email for questions. You explain the > circumstance in which the workaround is needed but not what the underlying > sequence of events is. > > -Ben > > > On Fri, Feb 4, 2011 at 1:45 AM, Alexey Timofeev <altimofeev@chromium.org>wrote: > >> 1/ I tried to explain the reasons in the comments. Have you got any >> questions about the comments? >> 2/ This fix is (was?) necessary only for the views realization of the find >> bar. Chrome on linux used gtk directly, not via views, so it perfectly works >> with gtk wrench menu. Same for Windows and Mac OS. CrOS was the only one who >> used views realization with gtk menu. >> >> On Thu, Feb 3, 2011 at 8:28 PM, <ben@chromium.org> wrote: >> >>> Was any attempt to understand the bug in views made before this >>> workaround was >>> inserted? Why is this fix necessary on Linux only? Does the focus manager >>> behave >>> differently? Why? >>> >>> http://codereview.chromium.org/5009001/ >>> >> >> >
> No further thoughts? The following happens in CrOS: 1. user opens wrench menu, wrench menu is created 2. user clicks on the find menu item 3. code that shows find bar is invoked 4. gtk sends focus to the previously focused widget (my guess is that it is related to asynchronous nature of gtk) Probably, in Windows, 3-rd and 4-th actions are swapped or there is no 4-th action at all. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
