Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556573002:
Fix SizeLabelToMinWidth() function such that no unnecessary space (Closed)
|
Created:
4 years ago by Jialiu Lin Modified:
4 years ago Reviewers:
Peter Kasting CC:
chromium-reviews, asanka, tfarina, dbeam+watch-downloads_chromium.org, Nathan Parker Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix SizeLabelToMinWidth() function such that no unnecessary space
between warning label and Discard button on download shelf.
Note, this bug has been there for more than 3 years. The string
change in crrev.com/2443343002 exposed this issue. The reason this bug
was only reproducible on Win is because Win's font is narrower than
other OSs' (a.k.a label with the same text on Win yields a smaller width)
BUG=668472
Committed: https://crrev.com/047611df69b7f337f96d4ce684d21cb69baeb0b8
Cr-Commit-Position: refs/heads/master@{#437563}
Patch Set 1 #Patch Set 2 : nit #
Total comments: 12
Patch Set 3 : address comments from pkasting@ #Patch Set 4 : nits #
Total comments: 22
Patch Set 5 : 2nd round comments #
Total comments: 10
Patch Set 6 : refactor code #Patch Set 7 : change to static function #
Total comments: 10
Patch Set 8 : address final nits #Patch Set 9 : change to ViewsTestBase #
Messages
Total messages: 79 (59 generated)
The CQ bit was checked by jialiul@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...
jialiul@chromium.org changed reviewers: + pkasting@chromium.org
Hi pkasting@, Could you take a look at this CL? Thanks!
The CQ bit was checked by jialiul@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can we unittest this function? It seems like this has no tests and could use some. https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:1021: // is malicious") from being broken up unnecessarily. Nit: Grammar and constant naming problems. How about just: If the label is already narrower than kDangerousTextWidth, we don't need to linebreak it, as it will fit on a single line. https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:1024: dangerous_download_label_sized_ = true; Nit: Instead of doing this here, set it immediately after the conditional atop the function, then we don't need to remember to set it on every exit. Another way: split this into two functions, like: void DownloadItemView::SizeLabelToMinWidth() { if (dangerous_download_label_sized_) return; dangerous_download_label_->SetSize(GetLabelSize()); dangerous_download_label_sized_ = true; } gfx::Size DownloadItemView::GetLabelSize() { ... most of the rest of this function The advantage of this is that you can just return the desired size immediately from places like this or the loop below, instead of having to do cleanup code. https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:1048: Nit: Use at most one blank line at a time within a function to minimize use of vertical whitespace. (See http://google.github.io/styleguide/cppguide.html#Vertical_Whitespace .) But here I would put zero blank lines as both these variables relate to each other. They also relate to the loop below, so I'd actually declare them below the comment instead of above it (with no blank lines around them). At that point it becomes clear that the loop could be written like this, reducing the body of the loop and scoping |min_size_width| as narrowly as possible (see http://google.github.io/styleguide/cppguide.html#Local_Variables ): for (gfx::Size min_width_size = size; iter.Advance(); min_width_size = size) { https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:1072: if (size.width() >= min_width_size.width()) { Changing this to >= seems worse. The effect is that when we have multiple linebreaks that result in the same minimum width, we'll choose to break after the first rather than the last. Ideally, we should place as much text on the first line as possible subject to the algorithmic constraints, so using '>' would be better at doing that. https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:1077: } else { Nit: No else after return
The CQ bit was checked by jialiul@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...
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, pkasting@! (And sorry about all these nits. I was in a rush.) I addressed all your comments, and added unit tests for this function. Let me know if you have further comments. Thanks, https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:1021: // is malicious") from being broken up unnecessarily. On 2016/12/06 05:48:20, Peter Kasting wrote: > Nit: Grammar and constant naming problems. How about just: > > If the label is already narrower than kDangerousTextWidth, we don't need to > linebreak it, as it will fit on a single line. Ah.. auto complete.. Thanks for catching this. https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:1024: dangerous_download_label_sized_ = true; On 2016/12/06 05:48:20, Peter Kasting wrote: > Nit: Instead of doing this here, set it immediately after the conditional atop > the function, then we don't need to remember to set it on every exit. > > Another way: split this into two functions, like: > > void DownloadItemView::SizeLabelToMinWidth() { > if (dangerous_download_label_sized_) > return; > dangerous_download_label_->SetSize(GetLabelSize()); > dangerous_download_label_sized_ = true; > } > > gfx::Size DownloadItemView::GetLabelSize() { > ... most of the rest of this function > > The advantage of this is that you can just return the desired size immediately > from places like this or the loop below, instead of having to do cleanup code. There are actually two things to be set in this function: the label size, and the text (with "\n" inserted to break the line). So it might make more sense to set them in a single function. https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:1048: On 2016/12/06 05:48:20, Peter Kasting wrote: > Nit: Use at most one blank line at a time within a function to minimize use of > vertical whitespace. (See > http://google.github.io/styleguide/cppguide.html#Vertical_Whitespace .) > > But here I would put zero blank lines as both these variables relate to each > other. They also relate to the loop below, so I'd actually declare them below > the comment instead of above it (with no blank lines around them). > > At that point it becomes clear that the loop could be written like this, > reducing the body of the loop and scoping |min_size_width| as narrowly as > possible (see http://google.github.io/styleguide/cppguide.html#Local_Variables > ): > > for (gfx::Size min_width_size = size; iter.Advance(); min_width_size = size) { Done. https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:1072: if (size.width() >= min_width_size.width()) { On 2016/12/06 05:48:20, Peter Kasting wrote: > Changing this to >= seems worse. The effect is that when we have multiple > linebreaks that result in the same minimum width, we'll choose to break after > the first rather than the last. Ideally, we should place as much text on the > first line as possible subject to the algorithmic constraints, so using '>' > would be better at doing that. Agree, change to '>' will place as much text on the first line as possible. https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:1077: } else { On 2016/12/06 05:48:20, Peter Kasting wrote: > Nit: No else after return Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jialiul@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jialiul@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jialiul@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...
Patchset #5 (id:120001) has been deleted
Patchset #4 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:1024: dangerous_download_label_sized_ = true; On 2016/12/06 23:42:33, Jialiu Lin wrote: > On 2016/12/06 05:48:20, Peter Kasting wrote: > > Nit: Instead of doing this here, set it immediately after the conditional atop > > the function, then we don't need to remember to set it on every exit. > > > > Another way: split this into two functions, like: > > > > void DownloadItemView::SizeLabelToMinWidth() { > > if (dangerous_download_label_sized_) > > return; > > dangerous_download_label_->SetSize(GetLabelSize()); > > dangerous_download_label_sized_ = true; > > } > > > > gfx::Size DownloadItemView::GetLabelSize() { > > ... most of the rest of this function > > > > The advantage of this is that you can just return the desired size immediately > > from places like this or the loop below, instead of having to do cleanup code. > > There are actually two things to be set in this function: the label size, and > the text (with "\n" inserted to break the line). So it might make more sense to > set them in a single function. Return a tuple? Return the text to use, and recalculate the preferred size on the caller side (wastes a bit of effort)? But maybe the answer is to set the text within said function, but return the size to be set by the parent, since we don't always need to reset the text. See later patchset comments... https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:1078: } If this loop terminates (either due to the condition failing or due to the break), we definitely won't have called SetSize() on |dangerous_download_label_|. (We might not have called SetText(), but I think the cases where we didn't are ones where the existing text was fine.) This seems like a bug (and probably one the tests should catch?). It also seems like more motivation to try and return the size from a separate function, so we can't miss this? https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view_unittest.cc (right): https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:13: class DownloadItemViewTest: public views::ViewsTestBase{ Nit: Space before ':' and '{' https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:19: view_ = new DownloadItemView(&download_, NULL); Nit: nullptr (2 places) https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:23: delete view_->dangerous_download_label_; This seems extremely dangerous. |view_| should be responsible for its own members. I suggest making DownloadItemView construct this and add it as a child in its own constructor. Not only will this eliminate the need to manage it here, it will eliminate the need to manually create it in SetDownloadWarningLabelAndResize() below. https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:24: delete view_; Eliminate the need for this by using a std::unique_ptr, or a member by value. https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:37: base::string16 GetLabelText() { Nit: Can be const https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:44: }; Nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:49: base::string16 label_text(base::ASCIIToUTF16("very short label")); Nit: Prefer = to () for initializing strings (see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... ) (several places) https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:54: // when we have multiple linebreaks that result in the same minimum width, we Nit: Initial caps https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:55: // should place as much text on the first line. Nit: as much text -> as much text as possible https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:67: TEST_F(DownloadItemViewTest, SizeLabelToMinWidth_MaliciousDownloadWarnings) { Nit: Comment about what specific part of the algorithm this tests? (2 places) (If it doesn't test anything specific, consider removing it, so we have the minimum spanning set of tests)
The CQ bit was checked by jialiul@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jialiul@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...
Patchset #5 (id:160001) has been deleted
Thanks! https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2556573002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:1024: dangerous_download_label_sized_ = true; On 2016/12/08 01:05:56, Peter Kasting wrote: > On 2016/12/06 23:42:33, Jialiu Lin wrote: > > On 2016/12/06 05:48:20, Peter Kasting wrote: > > > Nit: Instead of doing this here, set it immediately after the conditional > atop > > > the function, then we don't need to remember to set it on every exit. > > > > > > Another way: split this into two functions, like: > > > > > > void DownloadItemView::SizeLabelToMinWidth() { > > > if (dangerous_download_label_sized_) > > > return; > > > dangerous_download_label_->SetSize(GetLabelSize()); > > > dangerous_download_label_sized_ = true; > > > } > > > > > > gfx::Size DownloadItemView::GetLabelSize() { > > > ... most of the rest of this function > > > > > > The advantage of this is that you can just return the desired size > immediately > > > from places like this or the loop below, instead of having to do cleanup > code. > > > > There are actually two things to be set in this function: the label size, and > > the text (with "\n" inserted to break the line). So it might make more sense > to > > set them in a single function. > > Return a tuple? > > Return the text to use, and recalculate the preferred size on the caller side > (wastes a bit of effort)? > > But maybe the answer is to set the text within said function, but return the > size to be set by the parent, since we don't always need to reset the text. See > later patchset comments... Done. https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:1078: } On 2016/12/08 01:05:56, Peter Kasting wrote: > If this loop terminates (either due to the condition failing or due to the > break), we definitely won't have called SetSize() on > |dangerous_download_label_|. (We might not have called SetText(), but I think > the cases where we didn't are ones where the existing text was fine.) > > This seems like a bug (and probably one the tests should catch?). It also seems > like more motivation to try and return the size from a separate function, so we > can't miss this? Oops, you're right. https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view_unittest.cc (right): https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:13: class DownloadItemViewTest: public views::ViewsTestBase{ On 2016/12/08 01:05:56, Peter Kasting wrote: > Nit: Space before ':' and '{' Done. https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:19: view_ = new DownloadItemView(&download_, NULL); On 2016/12/08 01:05:56, Peter Kasting wrote: > Nit: nullptr (2 places) Done. https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:23: delete view_->dangerous_download_label_; On 2016/12/08 01:05:56, Peter Kasting wrote: > This seems extremely dangerous. |view_| should be responsible for its own > members. > > I suggest making DownloadItemView construct this and add it as a child in its > own constructor. Not only will this eliminate the need to manage it here, it > will eliminate the need to manually create it in > SetDownloadWarningLabelAndResize() below. Changed DownloadItemView constructor and destructor accordingly. Also changed SetdownloadWarningLabelAndResize() to SetText instead of creating a new label. https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:24: delete view_; On 2016/12/08 01:05:56, Peter Kasting wrote: > Eliminate the need for this by using a std::unique_ptr, or a member by value. Done. https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:37: base::string16 GetLabelText() { On 2016/12/08 01:05:56, Peter Kasting wrote: > Nit: Can be const Done. https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:44: }; On 2016/12/08 01:05:56, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:49: base::string16 label_text(base::ASCIIToUTF16("very short label")); On 2016/12/08 01:05:56, Peter Kasting wrote: > Nit: Prefer = to () for initializing strings (see > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... > ) (several places) Thanks for the pointer! https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:54: // when we have multiple linebreaks that result in the same minimum width, we On 2016/12/08 01:05:56, Peter Kasting wrote: > Nit: Initial caps Done. https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:55: // should place as much text on the first line. On 2016/12/08 01:05:56, Peter Kasting wrote: > Nit: as much text -> as much text as possible Done. https://codereview.chromium.org/2556573002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:67: TEST_F(DownloadItemViewTest, SizeLabelToMinWidth_MaliciousDownloadWarnings) { On 2016/12/08 01:05:56, Peter Kasting wrote: > Nit: Comment about what specific part of the algorithm this tests? (2 places) > > (If it doesn't test anything specific, consider removing it, so we have the > minimum spanning set of tests) Removed these two, and add another test to check how SizeLabelToMinWidth work with extremely long "word"
https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:228: dangerous_download_label_ = nullptr; This potentially double-frees. Views is expecting to own the pointer you pass to AddChildView(). You could get around this by marking it as "owned by parent" (and changing the class member to a unique_ptr), but the best way, I think, is to move _all_ the code for |dangerous_download_label_| to the constructor, including the AddChildView() call, and then either moving the other calls that add children there as well, or else modifying them so they insert their children in a specific place in the child list instead of at the end using AddChildViewAt(). https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:803: // need to load the icon. Does it actually hurt to load this icon in a test? If not, I would just do that. If so, maybe we should not key off |shelf_|, which seems like an indirect way to be checking that this is in a test? Should there be some kind of variable that directly says if we're in a test? https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.h (right): https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.h:219: // Called by SizeLabelToMinWidth() to compute the optimal size of dangerous Nit: of -> of the, or "of |dangerous_download_label_|" https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.h:221: gfx::Size GetLabelSize(); Nit: Seems like this should be named SetLabelTextAndGetSize() (or similar), so it's clear at a glance why it doesn't make sense for it to be const. https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view_unittest.cc (right): https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:38: DISALLOW_COPY_AND_ASSIGN(DownloadItemViewTest); Nit: Suggest a blank line above this
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by jialiul@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...
Thanks pkasting@ for your thorough review!
I wish the original author of this class thought about writing tests for it… :-)
The ui and behavior of DownloadItemView largely depend on download item’s
current state (and download shelf too). When download item updates (e.g. get
started, paused, blocked by safe browsing etc), DownloadItemView shows different
child views accordingly.
In other words, when DownloadItemView is constructed, whether
dangerous_download_label_ will be part of the view is not yet determined.
Given that, I think it makes more sense to keep the original logic ---
constructing dangerous_download_label_ in ShowWarningDialog() instead of in the
constructor, and destructing it in ClearWarningDialog() instead of in the
destructor.
So here is the refactor I did in this patch to minimize the change to the
existing logic:
(1) create another private constructor for tests only, which has the minimum
logic needed for unit tests.
(2) Move the construction and configuring of dangerous_download_label_ into a
helper function SetUpDangerousDownloadLabel(), such that I can call it in unit
test.
(3) Move the destruction of dangerous_download_label_ into a
ClearDangerousDownloadLabel() helper function.
— Similarly ClearWarningDialog()
(4) call these two new helper functions in unit_test (instead of explicitly
creating or deleting label)
https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/download/download_item_view.cc (right):
https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_item_view.cc:228:
dangerous_download_label_ = nullptr;
On 2016/12/08 05:10:23, Peter Kasting wrote:
> This potentially double-frees. Views is expecting to own the pointer you pass
> to AddChildView(). You could get around this by marking it as "owned by
parent"
> (and changing the class member to a unique_ptr), but the best way, I think, is
> to move _all_ the code for |dangerous_download_label_| to the constructor,
> including the AddChildView() call, and then either moving the other calls that
> add children there as well, or else modifying them so they insert their
children
> in a specific place in the child list instead of at the end using
> AddChildViewAt().
Ack.
https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_item_view.cc:803: // need to load the
icon.
On 2016/12/08 05:10:23, Peter Kasting wrote:
> Does it actually hurt to load this icon in a test?
>
> If not, I would just do that.
>
> If so, maybe we should not key off |shelf_|, which seems like an indirect way
to
> be checking that this is in a test? Should there be some kind of variable
that
> directly says if we're in a test?
Unfortunately LoadIcon function involves shelf_. So in the new constructor, this
function will not be called.
https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/download/download_item_view.h (right):
https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_item_view.h:219: // Called by
SizeLabelToMinWidth() to compute the optimal size of dangerous
On 2016/12/08 05:10:23, Peter Kasting wrote:
> Nit: of -> of the, or "of |dangerous_download_label_|"
Done.
https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_item_view.h:221: gfx::Size
GetLabelSize();
On 2016/12/08 05:10:23, Peter Kasting wrote:
> Nit: Seems like this should be named SetLabelTextAndGetSize() (or similar), so
> it's clear at a glance why it doesn't make sense for it to be const.
Done.
https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/download/download_item_view_unittest.cc (right):
https://codereview.chromium.org/2556573002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_item_view_unittest.cc:38:
DISALLOW_COPY_AND_ASSIGN(DownloadItemViewTest);
On 2016/12/08 05:10:23, Peter Kasting wrote:
> Nit: Suggest a blank line above this
Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
OK, let me step back and think. Your new test is really trying to test the behavior of SetLabelTextAndGetSize(). That function doesn't look like it accesses any instance variables except dangerous_download_label_. So you could make it a static member and pass the label in by pointer. Then the tests don't need to create a DownloadItemView at all, they can just create a label and call this function. That avoids refactoring how the class creates its members, or the need to add conditional checks around any variables. WDYT of this plan?
Patchset #7 (id:220001) has been deleted
The CQ bit was checked by jialiul@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...
On 2016/12/08 19:10:10, Peter Kasting wrote: > OK, let me step back and think. > > Your new test is really trying to test the behavior of SetLabelTextAndGetSize(). > That function doesn't look like it accesses any instance variables except > dangerous_download_label_. So you could make it a static member and pass the > label in by pointer. > > Then the tests don't need to create a DownloadItemView at all, they can just > create a label and call this function. That avoids refactoring how the class > creates its members, or the need to add conditional checks around any variables. > > WDYT of this plan? That works! Thanks! I change AdjustTextAndGetSize(..) into a static member function since it only interacts with the target label. Please take a look at the latest patch. Thanks!
LGTM https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:1028: // If the label is already narrower than kDangerousTextWidth, we don't need to Nit: Can we move the definition of kDangerousTextWidth to here? It's not immediately obvious otherwise it's really local to this conditional in this function. (This came up as I tried to figure out a better header comment for this function.) https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.h (right): https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.h:144: AdjustTextAndGetSize_VeryLongTextWithoutSpace); Nit: No strong feeling here, but if you had a single testcase, it would reduce the number of declarations here from three to one, and eliminate the need to repeatedly call SetMultiLine(true) in that function (because I assume you'd just use one Label, and SetText() it repeatedly). https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.h:225: // width) of a label and set its text accordingly. Nit: Maybe more detail would help, especially for people trying to read the test and see why it's testing what it is: Given a multiline |label|, decides whether it should be displayed on one line (if short), or broken across two lines. In the latter case, linebreaks near the middle of the string and sets the label's text accordingly. Returns the preferred size for the label. https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view_unittest.cc (right): https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:11: typedef testing::Test DownloadItemViewDangerousDownloadLabelTest; Nit: Prefer type alias ("using") to typedef. https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:17: base::string16 label_text = base::ASCIIToUTF16("very short label"); Nit: I suggest just "short", to minimize the chance this gets anywhere close to the wrap length.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
All final nits addressed. Thanks, pkasting@! I learnt a lot from your review. Cheers, https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:1028: // If the label is already narrower than kDangerousTextWidth, we don't need to On 2016/12/08 21:53:52, Peter Kasting wrote: > Nit: Can we move the definition of kDangerousTextWidth to here? It's not > immediately obvious otherwise it's really local to this conditional in this > function. > > (This came up as I tried to figure out a better header comment for this > function.) Agree, this is the only place using this const, I'll remove it altogether. https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.h (right): https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.h:144: AdjustTextAndGetSize_VeryLongTextWithoutSpace); On 2016/12/08 21:53:52, Peter Kasting wrote: > Nit: No strong feeling here, but if you had a single testcase, it would reduce > the number of declarations here from three to one, and eliminate the need to > repeatedly call SetMultiLine(true) in that function (because I assume you'd just > use one Label, and SetText() it repeatedly). merged into a single test case. https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.h:225: // width) of a label and set its text accordingly. On 2016/12/08 21:53:52, Peter Kasting wrote: > Nit: Maybe more detail would help, especially for people trying to read the test > and see why it's testing what it is: > > Given a multiline |label|, decides whether it should be displayed on one line > (if short), or broken across two lines. In the latter case, linebreaks near the > middle of the string and sets the label's text accordingly. Returns the > preferred size for the label. Done. https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view_unittest.cc (right): https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:11: typedef testing::Test DownloadItemViewDangerousDownloadLabelTest; On 2016/12/08 21:53:53, Peter Kasting wrote: > Nit: Prefer type alias ("using") to typedef. Done. https://codereview.chromium.org/2556573002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view_unittest.cc:17: base::string16 label_text = base::ASCIIToUTF16("very short label"); On 2016/12/08 21:53:53, Peter Kasting wrote: > Nit: I suggest just "short", to minimize the chance this gets anywhere close to > the wrap length. Done.
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2556573002/#ps260001 (title: "address final nits")
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
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jialiul@chromium.org
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2556573002/#ps280001 (title: "change to ViewsTestBase")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jialiul@chromium.org
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": 280001, "attempt_start_ts": 1481300433489110,
"parent_rev": "bafb367a462d360bbeed12796dd66787d29a2f06", "commit_rev":
"5f0dca9ace9731eae31debb123c0e6f355907eb2"}
Message was sent while issue was closed.
Description was changed from ========== Fix SizeLabelToMinWidth() function such that no unnecessary space between warning label and Discard button on download shelf. Note, this bug has been there for more than 3 years. The string change in crrev.com/2443343002 exposed this issue. The reason this bug was only reproducible on Win is because Win's font is narrower than other OSs' (a.k.a label with the same text on Win yields a smaller width) BUG=668472 ========== to ========== Fix SizeLabelToMinWidth() function such that no unnecessary space between warning label and Discard button on download shelf. Note, this bug has been there for more than 3 years. The string change in crrev.com/2443343002 exposed this issue. The reason this bug was only reproducible on Win is because Win's font is narrower than other OSs' (a.k.a label with the same text on Win yields a smaller width) BUG=668472 Review-Url: https://codereview.chromium.org/2556573002 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Fix SizeLabelToMinWidth() function such that no unnecessary space between warning label and Discard button on download shelf. Note, this bug has been there for more than 3 years. The string change in crrev.com/2443343002 exposed this issue. The reason this bug was only reproducible on Win is because Win's font is narrower than other OSs' (a.k.a label with the same text on Win yields a smaller width) BUG=668472 Review-Url: https://codereview.chromium.org/2556573002 ========== to ========== Fix SizeLabelToMinWidth() function such that no unnecessary space between warning label and Discard button on download shelf. Note, this bug has been there for more than 3 years. The string change in crrev.com/2443343002 exposed this issue. The reason this bug was only reproducible on Win is because Win's font is narrower than other OSs' (a.k.a label with the same text on Win yields a smaller width) BUG=668472 Committed: https://crrev.com/047611df69b7f337f96d4ce684d21cb69baeb0b8 Cr-Commit-Position: refs/heads/master@{#437563} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/047611df69b7f337f96d4ce684d21cb69baeb0b8 Cr-Commit-Position: refs/heads/master@{#437563} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
