Commit d0da7919 authored by rbpotter's avatar rbpotter Committed by Commit Bot

History Web UI: Cleanup removeVisits chrome.send call

- Move wrapping logic around chrome.send('removeVisits') into
history_list.js instead of browser_service.js
- Convert this call to use cr.sendWithPromise instead of calling JS
directly from C++ after deletion succeeds/fails
- Delete browser service test, since the logic it is testing is no
longer in the browser service. The test is replaced by adding checks
for the arguments passed to removeVisits() in the history list tests.

Bug: 1022212
Change-Id: I7290d97472b34a0a233ac7921c18c2a298481a3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1947956Reviewed-by: default avatarDemetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721378}
parent 8dda8888
......@@ -63,6 +63,7 @@
<history-toolbar id="toolbar"
has-drawer="[[hasDrawer_]]"
has-more-results="[[!queryResult_.info.finished]]"
pending-delete="[[pendingDelete_]]"
query-info="[[queryResult_.info]]"
querying="[[queryState_.querying]]"
search-term="[[queryState_.searchTerm]]"
......@@ -85,6 +86,7 @@
items="{{items}}">
<history-list id="history" query-state="[[queryState_]]"
searched-term="[[queryResult_.info.term]]"
pending-delete="{{pendingDelete_}}"
query-result="[[queryResult_]]"
path="history">
</history-list>
......
......@@ -112,6 +112,9 @@ Polymer({
value: loadTimeData.getBoolean('isUserSignedIn'),
},
/** @private */
pendingDelete_: Boolean,
toolbarShadow_: {
type: Boolean,
reflectToAttribute: true,
......@@ -301,7 +304,7 @@ Polymer({
/** @private */
onDeleteCommand_: function() {
if (this.$.toolbar.count == 0) {
if (this.$.toolbar.count == 0 || this.pendingDelete_) {
return;
}
this.deleteSelected();
......
......@@ -9,49 +9,24 @@
cr.define('history', function() {
class BrowserService {
constructor() {
/** @private {Array<!HistoryEntry>} */
this.pendingDeleteItems_ = null;
/** @private {PromiseResolver} */
this.pendingDeletePromise_ = null;
}
historyLoaded() {
chrome.send('historyLoaded');
}
/**
* @param {!Array<!HistoryEntry>} items
* @return {Promise<!Array<!HistoryEntry>>}
* @param {!string} url
*/
deleteItems(items) {
if (this.pendingDeleteItems_ != null) {
// There's already a deletion in progress, reject immediately.
return new Promise(function(resolve, reject) {
reject(items);
});
}
const removalList = items.map(function(item) {
return {
url: item.url,
timestamps: item.allTimestamps,
};
});
this.pendingDeleteItems_ = items;
this.pendingDeletePromise_ = new PromiseResolver();
chrome.send('removeVisits', removalList);
return this.pendingDeletePromise_.promise;
removeBookmark(url) {
chrome.send('removeBookmark', [url]);
}
/**
* @param {!string} url
* @param {!Array<!HistoryEntry>} removalList
* @return {!Promise} Promise that is resolved when items are deleted
* successfully or rejected when deletion fails.
*/
removeBookmark(url) {
chrome.send('removeBookmark', [url]);
removeVisits(removalList) {
return cr.sendWithPromise('removeVisits', removalList);
}
/**
......@@ -113,23 +88,6 @@ cr.define('history', function() {
chrome.send('metricsHandler:recordTime', [histogram, time]);
}
/** @param {boolean} successful */
resolveDelete(successful) {
if (this.pendingDeleteItems_ == null ||
this.pendingDeletePromise_ == null) {
return;
}
if (successful) {
this.pendingDeletePromise_.resolve(this.pendingDeleteItems_);
} else {
this.pendingDeletePromise_.reject(this.pendingDeleteItems_);
}
this.pendingDeleteItems_ = null;
this.pendingDeletePromise_ = null;
}
menuPromoShown() {
chrome.send('menuPromoShown');
}
......@@ -167,17 +125,3 @@ cr.define('history', function() {
return {BrowserService: BrowserService};
});
/**
* Called by the history backend when deletion was succesful.
*/
function deleteComplete() {
history.BrowserService.getInstance().resolveDelete(true);
}
/**
* Called by the history backend when the deletion failed.
*/
function deleteFailed() {
history.BrowserService.getInstance().resolveDelete(false);
}
......@@ -88,7 +88,7 @@
$i18n{moreFromSite}
</button>
<button id="menuRemoveButton" class="dropdown-item"
hidden="[[!canDeleteHistory_]]"
hidden="[[!canDeleteHistory_]]" disabled="[[pendingDelete]]"
on-click="onRemoveFromHistoryTap_">
$i18n{removeFromHistory}
</button>
......
......@@ -48,6 +48,12 @@ Polymer({
lastSelectedIndex: Number,
pendingDelete: {
notify: true,
type: Boolean,
value: false,
},
/** @type {!QueryState} */
queryState: Object,
......@@ -235,19 +241,20 @@ Polymer({
* @private
*/
deleteSelected_: function() {
assert(!this.pendingDelete);
const toBeRemoved = Array.from(this.selectedItems.values())
.map((index) => this.get(`historyData_.${index}`));
history.BrowserService.getInstance()
.deleteItems(toBeRemoved)
.then((items) => {
this.removeItemsByIndex_(Array.from(this.selectedItems));
this.fire('unselect-all');
if (this.historyData_.length == 0) {
// Try reloading if nothing is rendered.
this.fire('query-history', false);
}
});
this.deleteItems_(toBeRemoved).then(() => {
this.pendingDelete = false;
this.removeItemsByIndex_(Array.from(this.selectedItems));
this.fire('unselect-all');
if (this.historyData_.length == 0) {
// Try reloading if nothing is rendered.
this.fire('query-history', false);
}
});
},
/**
......@@ -373,18 +380,37 @@ Polymer({
this.closeMenu_();
},
/**
* @param {!Array<!HistoryEntry>} items
* @return {!Promise}
* @private
*/
deleteItems_: function(items) {
const removalList = items.map(item => ({
url: item.url,
timestamps: item.allTimestamps,
}));
this.pendingDelete = true;
return history.BrowserService.getInstance().removeVisits(removalList);
},
/** @private */
onRemoveFromHistoryTap_: function() {
const browserService = history.BrowserService.getInstance();
browserService.recordAction('EntryMenuRemoveFromHistory');
assert(!this.pendingDelete);
const menu = assert(this.$.sharedMenu.getIfExists());
const itemData = this.actionMenuModel_;
browserService.deleteItems([itemData.item]).then((items) => {
this.deleteItems_([itemData.item]).then(() => {
// This unselect-all resets the toolbar when deleting a selected item
// and clears selection state which can be invalid if items move
// around during deletion.
// TODO(tsergeant): Make this automatic based on observing list
// modifications.
this.pendingDelete = false;
this.fire('unselect-all');
this.removeItemsByIndex_([itemData.index]);
......@@ -398,7 +424,7 @@ Polymer({
if (item) {
item.focusOnMenuButton();
}
});
}, 1);
const browserService = history.BrowserService.getInstance();
browserService.recordHistogram(
......
......@@ -54,6 +54,7 @@
<cr-toolbar-selection-overlay show="[[itemsSelected_]]"
delete-label="$i18n{delete}"
cancel-label="$i18n{cancel}"
delete-disabled="[[pendingDelete]]"
selection-label="[[numberOfItemsSelected_(count)]]"
on-clear-selected-items="clearSelectedItems"
on-delete-selected-items="deleteSelectedItems">
......
......@@ -21,6 +21,8 @@ Polymer({
value: false,
},
pendingDelete: Boolean,
// The most recent term entered in the search field. Updated incrementally
// as the user types.
searchTerm: {
......
......@@ -331,32 +331,42 @@ void BrowsingHistoryHandler::HandleQueryHistoryContinuation(
}
void BrowsingHistoryHandler::HandleRemoveVisits(const base::ListValue* args) {
std::vector<BrowsingHistoryService::HistoryEntry> items_to_remove;
items_to_remove.reserve(args->GetSize());
for (auto it = args->begin(); it != args->end(); ++it) {
const base::DictionaryValue* deletion = nullptr;
base::string16 url;
const base::ListValue* timestamps = nullptr;
CHECK(args->GetList().size() == 2);
const base::Value& callback_id = args->GetList()[0];
CHECK(remove_visits_callback_.empty());
remove_visits_callback_ = callback_id.GetString();
std::vector<BrowsingHistoryService::HistoryEntry> items_to_remove;
const base::Value& items = args->GetList()[1];
base::Value::ConstListView list = items.GetList();
items_to_remove.reserve(list.size());
for (size_t i = 0; i < list.size(); ++i) {
// Each argument is a dictionary with properties "url" and "timestamps".
if (!(it->GetAsDictionary(&deletion) && deletion->GetString("url", &url) &&
deletion->GetList("timestamps", &timestamps))) {
if (!list[i].is_dict()) {
NOTREACHED() << "Unable to extract arguments";
return;
}
DCHECK_GT(timestamps->GetSize(), 0U);
const std::string* url_ptr = list[i].FindStringKey("url");
const base::Value* timestamps_ptr = list[i].FindListKey("timestamps");
if (!url_ptr || !timestamps_ptr) {
NOTREACHED() << "Unable to extract arguments";
return;
}
base::Value::ConstListView timestamps = timestamps_ptr->GetList();
DCHECK_GT(timestamps.size(), 0U);
BrowsingHistoryService::HistoryEntry entry;
entry.url = GURL(url);
entry.url = GURL(*url_ptr);
double timestamp;
for (auto ts_iterator = timestamps->begin();
ts_iterator != timestamps->end(); ++ts_iterator) {
if (!ts_iterator->GetAsDouble(&timestamp)) {
for (size_t ts_index = 0; ts_index < timestamps.size(); ++ts_index) {
if (!timestamps[ts_index].is_double() && !timestamps[ts_index].is_int()) {
NOTREACHED() << "Unable to extract visit timestamp.";
continue;
}
base::Time visit_time = base::Time::FromJsTime(timestamp);
base::Time visit_time =
base::Time::FromJsTime(timestamps[ts_index].GetDouble());
entry.all_timestamps.insert(visit_time.ToInternalValue());
}
......@@ -417,11 +427,16 @@ void BrowsingHistoryHandler::OnQueryComplete(
}
void BrowsingHistoryHandler::OnRemoveVisitsComplete() {
web_ui()->CallJavascriptFunctionUnsafe("deleteComplete");
CHECK(!remove_visits_callback_.empty());
ResolveJavascriptCallback(base::Value(remove_visits_callback_),
base::Value());
remove_visits_callback_.clear();
}
void BrowsingHistoryHandler::OnRemoveVisitsFailed() {
web_ui()->CallJavascriptFunctionUnsafe("deleteFailed");
CHECK(!remove_visits_callback_.empty());
RejectJavascriptCallback(base::Value(remove_visits_callback_), base::Value());
remove_visits_callback_.clear();
}
void BrowsingHistoryHandler::HistoryDeleted() {
......
......@@ -81,6 +81,8 @@ class BrowsingHistoryHandler : public content::WebUIMessageHandler,
base::OnceClosure query_history_continuation_;
std::string remove_visits_callback_;
base::WeakPtrFactory<BrowsingHistoryHandler> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(BrowsingHistoryHandler);
......
......@@ -172,18 +172,33 @@ TEST_F(BrowsingHistoryHandlerTest, ObservingWebHistoryDeletions) {
BrowsingHistoryHandlerWithWebUIForTesting handler(web_ui());
handler.RegisterMessages();
// Simulate an ongoing delete request.
handler.browsing_history_service_->has_pending_delete_request_ = true;
web_history_service()->ExpireHistoryBetween(
std::set<GURL>(), base::Time(), base::Time::Max(),
base::Bind(
&history::BrowsingHistoryService::RemoveWebHistoryComplete,
handler.browsing_history_service_->weak_factory_.GetWeakPtr()),
PARTIAL_TRAFFIC_ANNOTATION_FOR_TESTS);
// First, send historyLoaded so that JS will be allowed.
base::Value init_args(base::Value::Type::LIST);
init_args.Append("history-loaded-callback-id");
handler.HandleHistoryLoaded(&base::Value::AsListValue(init_args));
// Simulate a delete request.
base::Value args(base::Value::Type::LIST);
args.Append("remove-visits-callback-id");
base::Value to_remove(base::Value::Type::LIST);
base::Value visit(base::Value::Type::DICTIONARY);
visit.SetStringKey("url", "https://www.google.com");
base::Value timestamps(base::Value::Type::LIST);
timestamps.Append(12345678.0);
visit.SetKey("timestamps", std::move(timestamps));
to_remove.Append(std::move(visit));
args.Append(std::move(to_remove));
handler.HandleRemoveVisits(&base::Value::AsListValue(args));
EXPECT_EQ(3U, web_ui()->call_data().size());
EXPECT_EQ("deleteComplete", web_ui()->call_data().back()->function_name());
const content::TestWebUI::CallData& data = *web_ui()->call_data().back();
EXPECT_EQ("cr.webUIResponse", data.function_name());
std::string callback_id;
ASSERT_TRUE(data.arg1()->GetAsString(&callback_id));
EXPECT_EQ("remove-visits-callback-id", callback_id);
bool success = false;
ASSERT_TRUE(data.arg2()->GetAsBoolean(&success));
ASSERT_TRUE(success);
}
// When history sync is not active, we don't listen to WebHistoryService
......
// Copyright 2016 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
suite('BrowserService', function() {
test('makes correct call to removeVisits', function(done) {
registerMessageCallback('removeVisits', this, function(toRemove) {
assertEquals('http://www.example.com', toRemove[0].url);
assertEquals('https://en.wikipedia.org', toRemove[1].url);
assertEquals(1234, toRemove[0].timestamps[0]);
assertEquals(5678, toRemove[1].timestamps[0]);
done();
});
toDelete = [
createHistoryEntry(1234, 'http://www.example.com'),
createHistoryEntry(5678, 'https://en.wikipedia.org')
];
history.BrowserService.getInstance().deleteItems(toDelete);
});
teardown(function() {
registerMessageCallback('removeVisits', this, undefined);
});
});
......@@ -40,20 +40,6 @@ HistoryBrowserTest.prototype = {
},
};
function HistoryBrowserServiceTest() {}
HistoryBrowserServiceTest.prototype = {
__proto__: HistoryBrowserTest.prototype,
extraLibraries: HistoryBrowserTest.prototype.extraLibraries.concat([
'browser_service_test.js',
]),
};
TEST_F('HistoryBrowserServiceTest', 'All', function() {
mocha.run();
});
function HistoryDrawerTest() {}
HistoryDrawerTest.prototype = {
......
......@@ -61,13 +61,15 @@ suite('<history-list>', function() {
const dialog = element.$.dialog.get();
assertTrue(dialog.open);
element.$$('.action-button').click();
return testService.whenCalled('deleteItems');
return testService.whenCalled('removeVisits');
})
.then(test_util.flushTasks)
.then(function() {
deleteComplete();
.then(function(visits) {
assertEquals(1, visits.length);
assertEquals('http://example.com', visits[0].url);
assertEquals('2015-01-01 UTC', visits[0].timestamps[0]);
return test_util.flushTasks();
})
.then(test_util.flushTasks)
.then(function() {
assertEquals(element.historyData_.length, 0);
});
......@@ -398,17 +400,27 @@ suite('<history-list>', function() {
return test_util.flushTasks();
})
.then(function() {
testService.resetResolver('deleteItems');
testService.resetResolver('removeVisits');
// Confirmation dialog should appear.
assertTrue(dialog.open);
element.$$('.action-button').click();
return testService.whenCalled('deleteItems');
return testService.whenCalled('removeVisits');
})
.then(test_util.flushTasks)
.then(function() {
deleteComplete();
.then(function(visits) {
assertEquals(3, visits.length);
assertEquals(TEST_HISTORY_RESULTS[2].url, visits[0].url);
assertEquals(
TEST_HISTORY_RESULTS[2].allTimestamps[0],
visits[0].timestamps[0]);
assertEquals(ADDITIONAL_RESULTS[1].url, visits[1].url);
assertEquals(
ADDITIONAL_RESULTS[1].allTimestamps[0], visits[1].timestamps[0]);
assertEquals(ADDITIONAL_RESULTS[3].url, visits[2].url);
assertEquals(
ADDITIONAL_RESULTS[3].allTimestamps[0], visits[2].timestamps[0]);
return test_util.flushTasks();
})
.then(test_util.flushTasks)
.then(function() {
assertEquals(5, element.historyData_.length);
assertEquals(element.historyData_[0].dateRelativeDay, '2016-03-15');
......@@ -440,13 +452,17 @@ suite('<history-list>', function() {
items[1].$['menu-button'].click();
element.$.sharedMenu.get();
element.$$('#menuRemoveButton').click();
return testService.whenCalled('deleteItems');
return testService.whenCalled('removeVisits');
})
.then(test_util.flushTasks)
.then(function() {
deleteComplete();
.then(function(visits) {
assertEquals(1, visits.length);
assertEquals(TEST_HISTORY_RESULTS[1].url, visits[0].url);
assertEquals(
TEST_HISTORY_RESULTS[1].allTimestamps[0],
visits[0].timestamps[0]);
return test_util.flushTasks();
})
.then(test_util.flushTasks)
.then(function() {
assertDeepEquals(
[
......@@ -463,6 +479,65 @@ suite('<history-list>', function() {
});
});
test('delete disabled while pending', function() {
app.historyResult(createHistoryInfo(), TEST_HISTORY_RESULTS);
let items;
testService.delayDelete();
return test_util.flushTasks()
.then(function() {
Polymer.dom.flush();
items = polymerSelectAll(element, 'history-item');
items[1].$.checkbox.click();
items[2].$.checkbox.click();
items[1].$['menu-button'].click();
element.$.sharedMenu.get();
element.$$('#menuRemoveButton').click();
return testService.whenCalled('removeVisits');
})
.then(function(visits) {
assertEquals(1, visits.length);
assertEquals(TEST_HISTORY_RESULTS[1].url, visits[0].url);
assertEquals(
TEST_HISTORY_RESULTS[1].allTimestamps[0],
visits[0].timestamps[0]);
// Deletion is still happening. Verify that menu button and toolbar
// are disabled.
assertTrue(element.$$('#menuRemoveButton').disabled);
assertEquals(2, toolbar.count);
assertTrue(toolbar.$$('cr-toolbar-selection-overlay').deleteDisabled);
// Key event should be ignored.
assertEquals(1, testService.getCallCount('removeVisits'));
MockInteractions.pressAndReleaseKeyOn(
document.body, 46, '', 'Delete');
return test_util.flushTasks();
})
.then(function() {
assertEquals(1, testService.getCallCount('removeVisits'));
testService.finishRemoveVisits();
return test_util.flushTasks();
})
.then(test_util.flushTasks)
.then(function() {
// Reselect some items.
items = polymerSelectAll(element, 'history-item');
items[1].$.checkbox.click();
items[2].$.checkbox.click();
// Check that delete option is re-enabled.
assertEquals(2, toolbar.count);
assertFalse(
toolbar.$$('cr-toolbar-selection-overlay').deleteDisabled);
// Menu button should also be re-enabled.
items[1].$['menu-button'].click();
element.$.sharedMenu.get();
assertFalse(element.$$('#menuRemoveButton').disabled);
});
});
test('deleting items using shortcuts', function() {
app.historyResult(createHistoryInfo(), TEST_HISTORY_RESULTS);
const dialog = element.$.dialog.get();
......@@ -501,7 +576,7 @@ suite('<history-list>', function() {
.then(function() {
assertTrue(dialog.open);
element.$$('.action-button').click();
return testService.whenCalled('deleteItems');
return testService.whenCalled('removeVisits');
})
.then(function(toRemove) {
assertEquals('https://www.example.com', toRemove[0].url);
......
......@@ -118,10 +118,10 @@ suite('Metrics', function() {
await test_util.flushTasks();
MockInteractions.tap(app.$.history.$$('#menuRemoveButton'));
await testService.whenCalled('deleteItems');
deleteComplete();
await test_util.flushTasks();
await Promise.all([
testService.whenCalled('removeVisits'),
test_util.flushTasks(),
]);
assertEquals(1, histogramMap['HistoryPage.RemoveEntryPosition'][0]);
assertEquals(1, histogramMap['HistoryPage.RemoveEntryPositionSubset'][0]);
......
......@@ -37,7 +37,7 @@ suite('history-list supervised-user', function() {
// Make sure that removeVisits is not being called.
historyList.historyData_[0].selected = true;
toolbar.deleteSelectedItems();
assertEquals(0, testService.getCallCount('deleteItems'));
assertEquals(0, testService.getCallCount('removeVisits'));
});
test('remove history menu button disabled', function() {
......
......@@ -6,18 +6,22 @@ class TestBrowserService extends TestBrowserProxy {
constructor() {
super([
'deleteForeignSession',
'deleteItems',
'historyLoaded',
'navigateToUrl',
'openForeignSessionTab',
'otherDevicesInitialized',
'recordHistogram',
'removeVisits',
'queryHistory',
]);
this.histogramMap = {};
this.actionMap = {};
this.pendingDeletePromise_ = null;
this.deleted_ = null;
/** @private {?PromiseResolver} */
this.delayedRemove_ = null;
}
delayDelete() {
this.delayedRemove_ = new PromiseResolver();
}
/** @override */
......@@ -26,17 +30,17 @@ class TestBrowserService extends TestBrowserProxy {
}
/** @override */
deleteItems(items) {
this.deleted_ = items;
this.pendingDeletePromise_ = new PromiseResolver();
this.methodCalled('deleteItems', items.map(item => {
return {
url: item.url,
timestamps: item.allTimestamps,
};
}));
return this.pendingDeletePromise_.promise;
removeVisits(visits) {
this.methodCalled('removeVisits', visits);
if (this.delayedRemove_) {
return this.delayedRemove_.promise;
}
return Promise.resolve();
}
finishRemoveVisits() {
this.delayedRemove_.resolve();
this.delayedRemove_ = null;
}
/** @override */
......@@ -111,13 +115,4 @@ class TestBrowserService extends TestBrowserProxy {
/** @override */
removeBookmark() {}
/** @override */
resolveDelete(successful) {
if (successful) {
this.pendingDeletePromise_.resolve(this.deleted_);
} else {
this.pendingDeletePromise_.reject(this.deleted_);
}
}
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment