History UI: Apply the pref that prohibits deleting browsing and download history.

Apply the pref that prohibits deleting browsing and download history.

When kDeletingBrowserHistoryEnabled is false, the UI elements to delete history items will be disabled, and the back-end calls will likewise be skipped (in case someone works around the UI, e.g. via the JavaScript console).

BUG=230401
TEST=browser_tests --gtest_filter=HistoryWebUIDeleteProhibitedTest.*

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

[Pam (message me for reviews), 2013-04-18 13:50:38]

James: ui/webui/history_ui.*
Pat: history.js, the tests, and the gypi

Thanks!

- Pam

[Patrick Dubroy, 2013-04-18 14:15:19]

https://codereview.chromium.org/14046016/diff/12003/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/14046016/diff/12003/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/history_ui.cc:563: if
(!profile->GetPrefs()->GetBoolean(prefs::kAllowDeletingBrowserHistory)) {
Maybe just OR this onto the other condition below?

https://codereview.chromium.org/14046016/diff/12003/chrome/test/data/webui/hi...
File chrome/test/data/webui/history_browsertest.js (right):

https://codereview.chromium.org/14046016/diff/12003/chrome/test/data/webui/hi...
chrome/test/data/webui/history_browsertest.js:140: typedefCppFixture:
'HistoryUIBrowserTest',
Sweeeet! Thanks for figuring out how to do this. This will be very helpful when
writing future tests.

https://codereview.chromium.org/14046016/diff/12003/chrome/test/data/webui/hi...
chrome/test/data/webui/history_browsertest.js:443: // The "remove" button should
be initially disabled.
Haha, I don't know what happened here. Thanks for fixing.

https://codereview.chromium.org/14046016/diff/12003/chrome/test/data/webui/hi...
chrome/test/data/webui/history_browsertest.js:773: var model = new
HistoryModel();
Why are you creating a new HistoryModel here? Why not use the one that's already
created? It's in a global var named "historyModel".

https://codereview.chromium.org/14046016/diff/12003/chrome/test/data/webui/hi...
chrome/test/data/webui/history_browsertest.js:777: testDone();
Hmmm, won't the test hang on failure then?

https://codereview.chromium.org/14046016/diff/12003/chrome/test/data/webui/hi...
File chrome/test/data/webui/history_ui_browsertest.cc (right):

https://codereview.chromium.org/14046016/diff/12003/chrome/test/data/webui/hi...
chrome/test/data/webui/history_ui_browsertest.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
Ditto on the "(c)"

https://codereview.chromium.org/14046016/diff/12003/chrome/test/data/webui/hi...
File chrome/test/data/webui/history_ui_browsertest.h (right):

https://codereview.chromium.org/14046016/diff/12003/chrome/test/data/webui/hi...
chrome/test/data/webui/history_ui_browsertest.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
Nit: I think we aren't supposed to be putting the "(c)" in anymore:
http://crbug.com/140977

[Pam (message me for reviews), 2013-04-18 14:41:44]

https://codereview.chromium.org/14046016/diff/12003/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/14046016/diff/12003/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/history_ui.cc:563: if
(!profile->GetPrefs()->GetBoolean(prefs::kAllowDeletingBrowserHistory)) {
On 2013/04/18 14:15:19, dubroy wrote:
> Maybe just OR this onto the other condition below?

Done. Done. Done. I'll spare you the spam. It's all done, except as commented
below. :)

https://codereview.chromium.org/14046016/diff/12003/chrome/test/data/webui/hi...
File chrome/test/data/webui/history_browsertest.js (right):

https://codereview.chromium.org/14046016/diff/12003/chrome/test/data/webui/hi...
chrome/test/data/webui/history_browsertest.js:777: testDone();
On 2013/04/18 14:15:19, dubroy wrote:
> Hmmm, won't the test hang on failure then?

In principle (I thought), if it "succeeds" (which is a failure), it throws the
error and ends the test. In practice, as you say, it times out. I thought it was
because I'm not stubbing out the history back-end call, so that gets wedged
somewhere. I couldn't figure out how to fix that. Or am I missing something
simpler?

Or do you mean on "failure", which is success for this test?
removeVisitsFromHistory calls its own callback on success, and deleteFailed on
failure. So the waitForCallback just above catches the case we expect.

[Patrick Dubroy, 2013-04-18 14:55:53]

https://codereview.chromium.org/14046016/diff/12003/chrome/test/data/webui/hi...
File chrome/test/data/webui/history_browsertest.js (right):

https://codereview.chromium.org/14046016/diff/12003/chrome/test/data/webui/hi...
chrome/test/data/webui/history_browsertest.js:777: testDone();
On 2013/04/18 14:41:44, Pam wrote:
> On 2013/04/18 14:15:19, dubroy wrote:
> > Hmmm, won't the test hang on failure then?
> 
> In principle (I thought), if it "succeeds" (which is a failure), it throws the
> error and ends the test. In practice, as you say, it times out. I thought it
was
> because I'm not stubbing out the history back-end call, so that gets wedged
> somewhere. I couldn't figure out how to fix that. Or am I missing something
> simpler?
> 
> Or do you mean on "failure", which is success for this test?
> removeVisitsFromHistory calls its own callback on success, and deleteFailed on
> failure. So the waitForCallback just above catches the case we expect.

All of these tests are required to call testDone() at the end of a successful
test. That's due to the "isAsync: true" at the top. I believe it's needed
exactly for cases like this, because control would hit the end of the test
function before the callback is called, so the test could never fail.

I think you can just do waitForCallback('deleteFailed', testDone) and it will do
what you want.

[Pam (message me for reviews), 2013-04-18 19:08:38]

Ready for another look, thanks.

[Patrick Dubroy, 2013-04-19 11:00:35]

On 2013/04/18 19:08:38, Pam wrote:
> Ready for another look, thanks.

lgtm

[Pam (message me for reviews), 2013-04-22 07:34:32]

James, could you please give owners approval for the webui/ files?

[Pam (message me for reviews), 2013-04-23 12:30:45]

On 2013/04/22 07:34:32, Pam wrote:
> James, could you please give owners approval for the webui/ files?

Ping?

[Pam (message me for reviews), 2013-04-24 10:44:04]

Hi Evan, could you please give owners approval for the webui/ files?

- Pam

[James Hawkins, 2013-04-24 21:12:20]

https://codereview.chromium.org/14046016/diff/36001/chrome/browser/resources/...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/14046016/diff/36001/chrome/browser/resources/...
chrome/browser/resources/history/history.js:131: if
(this.model_.createEditingCheckboxes) {
Should this param be less imperative and more behavioral, e.g.,
this.model_.somePrefAllowed?

[Pam (message me for reviews), 2013-04-25 07:53:24]

Thanks -- please take another look.

- Pam

https://codereview.chromium.org/14046016/diff/36001/chrome/browser/resources/...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/14046016/diff/36001/chrome/browser/resources/...
chrome/browser/resources/history/history.js:131: if
(this.model_.createEditingCheckboxes) {
On 2013/04/24 21:12:20, James Hawkins wrote:
> Should this param be less imperative and more behavioral, e.g.,
> this.model_.somePrefAllowed?

Renamed to editingEntriesAllowed.

[James Hawkins, 2013-04-25 17:14:59]

lgtm

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/14046016/9010

[commit-bot: I haz the power, 2013-04-25 20:54:23]

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

[commit-bot: I haz the power, 2013-04-26 06:32:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/14046016/9010

[commit-bot: I haz the power, 2013-04-26 07:40:53]

Change committed as 196655