views: Fix progress bar tooltip text function.

views: Fix progress bar tooltip text function.

We should not handle DCHECK failures, read
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
for more information.

R=pkasting@chromium.org
TBR=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=162448

[tfarina, 2012-10-14 22:47:04]

Hi Peter, can you check this to me?

Thanks.

[Peter Kasting, 2012-10-15 19:03:10]

http://codereview.chromium.org/11155002/diff/3001/ui/views/controls/progress_...
File ui/views/controls/progress_bar.cc (left):

http://codereview.chromium.org/11155002/diff/3001/ui/views/controls/progress_...
ui/views/controls/progress_bar.cc:171: DCHECK(tooltip);
Why remove this?

http://codereview.chromium.org/11155002/diff/3001/ui/views/controls/progress_...
File ui/views/controls/progress_bar.cc (right):

http://codereview.chromium.org/11155002/diff/3001/ui/views/controls/progress_...
ui/views/controls/progress_bar.cc:171: if (tooltip_text_.empty())
This is a behavior change: the old code still assigned to |tooltip| in this
case.  Are we guaranteed the new behavior is safe and correct?  Note that the
old behavior is actually simpler to implement.

http://codereview.chromium.org/11155002/diff/3001/ui/views/controls/progress_...
File ui/views/controls/progress_bar_unittest.cc (right):

http://codereview.chromium.org/11155002/diff/3001/ui/views/controls/progress_...
ui/views/controls/progress_bar_unittest.cc:17: EXPECT_EQ(string16(), tooltip);
If you do preserve your behavior change, this EXPECT no longer really makes
sense.

[tfarina, 2012-10-15 19:15:49]

http://codereview.chromium.org/11155002/diff/3001/ui/views/controls/progress_...
File ui/views/controls/progress_bar.cc (right):

http://codereview.chromium.org/11155002/diff/3001/ui/views/controls/progress_...
ui/views/controls/progress_bar.cc:171: if (tooltip_text_.empty())
On 2012/10/15 19:03:10, Peter Kasting wrote:
> This is a behavior change: the old code still assigned to |tooltip| in this
> case.  Are we guaranteed the new behavior is safe
The impact would be minimal, ProgressBar control is used only by
ash/system/drive/tray_drive.cc and progress_bar_example.cc

> and correct?
It seems more correct to me and in pair with the implementation of button.cc and
label.cc

>  Note that the
> old behavior is actually simpler to implement.
I'm fine to back to it if you want, and just remove the handle of DCHECK
failure.

[Peter Kasting, 2012-10-15 19:19:54]

http://codereview.chromium.org/11155002/diff/3001/ui/views/controls/progress_...
File ui/views/controls/progress_bar.cc (right):

http://codereview.chromium.org/11155002/diff/3001/ui/views/controls/progress_...
ui/views/controls/progress_bar.cc:171: if (tooltip_text_.empty())
As long as the callers work fine with the behavioral change here I'm OK with it,
it's good to preserve consistency with button.cc and label.cc.

However I wonder if the better fix is to also change those (and any other
similar implementations of GetTooltipText()) to just always set the passed-in
string.

[Peter Kasting, 2012-10-15 19:21:50]

On 2012/10/15 19:19:54, Peter Kasting wrote:
> However I wonder if the better fix is to also change those (and any other
> similar implementations of GetTooltipText()) to just always set the passed-in
> string.

...in which case the signature could be simplified to:

  string16 GetTooltipText(const gfx::Point& p);

[tfarina, 2012-10-15 20:16:47]

Peter, could you check my changes again? Thanks.

[Peter Kasting, 2012-10-15 20:19:49]

LGTM.  Consider filing a bug to do the API simplification I suggested.

[tfarina, 2012-10-15 20:25:03]

On 2012/10/15 20:19:49, Peter Kasting wrote:
> LGTM.  Consider filing a bug to do the API simplification I suggested.

Yup. I filled crbug.com/155933 for this clean up task.

[tfarina, 2012-10-17 17:44:30]

TBRing sky...