зеркало из https://github.com/mozilla/gecko-dev.git
Bug 1385733 - Improve the performance of async transactions when bookmarking all tabs. r=adw
Optimise adding a folder with child bookmarks for transactions by allowing PlacesTransactions.NewFolder to take children details and use insertTree rather than needing separate NewFolder and then multiple NewBookmark transactions. MozReview-Commit-ID: 6s9j0pbsiUB --HG-- extra : rebase_source : 0b4029905dc76a0ca49d16a7e71c85f1f07b8e2d
This commit is contained in:
Mark Banner
Родитель
0038962d8f
Коммит
0b94d0fe3e
|
|
@ -552,16 +552,10 @@ var BookmarkPropertiesPanel = {
|
|||
_getTransactionsForURIList: function BPP__getTransactionsForURIList() {
|
||||
var transactions = [];
|
||||
for (let uri of this._URIs) {
|
||||
// uri should be an object in the form { uri, title }. Though add-ons
|
||||
// could still use the legacy form, where it's an nsIURI.
|
||||
// TODO: Remove This from v57 on.
|
||||
let [_uri, _title] = uri instanceof Ci.nsIURI ?
|
||||
[uri, this._getURITitleFromHistory(uri)] : [uri.uri, uri.title];
|
||||
|
||||
let createTxn =
|
||||
new PlacesCreateBookmarkTransaction(_uri, -1,
|
||||
new PlacesCreateBookmarkTransaction(uri.uri, -1,
|
||||
PlacesUtils.bookmarks.DEFAULT_INDEX,
|
||||
_title);
|
||||
uri.title);
|
||||
transactions.push(createTxn);
|
||||
}
|
||||
return transactions;
|
||||
|
|
@ -665,14 +659,11 @@ var BookmarkPropertiesPanel = {
|
|||
|
||||
itemGuid = await PlacesTransactions.NewLivemark(info).transact();
|
||||
} else if (this._itemType == BOOKMARK_FOLDER) {
|
||||
// NewFolder requires a url rather than uri.
|
||||
info.children = this._URIs.map(item => {
|
||||
return { url: item.uri, title: item.title };
|
||||
});
|
||||
itemGuid = await PlacesTransactions.NewFolder(info).transact();
|
||||
// URIs is an array of objects in the form { uri, title }. It is still
|
||||
// named URIs because for backwards compatibility it could also be an
|
||||
// array of nsIURIs. TODO: Fix the property names from v57.
|
||||
for (let { uri: url, title } of this._URIs) {
|
||||
await PlacesTransactions.NewBookmark({ parentGuid: itemGuid, url, title })
|
||||
.transact();
|
||||
}
|
||||
} else {
|
||||
throw new Error(`unexpected value for _itemType: ${this._itemType}`);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -45,6 +45,11 @@ add_task(async function() {
|
|||
});
|
||||
|
||||
is(newFolder.title, "n", "folder name has been edited");
|
||||
|
||||
let bm = await PlacesUtils.bookmarks.fetch(newBookmark.guid);
|
||||
Assert.equal(bm.index, insertionIndex + 1,
|
||||
"Bookmark should have been shifted to the next index");
|
||||
|
||||
await PlacesUtils.bookmarks.remove(newFolder);
|
||||
await PlacesUtils.bookmarks.remove(newBookmark);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -726,19 +726,19 @@ function isPrimitive(v) {
|
|||
return v === null || (typeof(v) != "object" && typeof(v) != "function");
|
||||
}
|
||||
|
||||
function checkProperty(obj, prop, required, checkFn) {
|
||||
if (prop in obj)
|
||||
return checkFn(obj[prop]);
|
||||
|
||||
return !required;
|
||||
}
|
||||
|
||||
DefineTransaction.annotationObjectValidate = function(obj) {
|
||||
let checkProperty = (prop, required, checkFn) => {
|
||||
if (prop in obj)
|
||||
return checkFn(obj[prop]);
|
||||
|
||||
return !required;
|
||||
};
|
||||
|
||||
if (obj &&
|
||||
checkProperty("name", true, v => typeof(v) == "string" && v.length > 0) &&
|
||||
checkProperty("expires", false, Number.isInteger) &&
|
||||
checkProperty("flags", false, Number.isInteger) &&
|
||||
checkProperty("value", false, isPrimitive) ) {
|
||||
checkProperty(obj, "name", true, v => typeof(v) == "string" && v.length > 0) &&
|
||||
checkProperty(obj, "expires", false, Number.isInteger) &&
|
||||
checkProperty(obj, "flags", false, Number.isInteger) &&
|
||||
checkProperty(obj, "value", false, isPrimitive) ) {
|
||||
// Nothing else should be set
|
||||
let validKeys = ["name", "value", "flags", "expires"];
|
||||
if (Object.keys(obj).every(k => validKeys.includes(k)))
|
||||
|
|
@ -747,6 +747,19 @@ DefineTransaction.annotationObjectValidate = function(obj) {
|
|||
throw new Error("Invalid annotation object");
|
||||
};
|
||||
|
||||
DefineTransaction.childObjectValidate = function(obj) {
|
||||
if (obj &&
|
||||
checkProperty(obj, "title", false, v => typeof(v) == "string") &&
|
||||
!("type" in obj && obj.type != PlacesUtils.bookmarks.TYPE_BOOKMARK)) {
|
||||
obj.url = DefineTransaction.urlValidate(obj.url);
|
||||
let validKeys = ["title", "url"];
|
||||
if (Object.keys(obj).every(k => validKeys.includes(k))) {
|
||||
return obj;
|
||||
}
|
||||
}
|
||||
throw new Error("Invalid child object");
|
||||
};
|
||||
|
||||
DefineTransaction.urlValidate = function(url) {
|
||||
if (url instanceof Ci.nsIURI)
|
||||
return new URL(url.spec);
|
||||
|
|
@ -763,7 +776,7 @@ DefineTransaction.defineInputProps = function(names, validateFn, defaultValue) {
|
|||
try {
|
||||
return validateFn(value);
|
||||
} catch (ex) {
|
||||
throw new Error(`Invalid value for input property ${name}`);
|
||||
throw new Error(`Invalid value for input property ${name}: ${ex}`);
|
||||
}
|
||||
},
|
||||
|
||||
|
|
@ -898,10 +911,13 @@ DefineTransaction.defineInputProps(["index", "newIndex"],
|
|||
PlacesUtils.bookmarks.DEFAULT_INDEX);
|
||||
DefineTransaction.defineInputProps(["annotation"],
|
||||
DefineTransaction.annotationObjectValidate);
|
||||
DefineTransaction.defineInputProps(["child"],
|
||||
DefineTransaction.childObjectValidate);
|
||||
DefineTransaction.defineArrayInputProp("guids", "guid");
|
||||
DefineTransaction.defineArrayInputProp("urls", "url");
|
||||
DefineTransaction.defineArrayInputProp("tags", "tag");
|
||||
DefineTransaction.defineArrayInputProp("annotations", "annotation");
|
||||
DefineTransaction.defineArrayInputProp("children", "child");
|
||||
DefineTransaction.defineArrayInputProp("excludingAnnotations",
|
||||
"excludingAnnotation");
|
||||
|
||||
|
|
@ -1102,35 +1118,61 @@ PT.NewBookmark.prototype = Object.seal({
|
|||
* Transaction for creating a folder.
|
||||
*
|
||||
* Required Input Properties: title, parentGuid.
|
||||
* Optional Input Properties: index, annotations.
|
||||
* Optional Input Properties: index, annotations, children
|
||||
*
|
||||
* When this transaction is executed, it's resolved to the new folder's GUID.
|
||||
*/
|
||||
PT.NewFolder = DefineTransaction(["parentGuid", "title"],
|
||||
["index", "annotations"]);
|
||||
["index", "annotations", "children"]);
|
||||
PT.NewFolder.prototype = Object.seal({
|
||||
async execute({ parentGuid, title, index, annotations }) {
|
||||
let info = { type: PlacesUtils.bookmarks.TYPE_FOLDER,
|
||||
parentGuid, index, title };
|
||||
async execute({ parentGuid, title, index, annotations, children }) {
|
||||
let folderGuid;
|
||||
let info = {
|
||||
children: [{
|
||||
title,
|
||||
type: PlacesUtils.bookmarks.TYPE_FOLDER,
|
||||
}],
|
||||
// insertTree uses guid as the parent for where it is being inserted
|
||||
// into.
|
||||
guid: parentGuid,
|
||||
};
|
||||
|
||||
if (children && children.length > 0) {
|
||||
info.children[0].children = children;
|
||||
}
|
||||
|
||||
async function createItem() {
|
||||
info = await PlacesUtils.bookmarks.insert(info);
|
||||
// Note, insertTree returns an array, rather than the folder/child structure.
|
||||
// For simplicity, we only get the new folder id here. This means that
|
||||
// an undo then redo won't retain exactly the same information for all
|
||||
// the child bookmarks, but we believe that isn't important at the moment.
|
||||
let bmInfo = await PlacesUtils.bookmarks.insertTree(info);
|
||||
// insertTree returns an array, but we only need to deal with the folder guid.
|
||||
folderGuid = bmInfo[0].guid;
|
||||
|
||||
// Bug 1388097: insertTree doesn't handle inserting at a specific index for the folder,
|
||||
// therefore we update the bookmark manually afterwards.
|
||||
if (index != PlacesUtils.bookmarks.DEFAULT_INDEX) {
|
||||
bmInfo[0].index = index;
|
||||
bmInfo = await PlacesUtils.bookmarks.update(bmInfo[0]);
|
||||
}
|
||||
|
||||
if (annotations.length > 0) {
|
||||
let itemId = await PlacesUtils.promiseItemId(info.guid);
|
||||
let itemId = await PlacesUtils.promiseItemId(folderGuid);
|
||||
PlacesUtils.setAnnotationsForItem(itemId, annotations);
|
||||
}
|
||||
}
|
||||
await createItem();
|
||||
|
||||
this.undo = async function() {
|
||||
await PlacesUtils.bookmarks.remove(info);
|
||||
await PlacesUtils.bookmarks.remove(folderGuid);
|
||||
};
|
||||
this.redo = async function() {
|
||||
await createItem();
|
||||
// See the reasoning in CreateItem for why we don't care
|
||||
// about precisely resetting the lastModified value.
|
||||
};
|
||||
return info.guid;
|
||||
return folderGuid;
|
||||
}
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -9,6 +9,7 @@ const tagssvc = PlacesUtils.tagging;
|
|||
const annosvc = PlacesUtils.annotations;
|
||||
const PT = PlacesTransactions;
|
||||
const rootGuid = PlacesUtils.bookmarks.rootGuid;
|
||||
const menuGuid = PlacesUtils.bookmarks.menuGuid;
|
||||
|
||||
Components.utils.importGlobalProperties(["URL"]);
|
||||
|
||||
|
|
@ -161,11 +162,17 @@ function ensureUndoState(aExpectedEntries = [], aExpectedUndoPosition = 0) {
|
|||
}
|
||||
|
||||
function ensureItemsAdded(...items) {
|
||||
Assert.equal(observer.itemsAdded.size, items.length);
|
||||
let expectedResultsCount = items.length;
|
||||
|
||||
for (let item of items) {
|
||||
Assert.ok(observer.itemsAdded.has(item.guid));
|
||||
if ("children" in item) {
|
||||
expectedResultsCount += item.children.length;
|
||||
}
|
||||
Assert.ok(observer.itemsAdded.has(item.guid),
|
||||
`Should have the expected guid ${item.guid}`);
|
||||
let info = observer.itemsAdded.get(item.guid);
|
||||
Assert.equal(info.parentGuid, item.parentGuid);
|
||||
Assert.equal(info.parentGuid, item.parentGuid,
|
||||
"Should have notified the correct parentGuid");
|
||||
for (let propName of ["title", "index", "itemType"]) {
|
||||
if (propName in item)
|
||||
Assert.equal(info[propName], item[propName]);
|
||||
|
|
@ -173,22 +180,36 @@ function ensureItemsAdded(...items) {
|
|||
if ("url" in item)
|
||||
Assert.ok(info.url.equals(item.url));
|
||||
}
|
||||
|
||||
Assert.equal(observer.itemsAdded.size, expectedResultsCount,
|
||||
"Should have added the correct number of items");
|
||||
}
|
||||
|
||||
function ensureItemsRemoved(...items) {
|
||||
Assert.equal(observer.itemsRemoved.size, items.length);
|
||||
let expectedResultsCount = items.length;
|
||||
|
||||
for (let item of items) {
|
||||
// We accept both guids and full info object here.
|
||||
if (typeof(item) == "string") {
|
||||
Assert.ok(observer.itemsRemoved.has(item));
|
||||
Assert.ok(observer.itemsRemoved.has(item),
|
||||
`Should have removed the expected guid ${item}`);
|
||||
} else {
|
||||
Assert.ok(observer.itemsRemoved.has(item.guid));
|
||||
if ("children" in item) {
|
||||
expectedResultsCount += item.children.length;
|
||||
}
|
||||
|
||||
Assert.ok(observer.itemsRemoved.has(item.guid),
|
||||
`Should have removed expected guid ${item.guid}`);
|
||||
let info = observer.itemsRemoved.get(item.guid);
|
||||
Assert.equal(info.parentGuid, item.parentGuid);
|
||||
Assert.equal(info.parentGuid, item.parentGuid,
|
||||
"Should have notified the correct parentGuid");
|
||||
if ("index" in item)
|
||||
Assert.equal(info.index, item.index);
|
||||
}
|
||||
}
|
||||
|
||||
Assert.equal(observer.itemsRemoved.size, expectedResultsCount,
|
||||
"Should have removed the correct number of items");
|
||||
}
|
||||
|
||||
function ensureItemsChanged(...items) {
|
||||
|
|
@ -245,8 +266,13 @@ function ensureTagsForURI(aURI, aTags) {
|
|||
do_check_true(aTags.every( t => tagsSet.includes(t)));
|
||||
}
|
||||
|
||||
function createTestFolderInfo(aTitle = "Test Folder") {
|
||||
return { parentGuid: rootGuid, title: aTitle };
|
||||
function createTestFolderInfo(title = "Test Folder", parentGuid = menuGuid,
|
||||
children = undefined) {
|
||||
let info = { parentGuid, title };
|
||||
if (children) {
|
||||
info.children = children;
|
||||
}
|
||||
return info;
|
||||
}
|
||||
|
||||
function isLivemarkTree(aTree) {
|
||||
|
|
@ -259,20 +285,42 @@ async function ensureLivemarkCreatedByAddLivemark(aLivemarkGuid) {
|
|||
await PlacesUtils.livemarks.getLivemark({ guid: aLivemarkGuid });
|
||||
}
|
||||
|
||||
function removeAllDatesInTree(tree) {
|
||||
if ("lastModified" in tree) {
|
||||
delete tree.lastModified;
|
||||
}
|
||||
if ("dateAdded" in tree) {
|
||||
delete tree.dateAdded;
|
||||
}
|
||||
|
||||
if (!tree.children) {
|
||||
return;
|
||||
}
|
||||
|
||||
for (let child of tree.children) {
|
||||
removeAllDatesInTree(child);
|
||||
}
|
||||
}
|
||||
|
||||
// Checks if two bookmark trees (as returned by promiseBookmarksTree) are the
|
||||
// same.
|
||||
// false value for aCheckParentAndPosition is ignored if aIsRestoredItem is set.
|
||||
async function ensureEqualBookmarksTrees(aOriginal,
|
||||
aNew,
|
||||
aIsRestoredItem = true,
|
||||
aCheckParentAndPosition = false) {
|
||||
aCheckParentAndPosition = false,
|
||||
aIgnoreAllDates = false) {
|
||||
// Note "id" is not-enumerable, and is therefore skipped by Object.keys (both
|
||||
// ours and the one at deepEqual). This is fine for us because ids are not
|
||||
// restored by Redo.
|
||||
if (aIsRestoredItem) {
|
||||
// Ignore lastModified for newly created items, for performance reasons.
|
||||
if (!aOriginal.lastModified)
|
||||
if (aIgnoreAllDates) {
|
||||
removeAllDatesInTree(aOriginal);
|
||||
removeAllDatesInTree(aNew);
|
||||
} else if (!aOriginal.lastModified) {
|
||||
// Ignore lastModified for newly created items, for performance reasons.
|
||||
aNew.lastModified = aOriginal.lastModified;
|
||||
}
|
||||
Assert.deepEqual(aOriginal, aNew);
|
||||
if (isLivemarkTree(aNew))
|
||||
await ensureLivemarkCreatedByAddLivemark(aNew.guid);
|
||||
|
|
@ -318,6 +366,14 @@ async function ensureBookmarksTreeRestoredCorrectly(...aOriginalBookmarksTrees)
|
|||
}
|
||||
}
|
||||
|
||||
async function ensureBookmarksTreeRestoredCorrectlyExceptDates(...aOriginalBookmarksTrees) {
|
||||
for (let originalTree of aOriginalBookmarksTrees) {
|
||||
let restoredTree =
|
||||
await PlacesUtils.promiseBookmarksTree(originalTree.guid);
|
||||
await ensureEqualBookmarksTrees(originalTree, restoredTree, true, false, true);
|
||||
}
|
||||
}
|
||||
|
||||
async function ensureNonExistent(...aGuids) {
|
||||
for (let guid of aGuids) {
|
||||
Assert.strictEqual((await PlacesUtils.promiseBookmarksTree(guid)), null);
|
||||
|
|
@ -384,7 +440,7 @@ add_task(async function test_new_folder_with_annotation() {
|
|||
if (aRedo) {
|
||||
// Ignore lastModified in the comparison, for performance reasons.
|
||||
originalInfo.lastModified = null;
|
||||
await ensureBookmarksTreeRestoredCorrectly(originalInfo);
|
||||
await ensureBookmarksTreeRestoredCorrectlyExceptDates(originalInfo);
|
||||
}
|
||||
observer.reset();
|
||||
};
|
||||
|
|
@ -408,6 +464,51 @@ add_task(async function test_new_folder_with_annotation() {
|
|||
ensureUndoState();
|
||||
});
|
||||
|
||||
add_task(async function test_new_folder_with_children() {
|
||||
let folder_info = createTestFolderInfo("Test folder", PlacesUtils.bookmarks.menuGuid, [{
|
||||
url: "http://test_create_item.com",
|
||||
title: "Test creating an item"
|
||||
}]);
|
||||
ensureUndoState();
|
||||
let txn = PT.NewFolder(folder_info);
|
||||
folder_info.guid = await txn.transact();
|
||||
let originalInfo = await PlacesUtils.promiseBookmarksTree(folder_info.guid);
|
||||
let ensureDo = async function(aRedo = false) {
|
||||
ensureUndoState([[txn]], 0);
|
||||
ensureItemsAdded(folder_info);
|
||||
if (aRedo) {
|
||||
// Ignore lastModified in the comparison, for performance reasons.
|
||||
originalInfo.lastModified = null;
|
||||
await ensureBookmarksTreeRestoredCorrectlyExceptDates(originalInfo);
|
||||
}
|
||||
observer.reset();
|
||||
};
|
||||
|
||||
let ensureUndo = () => {
|
||||
ensureUndoState([[txn]], 1);
|
||||
ensureItemsRemoved({
|
||||
guid: folder_info.guid,
|
||||
parentGuid: folder_info.parentGuid,
|
||||
index: bmStartIndex,
|
||||
children: [{
|
||||
title: "Test creating an item",
|
||||
url: "http://test_create_item.com",
|
||||
}]
|
||||
});
|
||||
observer.reset();
|
||||
};
|
||||
|
||||
await ensureDo();
|
||||
await PT.undo();
|
||||
await ensureUndo();
|
||||
await PT.redo();
|
||||
await ensureDo(true);
|
||||
await PT.undo();
|
||||
ensureUndo();
|
||||
await PT.clearTransactionsHistory();
|
||||
ensureUndoState();
|
||||
});
|
||||
|
||||
add_task(async function test_new_bookmark() {
|
||||
let bm_info = { parentGuid: rootGuid,
|
||||
url: NetUtil.newURI("http://test_create_item.com"),
|
||||
|
|
@ -659,7 +760,7 @@ add_task(async function test_remove_folder() {
|
|||
ensureUndoState([ [remove_folder_2_txn],
|
||||
[folder_level_2_txn_result, folder_level_1_txn_result] ], 1);
|
||||
await ensureItemsAdded(folder_level_1_info, folder_level_2_info);
|
||||
await ensureBookmarksTreeRestoredCorrectly(original_folder_level_1_tree);
|
||||
await ensureBookmarksTreeRestoredCorrectlyExceptDates(original_folder_level_1_tree);
|
||||
observer.reset();
|
||||
|
||||
// Redo Remove "Folder Level 2"
|
||||
|
|
@ -1463,9 +1564,9 @@ add_task(async function test_copy() {
|
|||
|
||||
async function redo() {
|
||||
await PT.redo();
|
||||
await ensureBookmarksTreeRestoredCorrectly(originalInfo);
|
||||
await ensureBookmarksTreeRestoredCorrectlyExceptDates(originalInfo);
|
||||
await PT.redo();
|
||||
await ensureBookmarksTreeRestoredCorrectly(duplicateInfo);
|
||||
await ensureBookmarksTreeRestoredCorrectlyExceptDates(duplicateInfo);
|
||||
}
|
||||
async function undo() {
|
||||
await PT.undo();
|
||||
|
|
@ -1603,7 +1704,7 @@ add_task(async function test_invalid_uri_spec_throws() {
|
|||
});
|
||||
|
||||
add_task(async function test_annotate_multiple_items() {
|
||||
let parentGuid = rootGuid;
|
||||
let parentGuid = menuGuid;
|
||||
let guids = [
|
||||
await PT.NewBookmark({ url: "about:blank", parentGuid }).transact(),
|
||||
await PT.NewFolder({ title: "Test Folder", parentGuid }).transact()];
|
||||
|
|
@ -1645,7 +1746,7 @@ add_task(async function test_remove_multiple() {
|
|||
let guids = [];
|
||||
await PT.batch(async function() {
|
||||
let folderGuid = await PT.NewFolder({ title: "Test Folder",
|
||||
parentGuid: rootGuid }).transact();
|
||||
parentGuid: menuGuid }).transact();
|
||||
let nestedFolderGuid =
|
||||
await PT.NewFolder({ title: "Nested Test Folder",
|
||||
parentGuid: folderGuid }).transact();
|
||||
|
|
@ -1655,7 +1756,7 @@ add_task(async function test_remove_multiple() {
|
|||
|
||||
let bmGuid =
|
||||
await PT.NewBookmark({ url: new URL("http://test.bookmark.removed"),
|
||||
parentGuid: rootGuid }).transact();
|
||||
parentGuid: menuGuid }).transact();
|
||||
guids.push(bmGuid);
|
||||
});
|
||||
|
||||
|
|
@ -1679,7 +1780,7 @@ add_task(async function test_remove_multiple() {
|
|||
|
||||
// Redo it.
|
||||
await PT.redo();
|
||||
await ensureBookmarksTreeRestoredCorrectly(...originalInfos);
|
||||
await ensureBookmarksTreeRestoredCorrectlyExceptDates(...originalInfos);
|
||||
|
||||
// Redo remove.
|
||||
await PT.redo();
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче