Chromium Code Reviews
Help | Chromium Project | Sign in
(8)

Issue 10386173: ash: Select omnibox text on mouse up instead of down. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years ago by Daniel Erat (OOO 5-14 to 5-22)
Modified:
3 years ago
CC:
chromium-reviews, tfarina, James Su, penghuang+watch_chromium.org, yusukes+watch_chromium.org
Visibility:
Public.

Description

ash: Select omnibox text on mouse up instead of down. Defer selection of the text in an unfocused omnibox until the release event. Otherwise, all of the text starting from the left side is selected when the user is trying to highlight just a portion of the URL. BUG=128126 TEST=added, also did manual testing Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137711

Patch Set 1 #

Patch Set 2 : add offset to clicks #

Patch Set 3 : only run tests for USE_AURA #

Total comments: 15

Patch Set 4 : apply review feedback #

Patch Set 5 : set select_all member to false on ineligible press #

Total comments: 4

Patch Set 6 : add consts to test #

Patch Set 7 : merge and add test todo #

Messages

Total messages: 15 (0 generated)
Daniel Erat (OOO 5-14 to 5-22)
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc#newcode1524 chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1524: #if defined(USE_AURA) I'd prefer to run these for TOOLKIT_VIEWS, ...
3 years ago (2012-05-16 21:06:01 UTC) #1
Peter Kasting
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omnibox/omnibox_view_views.cc File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omnibox/omnibox_view_views.cc#newcode323 chrome/browser/ui/views/omnibox/omnibox_view_views.cc:323: if (event.IsOnlyLeftMouseButton() && This check isn't right, because we ...
3 years ago (2012-05-16 21:10:52 UTC) #2
oshima (OOO May 22nd)
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc#newcode1524 chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1524: #if defined(USE_AURA) On 2012/05/16 21:06:02, Daniel Erat wrote: > ...
3 years ago (2012-05-16 21:28:31 UTC) #3
sky
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc#newcode1537 chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1537: ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset); You should wrap functions you call ...
3 years ago (2012-05-16 21:39:56 UTC) #4
Peter Kasting
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc#newcode1524 chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1524: #if defined(USE_AURA) On 2012/05/16 21:06:02, Daniel Erat wrote: > ...
3 years ago (2012-05-16 21:42:24 UTC) #5
Daniel Erat (OOO 5-14 to 5-22)
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc#newcode1537 chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1537: ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset); On 2012/05/16 21:39:56, sky wrote: > ...
3 years ago (2012-05-16 22:03:10 UTC) #6
Peter Kasting
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omnibox/omnibox_view_views.cc File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omnibox/omnibox_view_views.cc#newcode323 chrome/browser/ui/views/omnibox/omnibox_view_views.cc:323: if (event.IsOnlyLeftMouseButton() && On 2012/05/16 22:03:10, Daniel Erat wrote: ...
3 years ago (2012-05-16 22:11:34 UTC) #7
Daniel Erat (OOO 5-14 to 5-22)
On 2012/05/16 22:11:34, Peter Kasting wrote: > http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omnibox/omnibox_view_views.cc > File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): > > http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omnibox/omnibox_view_views.cc#newcode323 ...
3 years ago (2012-05-16 22:31:55 UTC) #8
oshima (OOO May 22nd)
LGTM with nits I still wonder why test fails on win, but I'll leave the ...
3 years ago (2012-05-17 00:18:17 UTC) #9
Daniel Erat (OOO 5-14 to 5-22)
http://codereview.chromium.org/10386173/diff/2002/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/10386173/diff/2002/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc#newcode1273 chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1273: BrowserView* GetBrowserView() { On 2012/05/17 00:18:18, oshima wrote: > ...
3 years ago (2012-05-17 00:28:53 UTC) #10
Peter Kasting
I'm busy WK sheriffing, so I don't have time to investigate Windows test results right ...
3 years ago (2012-05-17 01:06:54 UTC) #11
sky
LGTM
3 years ago (2012-05-17 03:51:31 UTC) #12
Daniel Erat (OOO 5-14 to 5-22)
I've added a TODO about getting the test working on Windows and filed http://crbug.com/128556. Peter, ...
3 years ago (2012-05-17 16:51:29 UTC) #13
I haz the power - commit-bot
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/10386173/12005
3 years ago (2012-05-17 17:45:35 UTC) #14
I haz the power - commit-bot
3 years ago (2012-05-17 19:43:01 UTC) #15
Change committed as 137711
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld ec887be