Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
561983002:
Make OmniboxViewTest.BasicTextOperations Work on Linux (Closed)
DescriptionMake OmniboxViewTest.BasicTextOperations Work on Linux
BUG=408637
Committed: https://crrev.com/21e34f1cbcf1014a39a66443bdd1dac086e01109
Cr-Commit-Position: refs/heads/master@{#295168}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Mike's comment revision #Patch Set 3 : remove extraneous swap #Messages
Total messages: 15 (4 generated)
mpearson@chromium.org changed reviewers: + msw@chromium.org
Please take a look, thanks. --mark
Thanks for working on this! https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:764: #if defined(OS_WIN) || defined(OS_LINUX) I'd suggest using "#if defined(TOOLKIT_VIEWS)", but I think that'll break Mac, which now defines that symbol but goes on to instantiate an OmniboxViewMac... So, I guess this is fine (or moot; see below). https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:765: // In Views textfields (i.e., Windows and Linux), select-all highlights end nit: consider "Views textfields select-all in reverse to show the leading text." https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:767: std::swap(start, end); Hmm, does it make sense to replace these EXPECT_EQ calls of specific range bounds with something more robust, like checking the range length (if we're not particular about the reversal of the bounds)? https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:786: std::swap(start, end); Why is this swap needed? The bounds are expected to be the same.
https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:764: #if defined(OS_WIN) || defined(OS_LINUX) On 2014/09/10 23:17:27, msw wrote: > I'd suggest using "#if defined(TOOLKIT_VIEWS)", but I think that'll break Mac, > which now defines that symbol but goes on to instantiate an OmniboxViewMac... > So, I guess this is fine (or moot; see below). Acknowledged. https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:765: // In Views textfields (i.e., Windows and Linux), select-all highlights end On 2014/09/10 23:17:27, msw wrote: > nit: consider "Views textfields select-all in reverse to show the leading text." Sounds good to me. https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:767: std::swap(start, end); On 2014/09/10 23:17:27, msw wrote: > Hmm, does it make sense to replace these EXPECT_EQ calls of specific range > bounds with something more robust, like checking the range length (if we're not > particular about the reversal of the bounds)? I think it was a conscious user-interface decision to have the bounds reversed in certain cases. I think we should leave this test as it currently is, testing this property, not make it more forgiving of something that may be important. https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:786: std::swap(start, end); On 2014/09/10 23:17:27, msw wrote: > Why is this swap needed? The bounds are expected to be the same. It's not needed. I added it because I liked the consistency of having every GetSelectionBounds call annotated with this reminder.
https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:764: #if defined(OS_WIN) || defined(OS_LINUX) On 2014/09/10 23:17:27, msw wrote: > I'd suggest using "#if defined(TOOLKIT_VIEWS)", but I think that'll break Mac, > which now defines that symbol but goes on to instantiate an OmniboxViewMac... > So, I guess this is fine (or moot; see below). Acknowledged. https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:765: // In Views textfields (i.e., Windows and Linux), select-all highlights end On 2014/09/10 23:17:27, msw wrote: > nit: consider "Views textfields select-all in reverse to show the leading text." Sounds good to me. https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:767: std::swap(start, end); On 2014/09/10 23:17:27, msw wrote: > Hmm, does it make sense to replace these EXPECT_EQ calls of specific range > bounds with something more robust, like checking the range length (if we're not > particular about the reversal of the bounds)? I think it was a conscious user-interface decision to have the bounds reversed in certain cases. I think we should leave this test as it currently is, testing this property, not make it more forgiving of something that may be important. https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:786: std::swap(start, end); On 2014/09/10 23:17:27, msw wrote: > Why is this swap needed? The bounds are expected to be the same. It's not needed. I added it because I liked the consistency of having every GetSelectionBounds call annotated with this reminder.
lgtm, hoping you'll remove the empty range reversal. https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:786: std::swap(start, end); On 2014/09/15 22:54:53, Mark P wrote: > On 2014/09/10 23:17:27, msw wrote: > > Why is this swap needed? The bounds are expected to be the same. > > It's not needed. I added it because I liked the consistency of having every > GetSelectionBounds call annotated with this reminder. But there's no reason to expect consistency in the reversal of selection ranges that aren't for select-all, when that quirk is only needed for cases that are select-all. In fact, it's [trivially] incorrect. I think you should remove this.
https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/561983002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:786: std::swap(start, end); On 2014/09/16 18:59:44, msw wrote: > On 2014/09/15 22:54:53, Mark P wrote: > > On 2014/09/10 23:17:27, msw wrote: > > > Why is this swap needed? The bounds are expected to be the same. > > > > It's not needed. I added it because I liked the consistency of having every > > GetSelectionBounds call annotated with this reminder. > > But there's no reason to expect consistency in the reversal of selection ranges > that aren't for select-all, when that quirk is only needed for cases that are > select-all. In fact, it's [trivially] incorrect. I think you should remove this. Done.
The CQ bit was checked by mpearson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/561983002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mpearson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/561983002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as 38e1d2b78dea90aac60555a6a4a4258fdbfeb597
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/21e34f1cbcf1014a39a66443bdd1dac086e01109 Cr-Commit-Position: refs/heads/master@{#295168} |
