Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2727253006:
Cleanup of omnibox_view_views_unittest.cc in advance of adding more to it. (Closed)
|
Created:
3 years, 9 months ago by Peter Kasting Modified:
3 years, 9 months ago Reviewers:
Mark P CC:
chromium-reviews, jdonnelly+watch_chromium.org, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCleanup of omnibox_view_views_unittest.cc in advance of adding more to it.
I added dividers between classes because I'm going to add a couple more classes,
and the code got hard to read without them. Similarly, I pulled function
definitions out-of-line in many cases because I'll be adding more functions or
more complexity, and without doing this it started getting cumbersome. (I left
at least one complex function inline because I'll just be deleting it anyway.)
Also switched to MakeUnique in a few places.
No intended functional change.
BUG=none
TEST=none
Review-Url: https://codereview.chromium.org/2727253006
Cr-Commit-Position: refs/heads/master@{#455430}
Committed: https://chromium.googlesource.com/chromium/src/+/6c4e7b6e12416d8e9fcb397b704f60d1a5f10db4
Patch Set 1 #
Total comments: 14
Patch Set 2 : Convert to loop #
Total comments: 4
Patch Set 3 : Review comments #
Dependent Patchsets: Messages
Total messages: 21 (13 generated)
The CQ bit was checked by pkasting@chromium.org to run a CQ dry run
pkasting@chromium.org changed reviewers: + mpearson@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
So, after I wrote all this, I found an easier way tonight to plumb the necessary changes to this test file without adding lots more classes and functionality. The ultimate goal is to land https://codereview.chromium.org/2735533002/ . So the question is, do you find the changes in this CL more readable as-is, or less readable? I think it's fair to say that the other CL could land with or without these, so we should make the decision here on its own merits. I personally tend to like de-inlining methods and adding class dividers and such, but that's definitely more personal preference than real style rules.
lgtm modulo comments On 2017/03/04 05:27:22, Peter Kasting wrote: > So, after I wrote all this, I found an easier way tonight to plumb the necessary > changes to this test file without adding lots more classes and functionality. > The ultimate goal is to land https://codereview.chromium.org/2735533002/ . > > So the question is, do you find the changes in this CL more readable as-is, or > less readable? I think it's fair to say that the other CL could land with or > without these, so we should make the decision here on its own merits. > > I personally tend to like de-inlining methods and adding class dividers and > such, but that's definitely more personal preference than real style rules. I generally think this is more readable. Most inlining feels heavy, and it's generally unnecessary with a good function-level comment. The only time I like a function inlined is when it's either extremely light or its meaning is not obvious / not explained well (so the code is necessary) or both. Mainly these are cases in which I would have trouble continually remembering when reading tests or (pretend) writing tests when I kept having trouble remembering what the function did, and then I'd have to make two hops (one back to the definition and one to the implementation) instead of one. Most of the inlining you did here I like. There are a few functions for which I'd prefer you left them inlined, marked as such in the changelist. In some of those cases, I'd probably be okay inlining them if there was a good function-level comment. That said, in some of these cases, the function-level comment might be as heavy as the code itself, so inlining is a better choice. I'm going to lgtm and trust you to make a reasonable decision given my opinions. cheers, mark https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:39: void CheckUpdatePopupCallInfo(size_t call_count, pref inline https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:48: void EmphasizeURLComponents() override; pref inline https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:52: void UpdatePopup() override; pref inline https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:68: Profile* profile_; nit: prefer blank line above here, to separate testing / result variables with context-type variables. https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:162: // Sets |new_text| as the text in the omnibox. nit: ... and emphasizes it appropriately.
The CQ bit was checked by pkasting@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL at the diff against the previous patch set. I did a couple more things here based on further work on the downstream patches: * Added "using gfx::Range" which shortened a number of places. * Converted TestingOmniboxViewViews to TestingOmniboxView, for similar reasons, and because it just reads less awkwardly to me. These made sense to put in this patch. Then I also did a few things which, in retrospect, I should have done as a separate CL, but I hope you'll forgive doing here because pulling them back out now might be more hassle than it's worth: * Converted |base_text_is_emphasized_| from a bool to a tristate enum, to make it clear when it hasn't been set yet. This is meaningful because test cases may or may not set this, and distinguishing "hasn't been set" against "was set to the default value (false)" is important. * Converted series of individual cases to a loop over an array. This will be useful down the road when I want to extend all the cases to test twice as many things. https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:39: void CheckUpdatePopupCallInfo(size_t call_count, On 2017/03/06 05:23:26, Mark P wrote: > pref inline I'd like to avoid inlining methods where the body is several lines. Same reply on UpdatePopup() below. https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:48: void EmphasizeURLComponents() override; On 2017/03/06 05:23:26, Mark P wrote: > pref inline I think this one makes sense, and the only reason I'm not doing it is because after my upcoming CLs, there will be no other inline methods that aren't "cheap accessors" and the like, so inlining this one would be inconsistent. https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:68: Profile* profile_; On 2017/03/06 05:23:27, Mark P wrote: > nit: prefer blank line above here, to separate testing / result variables with > context-type variables. Ignoring this because I'm about to rip this variable out entirely in the next CL. https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:162: // Sets |new_text| as the text in the omnibox. On 2017/03/06 05:23:26, Mark P wrote: > nit: ... and emphasizes it appropriately. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still lgtm, with comments (note comments appear on both patchsets) --mark https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:48: void EmphasizeURLComponents() override; On 2017/03/06 21:10:44, Peter Kasting wrote: > On 2017/03/06 05:23:26, Mark P wrote: > > pref inline > > I think this one makes sense, and the only reason I'm not doing it is because > after my upcoming CLs, there will be no other inline methods that aren't "cheap > accessors" and the like, so inlining this one would be inconsistent. Can you give it a better name or comment then? I would never guess EmphasizeURLComponents does something like UpdateTextStyle on a variable called text. https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:52: void UpdatePopup() override; On 2017/03/06 05:23:26, Mark P wrote: > pref inline The behavior is not what I'd expect an UpdatePopup() call to do, that's why I suggested it should be inline. If not inline, it should be commented. https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:68: Profile* profile_; On 2017/03/06 21:10:44, Peter Kasting wrote: > On 2017/03/06 05:23:27, Mark P wrote: > > nit: prefer blank line above here, to separate testing / result variables with > > context-type variables. > > Ignoring this because I'm about to rip this variable out entirely in the next > CL. Acknowledged. https://codereview.chromium.org/2727253006/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2727253006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:47: static BaseTextEmphasis to_emphasis(bool emphasize) { nit: awkward function name, hard to read outside of this declaration context. perhaps to_base_text_emphasis()? Also, consider moving this into a namespace, out of the class. Or--an alternative idea, less good in my opinion--simply instead have functions like SetEmphasis(bool) and VerifyEmphasis(bool) that looks at the local field directly. https://codereview.chromium.org/2727253006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:265: bool base_text_emphasized; nit: consider expected_ as appropriate to distinguish input from output.
https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:48: void EmphasizeURLComponents() override; On 2017/03/07 21:10:18, Mark P wrote: > On 2017/03/06 21:10:44, Peter Kasting wrote: > > On 2017/03/06 05:23:26, Mark P wrote: > > > pref inline > > > > I think this one makes sense, and the only reason I'm not doing it is because > > after my upcoming CLs, there will be no other inline methods that aren't > "cheap > > accessors" and the like, so inlining this one would be inconsistent. > > Can you give it a better name or comment then? > I would never guess EmphasizeURLComponents does something like UpdateTextStyle > on a variable called text. Since this is a virtual override, the names are set by the base class, which also gives the comments on what the functions generally do. Based on those, calling UpdateTextStyle() and passing in the current text of the control as the displayed text make sense to me. I dunno what to say here :( I don't love this name for this function, honestly. It's a bit overly-specific; really the names of these two methods should perhaps be swapped. However, I think making API changes to the core OmniboxView class is out of scope for this CL. If you feel strongly that this could be improved in some way (I don't feel strongly enough to do something about it myself), file a bug against me with whatever suggestion you might have, and I can try to implement it in a followup. https://codereview.chromium.org/2727253006/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:52: void UpdatePopup() override; On 2017/03/07 21:10:18, Mark P wrote: > On 2017/03/06 05:23:26, Mark P wrote: > > pref inline > > The behavior is not what I'd expect an UpdatePopup() call to do, that's why I > suggested it should be inline. If not inline, it should be commented. Done. https://codereview.chromium.org/2727253006/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2727253006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:47: static BaseTextEmphasis to_emphasis(bool emphasize) { On 2017/03/07 21:10:18, Mark P wrote: > nit: awkward function name, hard to read outside of this declaration context. > perhaps to_base_text_emphasis()? Done. > Also, consider moving this into a namespace, out of the class. It can be done if I also move the enum out of the class. I didn't want to do that, since the enum is only meaningful inside this specific class. Without that, there's no real way to do this, so left it as-is. https://codereview.chromium.org/2727253006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:265: bool base_text_emphasized; On 2017/03/07 21:10:18, Mark P wrote: > nit: consider expected_ as appropriate to distinguish input from output. Done.
The CQ bit was checked by pkasting@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2727253006/#ps40001 (title: "Review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488970654996570,
"parent_rev": "8af82bd5840c4c51bb1e31055446808abda90e7b", "commit_rev":
"6c4e7b6e12416d8e9fcb397b704f60d1a5f10db4"}
Message was sent while issue was closed.
Description was changed from ========== Cleanup of omnibox_view_views_unittest.cc in advance of adding more to it. I added dividers between classes because I'm going to add a couple more classes, and the code got hard to read without them. Similarly, I pulled function definitions out-of-line in many cases because I'll be adding more functions or more complexity, and without doing this it started getting cumbersome. (I left at least one complex function inline because I'll just be deleting it anyway.) Also switched to MakeUnique in a few places. No intended functional change. BUG=none TEST=none ========== to ========== Cleanup of omnibox_view_views_unittest.cc in advance of adding more to it. I added dividers between classes because I'm going to add a couple more classes, and the code got hard to read without them. Similarly, I pulled function definitions out-of-line in many cases because I'll be adding more functions or more complexity, and without doing this it started getting cumbersome. (I left at least one complex function inline because I'll just be deleting it anyway.) Also switched to MakeUnique in a few places. No intended functional change. BUG=none TEST=none Review-Url: https://codereview.chromium.org/2727253006 Cr-Commit-Position: refs/heads/master@{#455430} Committed: https://chromium.googlesource.com/chromium/src/+/6c4e7b6e12416d8e9fcb397b704f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6c4e7b6e12416d8e9fcb397b704f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
