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

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, 1 month ago by Daniel Erat
Modified:
3 years, 1 month 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
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, 1 month 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, 1 month ago (2012-05-16 21:10:52 UTC) #2
oshima (OOO until July 6th)
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, 1 month ago (2012-05-16 21:28:31 UTC) #3
sky (OOO until 7-20)
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, 1 month 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, 1 month ago (2012-05-16 21:42:24 UTC) #5
Daniel Erat
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, 1 month 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, 1 month ago (2012-05-16 22:11:34 UTC) #7
Daniel Erat
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, 1 month ago (2012-05-16 22:31:55 UTC) #8
oshima (OOO until July 6th)
LGTM with nits I still wonder why test fails on win, but I'll leave the ...
3 years, 1 month ago (2012-05-17 00:18:17 UTC) #9
Daniel Erat
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, 1 month 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, 1 month ago (2012-05-17 01:06:54 UTC) #11
sky (OOO until 7-20)
LGTM
3 years, 1 month ago (2012-05-17 03:51:31 UTC) #12
Daniel Erat
I've added a TODO about getting the test working on Windows and filed http://crbug.com/128556. Peter, ...
3 years, 1 month ago (2012-05-17 16:51:29 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/10386173/12005
3 years, 1 month ago (2012-05-17 17:45:35 UTC) #14
commit-bot: I haz the power
3 years, 1 month 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 1f9106d