Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
532143002:
Close the find bar on reload. (Closed)
DescriptionClose the find bar when we reloads the page or we navigate to new page.
As we are reloading the page then it should reset context and find bar should get disappeared.
BUG=19311, 410160
Committed: https://crrev.com/c13b60492eef039eff71d3cd40513acd47053448
Cr-Commit-Position: refs/heads/master@{#293860}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Changes as per reviewser comments. #
Total comments: 10
Patch Set 3 : changes done as per comments. #
Total comments: 4
Patch Set 4 : changes as per reviewers comments. #
Total comments: 13
Patch Set 5 : Changes as per reviewer comments. #Patch Set 6 : nit chnages done. #
Messages
Total messages: 30 (5 generated)
deepak.m1@samsung.com changed reviewers: + finnur@chromium.org
PTAL
https://codereview.chromium.org/532143002/diff/1/chrome/browser/ui/find_bar/f... File chrome/browser/ui/find_bar/find_bar_controller.cc (right): https://codereview.chromium.org/532143002/diff/1/chrome/browser/ui/find_bar/f... chrome/browser/ui/find_bar/find_bar_controller.cc:173: find_tab_helper->find_text()); I'm actually not convinced that if we clear the find results that we should keep the box open. I mean, it is not showing much useful info at that point so we might as well close it, right? I suspect we can just do: if (reload || navigation_to_different_page) EndFindSession()
On 2014/09/03 13:22:48, Finnur wrote: > https://codereview.chromium.org/532143002/diff/1/chrome/browser/ui/find_bar/f... > File chrome/browser/ui/find_bar/find_bar_controller.cc (right): > > https://codereview.chromium.org/532143002/diff/1/chrome/browser/ui/find_bar/f... > chrome/browser/ui/find_bar/find_bar_controller.cc:173: > find_tab_helper->find_text()); > I'm actually not convinced that if we clear the find results that we should keep > the box open. I mean, it is not showing much useful info at that point so we > might as well close it, right? > > I suspect we can just do: > if (reload || navigation_to_different_page) > EndFindSession() I understand your meaning then I have 2 queries.. 1) What should be the case in https://codereview.chromium.org/498133002/ review and issue 88235. 2) Their is no use of if (content::PageTransitionStripQualifier(transition_type) != content::PAGE_TRANSITION_RELOAD) { } That separates that weather the transition happen to the new page or not. 3) The purpose of clearing the count is so that user knows what he have searched last and just by selecting up or down navigation button he will get matched items in the page. Please let me know your opition, so that we can conclude on this issue as well as https://codereview.chromium.org/498133002/ review also.
> 1) What should be the case in https://codereview.chromium.org/498133002/ review > and issue 88235. I don't want to conflate two separate issues. They may be changing the same file, but we should not make one dependent on the other. This fix has a higher chance of success, because it is a smaller fix and I'm not sure what the fate of the other one is as the problem there seems minor and the fix a bit hacky. I meant to test it yesterday to see if it had any side effects, but ran out of time. > 2) Their is no use of > if (content::PageTransitionStripQualifier(transition_type) != > content::PAGE_TRANSITION_RELOAD) { > } > That separates that weather the transition happen to the new page or not. No. I don't think that's correct. This is what I had in mind (leaving out the comments for clarity): if (content::PageTransitionStripQualifier(transition_type) != content::PAGE_TRANSITION_RELOAD) { if (commit_details->is_navigation_to_different_page()) EndFindSession(kKeepSelectionOnPage, kClearResultsInFindBox); } else { EndFindSession(kKeepSelectionOnPage, kClearResultsInFindBox); } This converts into: if (content::PageTransitionStripQualifier(transition_type) == content::PAGE_TRANSITION_RELOAD || commit_details->is_navigation_to_different_page()) EndFindSession(kKeepSelectionOnPage, kClearResultsInFindBox); Which is equivalent to the pseudo-code I provided: if (reload || navigation_to_different_page) EndFindSession() > > 3) The purpose of clearing the count is so that user knows what he have searched > last and just by selecting up or down navigation button he will get matched > items in the page. > > Please let me know your opition, so that we can conclude on this issue as well > as https://codereview.chromium.org/498133002/ review also. My point was that you'd be telling the users something they already know (what they searched for) and that's not very useful.
On 2014/09/04 10:22:19, Finnur wrote: > > 1) What should be the case in https://codereview.chromium.org/498133002/ > review > > and issue 88235. > > I don't want to conflate two separate issues. They may be changing the same > file, but we should not make one dependent on the other. This fix has a higher > chance of success, because it is a smaller fix and I'm not sure what the fate of > the other one is as the problem there seems minor and the fix a bit hacky. I > meant to test it yesterday to see if it had any side effects, but ran out of > time. > > > 2) Their is no use of > > if (content::PageTransitionStripQualifier(transition_type) != > > content::PAGE_TRANSITION_RELOAD) { > > } > > That separates that weather the transition happen to the new page or not. > > No. I don't think that's correct. This is what I had in mind (leaving out the > comments for clarity): > > if (content::PageTransitionStripQualifier(transition_type) != > content::PAGE_TRANSITION_RELOAD) { > if (commit_details->is_navigation_to_different_page()) > EndFindSession(kKeepSelectionOnPage, kClearResultsInFindBox); > } else { > EndFindSession(kKeepSelectionOnPage, kClearResultsInFindBox); > } > > This converts into: > > if (content::PageTransitionStripQualifier(transition_type) == > content::PAGE_TRANSITION_RELOAD || > commit_details->is_navigation_to_different_page()) > EndFindSession(kKeepSelectionOnPage, kClearResultsInFindBox); > > Which is equivalent to the pseudo-code I provided: > > if (reload || navigation_to_different_page) > EndFindSession() > > > > > 3) The purpose of clearing the count is so that user knows what he have > searched > > last and just by selecting up or down navigation button he will get matched > > items in the page. > > > > Please let me know your opition, so that we can conclude on this issue as well > > as https://codereview.chromium.org/498133002/ review also. > > My point was that you'd be telling the users something they already know (what > they searched for) and that's not very useful. Thanks for clearing my doubt, I will do change and upload changes again.
Changes done as suggested & verified. PTAL https://codereview.chromium.org/532143002/diff/1/chrome/browser/ui/find_bar/f... File chrome/browser/ui/find_bar/find_bar_controller.cc (right): https://codereview.chromium.org/532143002/diff/1/chrome/browser/ui/find_bar/f... chrome/browser/ui/find_bar/find_bar_controller.cc:173: find_tab_helper->find_text()); On 2014/09/03 13:22:47, Finnur wrote: > I'm actually not convinced that if we clear the find results that we should keep > the box open. I mean, it is not showing much useful info at that point so we > might as well close it, right? > > I suspect we can just do: > if (reload || navigation_to_different_page) > EndFindSession() Done.
https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... File chrome/browser/ui/find_bar/find_bar_controller.cc (right): https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_controller.cc:161: if (content::PageTransitionStripQualifier(transition_type) == We prefer: if (x && (y || z)) ... over ... if (x) if (y || z) https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_controller.cc:165: } nit: We don't use braces around one-line if bodies, even if the conditional is multi-line. https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:842: IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, FindDisappearOnNavigate) { Should probably be named FindDisappearOnNavigateAndReload now. https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:866: EXPECT_FALSE(fully_visible); With this change, I'm not sure the added test (at the bottom of this file) buys us anything. I think the changes to this test probably cover what needs to be covered. https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:1565: return; Why is this needed if it is not needed in FindDisappearOnNavigation?
Changes done as suggested. PTAL. https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... File chrome/browser/ui/find_bar/find_bar_controller.cc (right): https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_controller.cc:161: if (content::PageTransitionStripQualifier(transition_type) == On 2014/09/04 12:45:10, Finnur wrote: > We prefer: > > if (x && (y || z)) > > ... over ... > > if (x) > if (y || z) Done. https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_controller.cc:165: } On 2014/09/04 12:45:10, Finnur wrote: > nit: We don't use braces around one-line if bodies, even if the conditional is > multi-line. Done. https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:842: IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, FindDisappearOnNavigate) { On 2014/09/04 12:45:10, Finnur wrote: > Should probably be named FindDisappearOnNavigateAndReload now. Done. https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:866: EXPECT_FALSE(fully_visible); On 2014/09/04 12:45:10, Finnur wrote: > With this change, I'm not sure the added test (at the bottom of this file) buys > us anything. I think the changes to this test probably cover what needs to be > covered. Acknowledged. https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:1565: return; On 2014/09/04 12:45:10, Finnur wrote: > Why is this needed if it is not needed in FindDisappearOnNavigation? Acknowledged.
On 2014/09/04 13:18:15, deepak.m1 wrote: > Changes done as suggested. > PTAL. > > https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... > File chrome/browser/ui/find_bar/find_bar_controller.cc (right): > > https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... > chrome/browser/ui/find_bar/find_bar_controller.cc:161: if > (content::PageTransitionStripQualifier(transition_type) == > On 2014/09/04 12:45:10, Finnur wrote: > > We prefer: > > > > if (x && (y || z)) > > > > ... over ... > > > > if (x) > > if (y || z) > > Done. > > https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... > chrome/browser/ui/find_bar/find_bar_controller.cc:165: } > On 2014/09/04 12:45:10, Finnur wrote: > > nit: We don't use braces around one-line if bodies, even if the conditional is > > multi-line. > > Done. > > https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... > File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): > > https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... > chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:842: > IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, FindDisappearOnNavigate) { > On 2014/09/04 12:45:10, Finnur wrote: > > Should probably be named FindDisappearOnNavigateAndReload now. > > Done. > > https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... > chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:866: > EXPECT_FALSE(fully_visible); > On 2014/09/04 12:45:10, Finnur wrote: > > With this change, I'm not sure the added test (at the bottom of this file) > buys > > us anything. I think the changes to this test probably cover what needs to be > > covered. > > Acknowledged. > > https://codereview.chromium.org/532143002/diff/20001/chrome/browser/ui/find_b... > chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:1565: return; > On 2014/09/04 12:45:10, Finnur wrote: > > Why is this needed if it is not needed in FindDisappearOnNavigation? > > Acknowledged. PTAL
LGTM with one nit. https://codereview.chromium.org/532143002/diff/40001/chrome/browser/ui/find_b... File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): https://codereview.chromium.org/532143002/diff/40001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:843: // and on navigation to new page also Findbar should get disappeared. This comment is confusing. Suggest: // This test will verify that the Find bar should disappear // on reload and on navigation.
https://codereview.chromium.org/532143002/diff/40001/chrome/browser/ui/find_b... File chrome/browser/ui/find_bar/find_bar_controller.cc (right): https://codereview.chromium.org/532143002/diff/40001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_controller.cc:158: // We hide the FindInPage window when the user reload the page or Forgot one thing: s/reload/reloads/
On 2014/09/04 16:00:45, Finnur wrote: > https://codereview.chromium.org/532143002/diff/40001/chrome/browser/ui/find_b... > File chrome/browser/ui/find_bar/find_bar_controller.cc (right): > > https://codereview.chromium.org/532143002/diff/40001/chrome/browser/ui/find_b... > chrome/browser/ui/find_bar/find_bar_controller.cc:158: // We hide the FindInPage > window when the user reload the page or > Forgot one thing: > > s/reload/reloads/ Changes done. and I have already signed CLA also. As I have below comment in https://codereview.chromium.org/484133002/ review. "deepak.m1@samsung.com is listed as an authorized contributor in Samsung's list (on the Corporate Signers tab). So, looks fine to me."
PTAL https://codereview.chromium.org/532143002/diff/40001/chrome/browser/ui/find_b... File chrome/browser/ui/find_bar/find_bar_controller.cc (right): https://codereview.chromium.org/532143002/diff/40001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_controller.cc:158: // We hide the FindInPage window when the user reload the page or On 2014/09/04 16:00:45, Finnur wrote: > Forgot one thing: > > s/reload/reloads/ Done. https://codereview.chromium.org/532143002/diff/40001/chrome/browser/ui/find_b... File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): https://codereview.chromium.org/532143002/diff/40001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:843: // and on navigation to new page also Findbar should get disappeared. On 2014/09/04 15:58:51, Finnur wrote: > This comment is confusing. Suggest: > > // This test will verify that the Find bar should disappear > // on reload and on navigation. Done.
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.m1@samsung.com/532143002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
deepak.m1@samsung.com changed reviewers: + msw@chromium.org
@msw, Finnur has given LGTM. PTAL
On 2014/09/05 06:35:51, deepak.m1 wrote: Finnur has given LGTM. @msw PTAL
The bug only notes that the match count should be cleared; please include Finnur's rationale to hide the find bar altogether in your CL description. Please also cite BUG=19311,410160 (or close one issue as a duplicate of the other). https://codereview.chromium.org/532143002/diff/60001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/532143002/diff/60001/AUTHORS#newcode102 AUTHORS:102: Deepak Mittal <deepak.m1@samsung.com> Cool, you're listed as an authorized contributor in Samsung's list (on the Corporate Signers tab) as per http://www.chromium.org/developers/contributing-code/external-contributor-che... https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... File chrome/browser/ui/find_bar/find_bar_controller.cc (right): https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_controller.cc:158: // We hide the FindInPage window when the user reloads the page or nit: consider "Hide the find bar on reload or navigation." https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:842: // This test will verify that the Find bar should disappear nit: consider: "Verify that the find bar is hidden on reload and navigation." https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:845: FindDisappearOnNavigateAndReload) { nit: HideFindOnNavigateAndReload fits on the line above; otherwise consider HideFindBarOnNavigateAndReload. https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:860: // Reload and make sure the Find window goes away. nit: s/Find/find https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:871: // Opening Findbar again. nit: "Open the find bar again." Or remove this superfluous comment.
Also, please update the cl title to "Close the find bar on reload." or similar.
Thanks for review. All changes done as suggested. @PTAL https://codereview.chromium.org/532143002/diff/60001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/532143002/diff/60001/AUTHORS#newcode102 AUTHORS:102: Deepak Mittal <deepak.m1@samsung.com> On 2014/09/08 18:16:54, msw wrote: > Cool, you're listed as an authorized contributor in Samsung's list (on the > Corporate Signers tab) as per > http://www.chromium.org/developers/contributing-code/external-contributor-che... Done. https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... File chrome/browser/ui/find_bar/find_bar_controller.cc (right): https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_controller.cc:158: // We hide the FindInPage window when the user reloads the page or On 2014/09/08 18:16:55, msw wrote: > nit: consider "Hide the find bar on reload or navigation." Done. https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:842: // This test will verify that the Find bar should disappear On 2014/09/08 18:16:55, msw wrote: > nit: consider: "Verify that the find bar is hidden on reload and navigation." Done. https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:845: FindDisappearOnNavigateAndReload) { On 2014/09/08 18:16:55, msw wrote: > nit: HideFindOnNavigateAndReload fits on the line above; otherwise consider > HideFindBarOnNavigateAndReload. Done. https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:860: // Reload and make sure the Find window goes away. On 2014/09/08 18:16:55, msw wrote: > nit: s/Find/find Done. https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:871: // Opening Findbar again. On 2014/09/08 18:16:55, msw wrote: > nit: "Open the find bar again." Or remove this superfluous comment. Done.
lgtm with a nit https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:871: // Opening Findbar again. On 2014/09/09 03:56:06, deepak.m1 wrote: > On 2014/09/08 18:16:55, msw wrote: > > nit: "Open the find bar again." Or remove this superfluous comment. > > Done. You missed this nit; the comment should read. "Open the find bar again."
On 2014/09/09 04:25:03, msw wrote: > lgtm with a nit > > https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... > File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): > > https://codereview.chromium.org/532143002/diff/60001/chrome/browser/ui/find_b... > chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:871: // Opening Findbar > again. > On 2014/09/09 03:56:06, deepak.m1 wrote: > > On 2014/09/08 18:16:55, msw wrote: > > > nit: "Open the find bar again." Or remove this superfluous comment. > > > > Done. > > You missed this nit; the comment should read. "Open the find bar again." done, Thanks a lot.
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.m1@samsung.com/532143002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as ede2c9d9b41180f6fa0dac24fbe0cf1d57b06250
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c13b60492eef039eff71d3cd40513acd47053448 Cr-Commit-Position: refs/heads/master@{#293860} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
