aura: Really long tooltips should be wrapped not truncated.

aura: Really long tooltips should be wrapped not truncated.

BUG=119083
TEST=manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127975

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128177

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

[Daniel Erat, 2012-03-21 00:55:40]

Mostly looks good, with a question.

Also, mind making TrimTooltipToFit() a static method and adding some simple
tests to ash/tooltips/tooltip_controller_unittest.cc?

https://chromiumcodereview.appspot.com/9796004/diff/2001/ash/tooltips/tooltip...
File ash/tooltips/tooltip_controller.cc (right):

https://chromiumcodereview.appspot.com/9796004/diff/2001/ash/tooltips/tooltip...
ash/tooltips/tooltip_controller.cc:104: base::SplitStringDontTrim(*l, ' ',
&words);
is it okay to rebuild the string like this, or are we supposed to preserve
whitespace?

[sky, 2012-03-21 04:17:23]

Can you share the code in  TooltipManager::TrimTooltipToFit for this? Seems to
be nearly the same.

https://chromiumcodereview.appspot.com/9796004/diff/2001/ash/tooltips/tooltip...
File ash/tooltips/tooltip_controller.cc (right):

https://chromiumcodereview.appspot.com/9796004/diff/2001/ash/tooltips/tooltip...
ash/tooltips/tooltip_controller.cc:39: const int kTooltipMaxWidthPixels = 400;
On the windows side we initially had tooltips extending the width of the screen.
This seemed problematic so we did something like that. That was even more
problematic so we went back to the width of the screen. You should want to
restrict them?

[varunjain, 2012-03-21 04:48:37]

Added tests as well.

http://codereview.chromium.org/9796004/diff/2001/ash/tooltips/tooltip_control...
File ash/tooltips/tooltip_controller.cc (right):

http://codereview.chromium.org/9796004/diff/2001/ash/tooltips/tooltip_control...
ash/tooltips/tooltip_controller.cc:39: const int kTooltipMaxWidthPixels = 400;
On 2012/03/21 04:17:23, sky wrote:
> On the windows side we initially had tooltips extending the width of the
screen.
> This seemed problematic so we did something like that. That was even more
> problematic so we went back to the width of the screen. You should want to
> restrict them?

I restrict them here to the smaller of available screen space and
kTooltipMaxWidthPixels. How is that problematic.
AFAICT, tooltip size is restricted even in render_widget_host_view_win (so
tooltips on webpages on win are restricted).

http://codereview.chromium.org/9796004/diff/2001/ash/tooltips/tooltip_control...
ash/tooltips/tooltip_controller.cc:104: base::SplitStringDontTrim(*l, ' ',
&words);
On 2012/03/21 00:55:40, Daniel Erat wrote:
> is it okay to rebuild the string like this, or are we supposed to preserve
> whitespace?

whitespace is preserver when I use SplitStringDontTrim. I have also added a
testcase to test this.

[varunjain, 2012-03-21 04:51:48]

On 2012/03/21 04:17:23, sky wrote:
> Can you share the code in  TooltipManager::TrimTooltipToFit for this? Seems to
> be nearly the same.


TooltipManager::TrimTooltipToFit is different in that it does not do wrapping.
There is very little code there I can reuse here... but I did not want to
introduce that dependency for maybe 5-10 lines of code.

> 
>
https://chromiumcodereview.appspot.com/9796004/diff/2001/ash/tooltips/tooltip...
> File ash/tooltips/tooltip_controller.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9796004/diff/2001/ash/tooltips/tooltip...
> ash/tooltips/tooltip_controller.cc:39: const int kTooltipMaxWidthPixels = 400;
> On the windows side we initially had tooltips extending the width of the
screen.
> This seemed problematic so we did something like that. That was even more
> problematic so we went back to the width of the screen. You should want to
> restrict them?

[Daniel Erat, 2012-03-21 05:04:31]

lgtm

[sky, 2012-03-21 14:48:33]

LGTM

[commit-bot: I haz the power, 2012-03-21 15:03:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/9796004/6003

[commit-bot: I haz the power, 2012-03-21 16:11:16]

Change committed as 127975

[commit-bot: I haz the power, 2012-03-22 05:06:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/9796004/14002

[commit-bot: I haz the power, 2012-03-22 06:10:16]

Change committed as 128177