Display an info banner on the clear browsing data dialog if the browsing data was cleared within th…

Display an info banner on the clear browsing data dialog if the browsing data was cleared within the last 24h. The displayed info banner suggest to use the incognito mode instead.


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

[markusheintz_, 2013-01-31 18:08:52]

@Evan: Could you please review this CL? You'll find all necessary screenshots
and mocks in the referenced issue.

@Scott: Please review the changes in  chrome/browser/ui/browser_ui_prefs.cc
Please let me know if it is fine to TRB you or another ui OWNER in such cases.


Thanks a lot!

https://codereview.chromium.org/12092089/diff/11/chrome/browser/resources/opt...
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

https://codereview.chromium.org/12092089/diff/11/chrome/browser/resources/opt...
chrome/browser/resources/options/clear_browser_data_overlay.html:5: <div
class="info-bar">
Please note that I could probably get rid of on div and us the margin instead of
the padding.

Not sure if that makes much of a difference or if one way is preferred over the
other. I tried to follow the examples of the code in this file, where it looks
like padding is favored over using margin. But I may misinterpret this.

[sky, 2013-01-31 21:07:13]

LGTM

[markusheintz_, 2013-02-01 08:28:51]

@Scott: Thanks a lot.

Just seeing that Evan is OOO. Replacing Evan with James as reviewer.

@James: Please review this CL. You'll find all necessary screenshots and mocks
in the referenced bug.

Thanks a lot.

[James Hawkins, 2013-02-01 17:42:02]

Nice, clean CL.  Just a few comments.

https://codereview.chromium.org/12092089/diff/11/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12092089/diff/11/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8099: +        Psst! Incognito mode (<ph
name="INCOGNITO_MODE_SHORTCUT">$1<ex>Shift+Ctrl+N</ex></ph>) may come in useful
next time.
I don't think "may come in useful" is proper english grammar.  Consider:

* may be useful next time.
* may come in handy next time. <- Danger, colloquialism.

https://codereview.chromium.org/12092089/diff/11/chrome/browser/resources/opt...
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

https://codereview.chromium.org/12092089/diff/11/chrome/browser/resources/opt...
chrome/browser/resources/options/clear_browser_data_overlay.html:5: <div
class="info-bar">
On 2013/01/31 18:08:52, markusheintz_ wrote:
> Please note that I could probably get rid of on div and us the margin instead
of
> the padding.
> 
> Not sure if that makes much of a difference or if one way is preferred over
the
> other. I tried to follow the examples of the code in this file, where it looks
> like padding is favored over using margin. But I may misinterpret this.

Yeah, prefer margin instead of padding.

https://codereview.chromium.org/12092089/diff/11/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

https://codereview.chromium.org/12092089/diff/11/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:60:
base::TimeDelta::FromHours(24);
nit: Pull 24 out into a named constant.  Check around to see if we already have
one somewhere in the code base.

https://codereview.chromium.org/12092089/diff/11/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/clear_browser_data_handler.h (right):

https://codereview.chromium.org/12092089/diff/11/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/clear_browser_data_handler.h:21: void
UpdateInfoBannerVisibility();
Optional nit: Move non-inherited methods below the interface implementations.

[markusheintz_, 2013-02-01 18:43:17]

https://codereview.chromium.org/12092089/diff/11/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12092089/diff/11/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8099: +        Psst! Incognito mode (<ph
name="INCOGNITO_MODE_SHORTCUT">$1<ex>Shift+Ctrl+N</ex></ph>) may come in useful
next time.
On 2013/02/01 17:42:02, James Hawkins wrote:
> I don't think "may come in useful" is proper english grammar.  Consider:
> 
> * may be useful next time.
> * may come in handy next time. <- Danger, colloquialism.

Thanks for pointing this out. I changed it "handy". I sticked to the more
colloquial version as this is what UX suggested. I also had to modify this
string in anther place to stay consistent. This is fine according to the
comments on the bug.

https://codereview.chromium.org/12092089/diff/11/chrome/browser/resources/opt...
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

https://codereview.chromium.org/12092089/diff/11/chrome/browser/resources/opt...
chrome/browser/resources/options/clear_browser_data_overlay.html:5: <div
class="info-bar">
On 2013/02/01 17:42:02, James Hawkins wrote:
> On 2013/01/31 18:08:52, markusheintz_ wrote:
> > Please note that I could probably get rid of on div and us the margin
instead
> of
> > the padding.
> > 
> > Not sure if that makes much of a difference or if one way is preferred over
> the
> > other. I tried to follow the examples of the code in this file, where it
looks
> > like padding is favored over using margin. But I may misinterpret this.
> 
> Yeah, prefer margin instead of padding.

Done.

https://codereview.chromium.org/12092089/diff/11/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

https://codereview.chromium.org/12092089/diff/11/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:60:
base::TimeDelta::FromHours(24);
On 2013/02/01 17:42:02, James Hawkins wrote:
> nit: Pull 24 out into a named constant.  Check around to see if we already
have
> one somewhere in the code base.

After searching a bit in the code ("_PER_DAY", "kHoursPerDat", "HOURSPERDAY") it
looks like such a const or macro is only used in third party code.

So I create a const in the anonymous namespace here.

https://codereview.chromium.org/12092089/diff/11/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/clear_browser_data_handler.h (right):

https://codereview.chromium.org/12092089/diff/11/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/clear_browser_data_handler.h:21: void
UpdateInfoBannerVisibility();
On 2013/02/01 17:42:02, James Hawkins wrote:
> Optional nit: Move non-inherited methods below the interface implementations.

Done.

[James Hawkins, 2013-02-01 18:47:11]

LGTM with comment.

https://codereview.chromium.org/12092089/diff/3011/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

https://codereview.chromium.org/12092089/diff/3011/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:28: const uint
kHoursPerDay = 24;
Go ahead and keep this inside the method until we need to share it.

[markusheintz_, 2013-02-01 18:52:18]

FYI: I change the const type from uint to int64 in order to match the param type
of the TimeDelta::FromHours method.

Thanks a lot for the super fast responses and the review :).

https://codereview.chromium.org/12092089/diff/3011/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

https://codereview.chromium.org/12092089/diff/3011/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:28: const uint
kHoursPerDay = 24;
On 2013/02/01 18:47:11, James Hawkins wrote:
> Go ahead and keep this inside the method until we need to share it.

Done.

[commit-bot: I haz the power, 2013-02-01 19:02:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/12092089/2002

[commit-bot: I haz the power, 2013-02-01 22:35:45]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-02-04 08:06:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/12092089/2002

[commit-bot: I haz the power, 2013-02-04 09:19:33]

Change committed as 180372