Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(18)

Issue 660137: Allow users to close the find session and activate the current link via ctrl-... (Closed)

Created:
10 years, 5 months ago by Evan Stade
Modified:
9 years, 1 month ago
Reviewers:
tony, Finnur
CC:
chromium-reviews, jam+cc_chromium.org, brettw+cc_chromium.org, ben+cc_chromium.org, John Grabowski, darin-cc_chromium.org, PaweĊ‚ Hajdan Jr., pam+watch_chromium.org
Visibility:
Public.

Description

Allow users to close the find session and activate the current link via ctrl-enter. this only hooks up ctrl-enter on gtk for now, but adding it on windows/mac should be trivial webkit side, which needs to land first, is here: https://bugs.webkit.org/show_bug.cgi?id=35407 This also enables the FindInPageControllerTest tests on linux/gtk. BUG=29500 TEST=FindInPageControllerTest.ActivateLinkNavigatesPage Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41291

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Total comments: 12

Patch Set 4 : '' #

Total comments: 19

Patch Set 5 : '' #

Patch Set 6 : '' #

Patch Set 7 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+219 lines, -840 lines) Patch
M chrome/browser/cocoa/find_bar_cocoa_controller.mm View 1 2 3 4 5 6 3 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/find_bar_controller.h View 1 2 3 4 5 6 3 chunks +9 lines, -2 lines 0 comments Download
M chrome/browser/find_bar_controller.cc View 1 2 3 4 5 6 3 chunks +3 lines, -4 lines 0 comments Download
A + chrome/browser/find_bar_host_browsertest.cc View 4 5 6 17 chunks +59 lines, -8 lines 0 comments Download
M chrome/browser/gtk/find_bar_gtk.cc View 1 2 3 4 5 6 3 chunks +12 lines, -3 lines 0 comments Download
M chrome/browser/renderer_host/render_view_host.h View 1 2 3 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/renderer_host/render_view_host.cc View 1 2 3 4 5 1 chunk +18 lines, -2 lines 0 comments Download
M chrome/browser/tab_contents/tab_contents.h View 1 2 3 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/tab_contents/tab_contents.cc View 1 2 3 1 chunk +5 lines, -4 lines 0 comments Download
M chrome/browser/views/find_bar_host.cc View 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/views/find_bar_host_browsertest.cc View 1 2 3 1 chunk +0 lines, -794 lines 0 comments Download
M chrome/browser/views/find_bar_host_interactive_uitest.cc View 2 3 4 5 6 3 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/views/find_bar_view.cc View 1 2 3 4 5 6 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/chrome_tests.gypi View 3 chunks +5 lines, -1 line 0 comments Download
M chrome/common/render_messages.h View 2 chunks +46 lines, -0 lines 0 comments Download
M chrome/common/render_messages_internal.h View 1 2 3 4 1 chunk +3 lines, -4 lines 0 comments Download
M chrome/renderer/render_view.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/renderer/render_view.cc View 1 2 3 3 chunks +16 lines, -1 line 0 comments Download
A chrome/test/data/find_in_page/link.html View 1 chunk +8 lines, -0 lines 0 comments Download
M third_party/WebKit/WebKit/chromium/public/WebDocument.h View 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/WebKit/chromium/public/WebNode.h View 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/WebKit/chromium/src/WebDocument.cpp View 3 1 chunk +5 lines, -0 lines 0 comments Download
third_party/WebKit/WebKit/chromium/src/WebNode.cpp View 3 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
Evan Stade
Ignore the webkit changes; I only included them for the sake of the trybots.
10 years, 5 months ago (2010-03-06 01:08:16 UTC) #1
tony
How hard would it be to write an interactive UI test for this? http://codereview.chromium.org/660137/diff/2001/2011 File ...
10 years, 5 months ago (2010-03-08 00:24:26 UTC) #2
Finnur
I'd like to see some tests for this functionality, preferably a browser test, not an ...
10 years, 5 months ago (2010-03-08 22:43:58 UTC) #3
Evan Stade
test + enum added http://codereview.chromium.org/660137/diff/2001/2011 File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right): http://codereview.chromium.org/660137/diff/2001/2011#newcode138 chrome/browser/cocoa/find_bar_cocoa_controller.mm:138: tab_contents->StopFinding(true, false); // true = ...
10 years, 5 months ago (2010-03-10 01:39:59 UTC) #4
tony
LGTM. Finnur might want to take another look. http://codereview.chromium.org/660137/diff/5001/6004 File chrome/browser/find_bar_controller.h (right): http://codereview.chromium.org/660137/diff/5001/6004#newcode20 chrome/browser/find_bar_controller.h:20: kClearSelection, ...
10 years, 5 months ago (2010-03-10 05:27:13 UTC) #5
Finnur
http://codereview.chromium.org/660137/diff/5001/6010 File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right): http://codereview.chromium.org/660137/diff/5001/6010#newcode138 chrome/browser/cocoa/find_bar_cocoa_controller.mm:138: tab_contents->StopFinding(true, false); This function signature has changed now. http://codereview.chromium.org/660137/diff/5001/6010#newcode359 ...
10 years, 5 months ago (2010-03-10 07:37:41 UTC) #6
Evan Stade
http://codereview.chromium.org/660137/diff/5001/6010 File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right): http://codereview.chromium.org/660137/diff/5001/6010#newcode138 chrome/browser/cocoa/find_bar_cocoa_controller.mm:138: tab_contents->StopFinding(true, false); On 2010/03/10 07:37:41, Finnur wrote: > This ...
10 years, 5 months ago (2010-03-10 16:50:18 UTC) #7
Finnur
http://codereview.chromium.org/660137/diff/5001/6003 File chrome/browser/find_bar_controller.cc (right): http://codereview.chromium.org/660137/diff/5001/6003#newcode37 chrome/browser/find_bar_controller.cc:37: void FindBarController::EndFindSession(bool activate) { Not sure I agree that ...
10 years, 5 months ago (2010-03-10 17:28:11 UTC) #8
Evan Stade
http://codereview.chromium.org/660137/diff/5001/6003 File chrome/browser/find_bar_controller.cc (right): http://codereview.chromium.org/660137/diff/5001/6003#newcode37 chrome/browser/find_bar_controller.cc:37: void FindBarController::EndFindSession(bool activate) { On 2010/03/10 17:28:11, Finnur wrote: ...
10 years, 5 months ago (2010-03-10 17:40:44 UTC) #9
Finnur
http://codereview.chromium.org/660137/diff/5001/6003 File chrome/browser/find_bar_controller.cc (right): http://codereview.chromium.org/660137/diff/5001/6003#newcode37 chrome/browser/find_bar_controller.cc:37: void FindBarController::EndFindSession(bool activate) { Yes, thank you. Appreciate it.
10 years, 5 months ago (2010-03-10 17:43:43 UTC) #10
Evan Stade
updated
10 years, 5 months ago (2010-03-10 18:04:06 UTC) #11
Finnur
10 years, 5 months ago (2010-03-10 18:11:06 UTC) #12
LGTM - thanks!

Powered by Google App Engine
This is Rietveld 408576698