Back in the day I coded bookmarkProperties.js in a rush. As it is, it stands in my way of refactoring the UI in question to work with my my new transaction manager. I guess I get what I deserve. In theory, it might have been better to refactor it all at once, but I think that going this way would lead to many more regressions, not to mention the review overload. I have three goals here: 1) Get all the "new-item" functionality out of the dialog. While it's true that the dialog is used for creating items, the items are actually created (by design) when the dialog is opened. We could simplify the code much by moving this first step to PlacesUIUtils, and keeping the dialog just for editing the bookmark. To be clear, this change won't affect user experience. 2) Simplify the code and remove dead code. 3) Make it more friendly to the async transaction manager change.

Mano, can you explain how the summary relates to comment #0, which seems to indicate this is more about moving code? In other words, I'm not clear on how we want the dialog to be "truly modal" and what that means, after reading comment #0 (which is also not really clear to me in terms of what I'd need to do to fix this bug).

Just ignore comment 0. This bug was morphed to cover what we actually need to do at last, which is to completely distinguish the instant-apply variant from the modal-variant. Since the current modal-variant is fake (it's just some hacks around the instant-apply variant), the first logical step is to write a true modal version of this dialog. Then we can (here or in a new bug) clean up the instant-apply code.

In bug 993375 I asked UX's approval for removing the "dummy item" behavior, assuming it's not going to play nice with the new API. Thinking about this more, I'm going to leave the "dummy" item feature in. If it's refactored correctly, I think it could actually ease the implementation quite a bit, and since no was has been complaining about this feature we don't really have to do away with it.

What's the status on this one, Mano?

Morphing again. I'll comment on the way forward on the rational later today.

Comment on attachment 8504026 [details] [diff] [review] part 1 Review of attachment 8504026 [details] [diff] [review]: ----------------------------------------------------------------- This is regression-prone, thus better land it now that we just merged. There are a couple mistakes in the patch, but I don't think any of those is worth another review pass. ::: browser/components/places/content/editBookmarkOverlay.js @@ +209,5 @@ > window.addEventListener("unload", this, false); > this._observersAdded = true; > } > > this._initialized = true; so what happens if I call init multiple times without a yield in the middle? like for example from different code points (that can't yield between each other) _initialized was used to avoid multiple init calls, it should be fixed to work in the async case or we might get race conditions. @@ +218,5 @@ > */ > + _getCommonTags() { > + if ("_cachedCommonTags" in this) > + return this._cachedCommonTags; > + trailing spaces @@ +219,5 @@ > + _getCommonTags() { > + if ("_cachedCommonTags" in this) > + return this._cachedCommonTags; > + > + let commonTags = new Set(PlacesUtils.tagging.getTagsForURI(this._uris[0])); tagging service will become async, so please ensure we can use ans async _getCommonTags, or eve make it a generator already. @@ +442,5 @@ > + > + let removedTags = [for (t of aCurrentTags) > + if (inputTags.indexOf(t) == -1) t]; > + let newTags = [for (t of inputTags) > + if (aCurrentTags.indexOf(t) == -1) t]; these sound like .filter calls hidden into comprehensions... @@ +448,4 @@ > }, > > + // Adds and removes tags for one or more uris. > + _setTagsFromInputField: Task.async(function* (aCurrentTags, uris) { _getTagsArrayFromTagField VS _setTagsFromInputField... I think we should make these names coherent (the part after "From"), TagField sounds fine, or even TagsInputField. @@ +550,3 @@ > if (description != PlacesUIUtils.getItemDescription(this._itemId)) { > + let annoObj = { name : PlacesUIUtils.DESCRIPTION_ANNO, > + value : description }; reindent colons... or not indent them at all :D @@ +581,1 @@ > this._uri = uri; I think in most cases where we do yield transact and then assign to an internal property, it would be safer to first assign, cause otherwise re-entrancy finds a broken status @@ +704,5 @@ > + PlacesTransactions.Move({ guid: this._itemGuid, newParentGuid })); > + } > + else { > + let txn = new PlacesMoveItemTransaction(this._itemId, > + container, while here please fix these trailing spaces @@ +822,3 @@ > > + let tagsInField = this._getTagsArrayFromTagField(); > + let allTags = PlacesUtils.tagging.allTags; this will become async, better making _rebuildTagsSelectorList async already? @@ +959,1 @@ > if (aURI.equals(changedURI)) { aURI is not going to work here, you changed the code from a forEach to a for...of. that means this code path is not tested at all :( should be uri.equals... if it's easy to write a simple test hitting this code path, please do, otherwise please file a bug to do that. ::: browser/components/places/content/places.js @@ +665,5 @@ > gEditItemOverlay.initPanel(itemId, { hiddenRows: ["folderPicker"] > , forceReadOnly: readOnly > , titleOverride: aSelectedNode.title > + }).then( > + () => { nit: I'd inline this with .then, and align .then with .initPanel

> @@ +442,5 @@ > > + > > + let removedTags = [for (t of aCurrentTags) > > + if (inputTags.indexOf(t) == -1) t]; > > + let newTags = [for (t of inputTags) > > + if (aCurrentTags.indexOf(t) == -1) t]; > > these sound like .filter calls hidden into comprehensions... Is that a problem? I think it'll look just fine once we get back Array.contains.

I just don't think it's wort reimplementing .filter, it could have some internal optimizations we lose by reimplementing it.

I cannot land this because of the expectations of browser_library_infoBox.js. This tests selects nodes in the library and expects the info pane to update synchronously. While most of the initialization is still synchronous, the function passed to initPane.then is only called on the next tick. I'm not sure what to do about this, because there's no event this test could wait for.

This bug is going to cover all remaining Places UI at last, including hacky Cancel support. This means that once we're done here, PT may be enabled on nightly (however, we're going to first switch it to use Bookmark.jsm). For future reference, here are the reasons I went with the the conservative and ugly solution, avoiding any major rewrites: 1) The time to rewrite this UIs as a whole isn't before Bookmark.jsm and Tagging.jsm work is done. Otherwise we'll have another refactoring round for no good reason. 2) If Library is to become in-content, there's a good chance the bookmark properties dialog will be removed. 3) It'll be easier to replace one UI with another once we can actually remove old PT code. 4) limited resources. The solution I'm going to use for Cancel here is pretty ugly and error-prone, but it'll do for now. I filed bug 1093030 to track a proper solution.

Comment on attachment 8519571 [details] [diff] [review] checkpoint I still need to write documentation for the lets-not-talk-about-it methods in PlacesUIUtils, file some bugs, fix the Bookmark-This-Frame caller in nsContextMenu.js, simulate the current initPanel API (for backwards compatibility) and polish some comments... but this is good for a first pass.

As above, just updated to some changes I did in bug 982115.

Comment on attachment 8519764 [details] [diff] [review] checkpoint Review of attachment 8519764 [details] [diff] [review]: ----------------------------------------------------------------- I went through everything but editBookmarkOverlay.js that is a huge part of the patch. saving this part of the review. ::: browser/base/content/browser-places.js @@ +69,5 @@ > } > this._restoreCommandsState(); > this._itemId = -1; > + if (this._batching) > + this.endBatch(); if endBatch is not considered "internal", I'm not sure _batching should. both should use _, or none. @@ +94,3 @@ > } > + // TODO mano: > + // Add support for PT.Remove({ url }) in PlacesTransactions. the rule is: one todo one bug :) Btw, unless there are multiple cases, doesn't sound so compelling. If this is the only use case, we can live with the simple workaround here @@ +98,5 @@ > + let guids = []; > + yield PlacesUtils.bookmarks.fetch({ url: this._uriForRemoval }, > + ({ guid }) => { > + guids.push(guid); > + }); nit: you can likely avoid bracing the function body an oneline this. @@ +100,5 @@ > + ({ guid }) => { > + guids.push(guid); > + }); > + yield PlacesTransactions.Remove(guids).transact(); > + }.bind(this)); are you avoiding .catch on purpose? since nobody is handling the Task promise, this is not guaranteed to be serialized by PlacesTransactions (due to the bookmarks.fetch yield) @@ +135,5 @@ > > _overlayLoaded: false, > _overlayLoading: false, > showEditBookmarkPopup: > + Task.async(function* (aNode, aAnchorElement, aPosition) { nit: when replacing "function label" with Task.async, I think we should oneline it with the property name as property: Task.async(... cause the function label is easy to parse while scrolling, while Task.async is generic and pointless. @@ +144,5 @@ > + if (typeof(aNode) == "number") { > + let itemId = aNode; > + if (PlacesUIUtils.useAsyncTransactions) { > + let guid = yield PlacesUtils.promiseItemGuid(itemId); > + aNode = yield PlacesUIUtils.promiseNodeLike(guid); should be fetchNodeLike.. but still fetchNodeLike("IamAguid") doesn't make much sense (the function name) maybe buildFakeNodeForBookmark() or something like that @@ +147,5 @@ > + let guid = yield PlacesUtils.promiseItemGuid(itemId); > + aNode = yield PlacesUIUtils.promiseNodeLike(guid); > + } > + else { > + aNode = yield PlacesUIUtils.completeNodeLikeObjectForItemId(itemId); completeNodeLikeObject sounds enough @@ +182,3 @@ > > _doShowEditBookmarkPanel: > + Task.async(function* (aNode, aAnchorElement, aPosition) { ditto for onelining this @@ +214,5 @@ > > // unset the unstarred state, if set > this._element("editBookmarkPanelStarIcon").removeAttribute("unstarred"); > > + this._itemId = aNode.itemId; I was trying to understand why the previous code was expecting an undefined aItemId, but I didn't find any cases... legacy code? @@ +280,5 @@ > + if (this._batching) > + return; > + if (PlacesUIUtils.useAsyncTransactions) { > + PlacesTransactions.batch(function* () { > + let endBatchPromise = new Promise((resolve, rejct) => { typo rejct (I think you can avoid it if you don't use it) @@ +298,5 @@ > + return; > + > + if (PlacesUIUtils.useAsyncTransactions) { > + this._endAsyncTransactionsBatch(); > + this._endAsyncTransactionsBatch = () => {}; I think it would be simpler to actually just set it to null and null check it... actually, couldn't we use that to detect _batching? something like _batchResolver, could be null or resolve function (bonus points if on execution it nullifies itself) @@ +321,5 @@ > * The folder in which to create a new bookmark if the page loaded in > * aBrowser isn't bookmarked yet, defaults to the unfiled root. > * @param [optional] aShowEditUI > * whether or not to show the edit-bookmark UI for the bookmark item > */ while here, trailing spaces @@ +324,5 @@ > * whether or not to show the edit-bookmark UI for the bookmark item > */ > + bookmarkPage: Task.async(function* (aBrowser, aParent, aShowEditUI) { > + if (PlacesUIUtils.useAsyncTransactions) > + return (yield this._bookmarkPagePT(aBrowser, aParent, aShowEditUI)); wouldn't be possible to do the same as in the other code points you touched, thus make this if (!useAsyncTransactions) return this._legacyFunction; new code I think the original idea to make old transactions removal simple is worth it. @@ +400,5 @@ > + }), > + > + // TODO: Replace bookmarkPage code with this function once legacy > + // transactions are removed. > + _bookmarkPagePT: Task.async(function* (aBrowser, aParent, aShowEditUI) { aParent should be aParentId @@ +401,5 @@ > + > + // TODO: Replace bookmarkPage code with this function once legacy > + // transactions are removed. > + _bookmarkPagePT: Task.async(function* (aBrowser, aParent, aShowEditUI) { > + let url = new URL(aBrowser.currentURI.spec); there should be no need for new URL since bookmarks support also plain hrefs and nsIURIs (and I think you added that support to PlacesTransactions too) @@ +405,5 @@ > + let url = new URL(aBrowser.currentURI.spec); > + let info = yield PlacesUtils.bookmarks.fetch({ url }); > + if (!info) { > + let parentGuid = aParent !== undefined ? > + yield PlacesUtils.promiseItemId(aParent) : and indeed this should be promiseItemGuid (aParentId would have made this more obvious) @@ +413,5 @@ > + // Bug 52536: We obtain the URL and title from the nsIWebNavigation > + // associated with a <browser/> rather than from a DOMWindow. > + // This is because when a full page plugin is loaded, there is > + // no DOMWindow (?) but information about the loaded document > + // may still be obtained from the webNavigation. OT: and this is sadly non e10s compatible (if you have an idea to fix bug 1067042 please post it there!) Actually, due to that, it may be wise to move this code to a common helper used by both old and new implementation. @@ +449,5 @@ > + // If it was not requested to open directly in "edit" mode, we are done. > + if (!aShowEditUI) > + return; > + > + let node = yield PlacesUIUtils.promiseNodeLikeFromFetchInfo(info); So, I think we should have only one util called buildFakeNodeFromBookmarkInfo() and we should do the fetch outside, so please remove fetchNodeLike and fetch before invoking it. @@ +487,5 @@ > * the address of the link target > * @param aTitle > * The link text > */ > + bookmarkLink: Task.async(function* (aParent, aURL, aTitle) { aParentId please @@ +488,5 @@ > * @param aTitle > * The link text > */ > + bookmarkLink: Task.async(function* (aParent, aURL, aTitle) { > + let node = null; s/node/fakeNode/ @@ +490,5 @@ > */ > + bookmarkLink: Task.async(function* (aParent, aURL, aTitle) { > + let node = null; > + if (PlacesUIUtils.useAsyncTransactions) { > + node = yield PlacesUIUtils.fetchNodeLike({ url: aURL }); just rename aURL to url? :) @@ +493,5 @@ > + if (PlacesUIUtils.useAsyncTransactions) { > + node = yield PlacesUIUtils.fetchNodeLike({ url: aURL }); > + } > + else { > + let itemId = PlacesUtils.getMostRecentBookmarkForURI(linkURI); this is not going to work since you removed the linkURI definition ::: browser/components/places/PlacesUIUtils.jsm @@ +1204,5 @@ > let cloudSyncEnabled = CloudSync && CloudSync.ready && CloudSync().tabsReady && CloudSync().tabs.hasRemoteTabs(); > return weaveEnabled || cloudSyncEnabled; > }, > + > + completeNodeLikeObjectForItemId(aNodeLike) { fwiw, I prefer fakeNode than nodeLike remember the big warning that add-ons should not use this. @@ +1218,5 @@ > + let itemId = aNodeLike.itemId; > + let defGetter = XPCOMUtils.defineLazyGetter.bind(XPCOMUtils, aNodeLike); > + > + if (!("title" in aNodeLike)) > + defGetter("title", () => PlacesUtils.bookmarks.getItemTitle(itemId)); dumb idea, why not a Proxy? @@ +1247,5 @@ > + }, > + > + promiseNodeLikeFromFetchInfo: Task.async(function* (aFetchInfo) { > + if (aFetchInfo.itemType == PlacesUtils.bookmarks.TYPE_SEPARATOR) > + throw new Error("promiseNodeLike doesn't support separators"); does it support folders? I think it's only used to create uri bookmarks ::: browser/components/places/content/bookmarkProperties.js @@ +37,5 @@ > * - "edit" - for editing a bookmark item or a folder. > * @ type (String). Possible values: > * - "bookmark" > + * @ node (an nsINavHistoryResultNode object) - a node representing > + * the bookmark. Please remember me what's the add-ons status regarding this change, any high population add-on using this with itemId? @@ +54,1 @@ > * - "folderPicker" - hides both the tree and the menu. I think we discussed making feedURI the default copied text for livemarks on copy, to allow to remove feedLocation/siteLocation... is that bug on file already? it's something we could/should fix before this change. @@ +237,3 @@ > switch (dialogInfo.type) { > case "bookmark": > this._itemType = BOOKMARK_ITEM; In future we should unify these with TYPE_BOOKMARK and TYPE_FOLDER... @@ +284,3 @@ > break; > case ACTION_ADD: > + yield this._promiseNewItem(); I think that this should read this._node = yield this._promiseNewItem(); I think it's better if the assignment is explicit rather than hidden. @@ +287,5 @@ > + > + // Edit the new item > + gEditItemOverlay.initPanel({ node: this._node > + , hiddenRows: this._hiddenRows }); > + window.sizeToContent(); this was not needed before, what changed? why is it not needed in the edit case? @@ +368,5 @@ > break; > } > }, > > + _endAsyncTransactionsBatch() { }, same comments as the StarUI batching @@ +609,5 @@ > > PlacesUtils.transactionManager.doTransaction(txn); > this._itemId = PlacesUtils.bookmarks.getIdForItemAt(container, index); > + > + this._node = Object.freeze({ yes, here too I think we should return an object and assign to this._node at the call point @@ +632,5 @@ > + { [BOOKMARK_FOLDER]: PlacesTransactions.NewFolder, > + [LIVEMARK_CONTAINER]: PlacesTransactions.NewLivemark, > + [BOOKMARK_ITEM]: PlacesTransactions.NewBookmark }[this._itemType]; > + > + let [container, index] = this._getInsertionPointDetails(); containerId please ::: browser/components/places/content/controller.js @@ +657,2 @@ > PlacesUIUtils.showBookmarkDialog({ action: "edit" > + , node here we were passing the concreteItemId for roots and tag folders to catch the "real" title. please check those are still correct or fix that. ::: browser/components/places/content/places.js @@ +662,5 @@ > else > itemId = PlacesUtils._uri(aSelectedNode.uri); > > + gEditItemOverlay.initPanel({ node: aSelectedNode > + , hiddenRows: ["folderPicker"] }); the code above this one does a lot of preparation for concreteItemId and readOnly that doesn't look needed anymore... ::: toolkit/components/places/PlacesTransactions.jsm @@ +1273,2 @@ > */ > +PT.Annotate = DefineTransaction(["guids", "annotations"]); Accepting multiple guids requires a test. @@ +1275,3 @@ > PT.Annotate.prototype = { > + execute: function* (aGuids, aNewAnnos) { > + let undoAnnosForItem = new Map(); // itemId => undoAnnos; ...ForItemId then @@ +1401,2 @@ > */ > +PT.Remove = DefineTransaction(["guids"]); ditto for a test @@ +1415,3 @@ > > + let removeThem = Task.async(function* () { > + // TODO: Use Bookmarks.remove(guids) actually we discussed this while I was coding the API, and we decided it was not needed cause looping had basically the same downsides

Comment on attachment 8519764 [details] [diff] [review] checkpoint Review of attachment 8519764 [details] [diff] [review]: ----------------------------------------------------------------- weeee. this was hard. ::: browser/components/places/content/editBookmarkOverlay.js @@ +22,5 @@ > + > + if ("uris" in aInitInfo && "node" in aInitInfo) > + throw new Error("ambiguous pane info"); > + if (!("uris" in aInitInfo) && !("node" in aInitInfo)) > + throw new Error("Neither node nor uris set for pane info"); are we keeping uris for compatibility reasons? since we are moving everything to urls... @@ +26,5 @@ > + throw new Error("Neither node nor uris set for pane info"); > + > + let node = "node" in aInitInfo ? aInitInfo.node : null, > + itemId = node ? node.itemId : -1, > + itemGuid = PlacesUIUtils.useAsyncTransactions && node ? nit: I honestly think one let per line is more readable and less error-prone than commas... @@ +35,5 @@ > + title = node ? node.title : null, > + isBookmark = isItem && isURI, > + isFolderShortcut = isItem ? > + node.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT : > + false; isItem && node.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT @@ +43,5 @@ > + > + let uriTags = null; > + if (node && "tags" in node) { > + let tagsStr = node.tags; > + uriTags = tagsStr === null ? [] : tagsStr.split(","); I'm not 100% sure node tags is always properly filled up... there's something lazy there... but I may be wrong @@ +73,5 @@ > + get uri() { > + if (!this.initialized) > + return null; > + if (this._paneInfo.bulkTagging) > + return this._paneInfo.uris[0] missing semicolon @@ +81,5 @@ > + > + // Check if the pane is initialized to show only read-only fields. > + get readOnly() { > + if (!this.initialized) > + return false; hm, I would have said the opposite, if it's not initialized, it would be "safer" as readonly. @@ +83,5 @@ > + get readOnly() { > + if (!this.initialized) > + return false; > + return (this.initialized && > + !this._paneInfo.visibleRows.has("tagsRow") && so here would be !initialized || (...) @@ +84,5 @@ > + if (!this.initialized) > + return false; > + return (this.initialized && > + !this._paneInfo.visibleRows.has("tagsRow") && > + (this._paneInfo.isFolderShortcut || hm, we allow to edit title of folder shortcuts... the tricky part is that what the user edits should be the original folder (unless it's a root shortcut or a tag shortcut) IIRC this is how things worked so far. we may evaluate to change that, but should discuss approaches. Surely we should allow some sort of editing to allow the user to assign nice names. @@ +123,5 @@ > + > + let newKeyword = aNewKeyword; > + if (newKeyword === undefined) { > + let itemId = this._paneInfo.itemId; > + newKeyword = PlacesUtils.bookmarks.getKeywordForBookmark(itemId); this will become async, thus _initKeywordField should already be fake async @@ +133,5 @@ > + if (!this._paneInfo.isBookmark) > + throw new Error("_initLoadInSidebar called unexpectedly"); > + > + this._loadInSidebarCheckbox.checked = > + PlacesUtils.annotations.itemHasAnnotation( this should become async as well... @@ +147,5 @@ > + * - node: either a result node or a node-like object representing the > + * item to be edited. A node-like object must have the following > + * properties (with values that match exactly those a result node > + * would have): itemId, bookmarkGuid, uri, title, type. > + * a node-like object leftover line? @@ +215,5 @@ > + // Technically we should check that the item is not moveable, but that's > + // not cheap (we don't always have the parent), and there's no use case for > + // this (it's only the Star UI that shows the folderPicker) > + if (showOrCollapse("folderRow", isItem, "folderPicker")) { > + let containerId = PlacesUtils.bookmarks.getFolderIdForItem(itemId); and this will become async as well... @@ +261,3 @@ > } > + return this._paneInfo._cachedCommonTags = [...commonTags]; > + }), sounds like an API that could be useful in the new tagging service... Unfortunately then it would be async, as well. @@ +389,4 @@ > }, > > + onTagsFieldChange() { > + if (this.initialized) { worth checking readonly too? in case add-ons do fancy things. @@ +452,2 @@ > > + // Only in the library info-pane it's safe (and necessary) to batch these. would be nice to also explain the reason :) @@ +500,5 @@ > prefs.setCharPref("browser.bookmarks.editDialog.firstEditField", aNewField); > }, > > + onNamePickerChange() { > + if (!this.initialized || !this._paneInfo.isItem) || readonly @@ +525,5 @@ > } > }, > > + onDescriptionFieldChange() { > + if (!this.initialized || !this._paneInfo.isItem) ditto @@ +546,5 @@ > } > }, > > + onLocationFieldChange() { > + if (!this.initialized || !this._paneInfo.isBookmark) ditto @@ +556,3 @@ > } > + catch(ex) { > + // XXX Bug 1089141. please expand the comment with a nicer todo @@ +574,4 @@ > }, > > + onKeywordFieldChange() { > + if (!this.initialized || !this._paneInfo.isBookmark) ditto @@ +590,4 @@ > }, > > + onLoadInSidebarCheckboxCommand() { > + if (!this.initialized || !this._paneInfo.isBookmark) ditto @@ +678,5 @@ > > if (aEvent.target.id == "editBMPanel_chooseFolderMenuItem") { > // reset the selection back to where it was and expand the tree > // (this menu-item is hidden when the tree is already visible > + let container = PlacesUtils.bookmarks.getFolderIdForItem(this._itemId); containerId? @@ +679,5 @@ > if (aEvent.target.id == "editBMPanel_chooseFolderMenuItem") { > // reset the selection back to where it was and expand the tree > // (this menu-item is hidden when the tree is already visible > + let container = PlacesUtils.bookmarks.getFolderIdForItem(this._itemId); > + let item = this._getFolderMenuItem(container); sigh, so confusing "item"... menuitem maybe?

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #24) > Comment on attachment 8519764 [details] [diff] [review] > checkpoint > > Review of attachment 8519764 [details] [diff] [review]: > ----------------------------------------------------------------- > > weeee. this was hard. > Truly sorry! > ::: browser/components/places/content/editBookmarkOverlay.js > @@ +22,5 @@ > > + > > + if ("uris" in aInitInfo && "node" in aInitInfo) > > + throw new Error("ambiguous pane info"); > > + if (!("uris" in aInitInfo) && !("node" in aInitInfo)) > > + throw new Error("Neither node nor uris set for pane info"); > > are we keeping uris for compatibility reasons? since we are moving > everything to urls... > I think it's better to stick to uri(s) in places where we don't support anything but nsIURI objects. > @@ +43,5 @@ > > + > > + let uriTags = null; > > + if (node && "tags" in node) { > > + let tagsStr = node.tags; > > + uriTags = tagsStr === null ? [] : tagsStr.split(","); > > I'm not 100% sure node tags is always properly filled up... there's > something lazy there... but I may be wrong > I will remove that for now then. > > @@ +81,5 @@ > > + > > + // Check if the pane is initialized to show only read-only fields. > > + get readOnly() { > > + if (!this.initialized) > > + return false; > > hm, I would have said the opposite, if it's not initialized, it would be > "safer" as readonly. > OK, or I could throw. > > @@ +84,5 @@ > > + if (!this.initialized) > > + return false; > > + return (this.initialized && > > + !this._paneInfo.visibleRows.has("tagsRow") && > > + (this._paneInfo.isFolderShortcut || > > hm, we allow to edit title of folder shortcuts... the tricky part is that > what the user edits should be the original folder (unless it's a root > shortcut or a tag shortcut) > IIRC this is how things worked so far. we may evaluate to change that, but > should discuss approaches. Surely we should allow some sort of editing to > allow the user to assign nice names. Not really... the only shortcuts we have are those under All Bookmarks, and those aren't editable (it was previously done by forceReadOnly). > > @@ +389,4 @@ > > }, > > > > + onTagsFieldChange() { > > + if (this.initialized) { > > worth checking readonly too? in case add-ons do fancy things. > the tags field is visible only in !readOnly mode. > > @@ +500,5 @@ > > prefs.setCharPref("browser.bookmarks.editDialog.firstEditField", aNewField); > > }, > > > > + onNamePickerChange() { > > + if (!this.initialized || !this._paneInfo.isItem) > > || readonly > It's now a change listener (rather than blur), which cannot be dispatched when the field is read-only. > @@ +525,5 @@ > > } > > }, > > > > + onDescriptionFieldChange() { > > + if (!this.initialized || !this._paneInfo.isItem) > > ditto > ditto here and elsewhere :)

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #25) > Not really... the only shortcuts we have are those under All Bookmarks, and > those aren't editable (it was previously done by forceReadOnly). users can copy those shortcuts around, I'm sure many did, especially with tags in the past. With that change they will become un-editable, I think. > the tags field is visible only in !readOnly mode. I was mostly worried about add-ons that really don't care if your field is visible. but likely I was just overzealous.

The PlacesTransactions parts of this patch have been moved to bug 1103622 and bug 1103636 (patches for both bugs are in).

I still want to write some compatibility magic in editBookmarkOverlay, but I'll do so in a separate patch. This is big enough already.

Comment on attachment 8547419 [details] [diff] [review] patch Review of attachment 8547419 [details] [diff] [review]: ----------------------------------------------------------------- this looks ok, please make changes in a new interdiff and ask for a further review just on that part. Let's try to land this asap, I'm sure we'll have regressions to deal with. ::: browser/base/content/browser-places.js @@ +77,5 @@ > + if (!PlacesUIUtils.useAsyncTransactions) { > + PlacesUtils.transactionManager.undoTransaction(); > + break; > + } > + PlacesTransactions.undo().then(Cu.reportError); I guess you meant .catch(Cu.reportError); @@ +128,5 @@ > > _overlayLoaded: false, > _overlayLoading: false, > showEditBookmarkPopup: > + Task.async(function* (aNode, aAnchorElement, aPosition) { please oneline, even if we go over 80 by a couple chars @@ +130,5 @@ > _overlayLoading: false, > showEditBookmarkPopup: > + Task.async(function* (aNode, aAnchorElement, aPosition) { > + // Support the legacy aItemId signature for the time being. > + // TODO: deprecate this once async transactions are enabled (we don't want please add bug # (I assume is the bug where we'll remove old transactions code) @@ +176,3 @@ > > _doShowEditBookmarkPanel: > + Task.async(function* (aNode, aAnchorElement, aPosition) { please oneline this as well. @@ +274,5 @@ > + if (this._batching) > + return; > + if (PlacesUIUtils.useAsyncTransactions) { > + PlacesTransactions.batch(function* () { > + let endBatchPromise = new Promise((resolve, rejct) => { typo: rejct @@ +277,5 @@ > + PlacesTransactions.batch(function* () { > + let endBatchPromise = new Promise((resolve, rejct) => { > + this._endAsyncTransactionsBatch = resolve; > + }); > + yield endBatchPromise; would PromiseUtils.defer() help here? While we're supposed to generally not use defer, in some cases it can really help clarifying code. In this case looks like it would look cleaner than storing the resolve function into a local property @@ +292,5 @@ > + return; > + > + if (PlacesUIUtils.useAsyncTransactions) { > + this._endAsyncTransactionsBatch(); > + this._endAsyncTransactionsBatch = () => {}; yeah, let's try to use a deferred if possible @@ +316,5 @@ > * aBrowser isn't bookmarked yet, defaults to the unfiled root. > * @param [optional] aShowEditUI > * whether or not to show the edit-bookmark UI for the bookmark item > */ > + bookmarkPage: Task.async(function* (aBrowser, aParent, aShowEditUI) { nit: old trailing spaces above @@ +394,5 @@ > + }), > + > + // TODO: Replace bookmarkPage code with this function once legacy > + // transactions are removed. > + _bookmarkPagePT: Task.async(function* (aBrowser, aParent, aShowEditUI) { aParentId please. @@ +399,5 @@ > + let url = new URL(aBrowser.currentURI.spec); > + let info = yield PlacesUtils.bookmarks.fetch({ url }); > + if (!info) { > + let parentGuid = aParent !== undefined ? > + yield PlacesUtils.promiseItemId(aParent) : promiseItemGuid @@ +417,5 @@ > + .test(webNav.document.documentURI); > + info.title = isErrorPage ? > + (yield PlacesUtils.promisePlaceInfo(aBrowser.currentURI)).title : > + webNav.document.title; > + title = title || url.href; shouldn't this be info.title = info.title... ? @@ +419,5 @@ > + (yield PlacesUtils.promisePlaceInfo(aBrowser.currentURI)).title : > + webNav.document.title; > + title = title || url.href; > + description = PlacesUIUtils.getDescriptionFromDocument(webNav.document); > + charset = webNav.document.characterSet; shouldn't you do "something" with the charset? The old code was setting it... @@ +431,5 @@ > + StarUI.beginBatch(); > + } > + > + info.annotations = [{ name: PlacesUIUtils.DESCRIPTION_ANNO > + , value: description }]; what if there's no description? do we set the anno regardless? @@ +481,5 @@ > * the address of the link target > * @param aTitle > * The link text > */ > + bookmarkLink: Task.async(function* (aParent, aURL, aTitle) { aParentId @@ +487,5 @@ > + if (PlacesUIUtils.useAsyncTransactions) { > + node = yield PlacesUIUtils.fetchNodeLike({ url: aURL }); > + } > + else { > + let itemId = PlacesUtils.getMostRecentBookmarkForURI(linkURI); this doesn't work cause you wrongly removed the linkURI definition ::: browser/components/places/PlacesUIUtils.jsm @@ +1216,5 @@ > + * Given a partial node-like object, having at least the itemId property set, this > + * method completes the rest of the properties necessary for initialising the edit > + * overlay with it. > + * > + * Use this method only if you must, it's likely to be removed in a future release. please better specify add-ons should not use this. @@ +1221,5 @@ > + * > + * @param aNodeLike > + * an object having at least the itemId nsINavHistoryResultNode property set, > + * along with any other properties available. > + */ trailing space @@ +1252,5 @@ > + if (!("type" in aNodeLike)) { > + defGetter("type", () => { > + if (aNodeLike.uri.length > 0) { > + if (/^place:/.test(aNodeLike.uri)) > + return Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT; well, not all place: queries are folder shortcuts... @@ +1290,5 @@ > + return Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER; > + if (this.uri.length == 0) > + throw new Error("Unexpected item type"); > + if (/^place:/.test(this.uri)) > + return Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT; ditto... this type helper could maybe be shared. @@ +1300,5 @@ > + /** > + * promiseNodeLikeFromFetchInfo(Bookmarks.fetch) > + * @see promiseNodeLikeFromFetchInfo above and Bookmarks.fetch in Bookmarks.jsm. > + */ > + fetchNodeLike: Task.async(function* (aGuidOrInfo) { considered the single use of this, I think you could omit it and just inline the code... ::: browser/components/places/content/bookmarkProperties.js @@ +42,3 @@ > * - "folder" (also applies to livemarks) > + * @ node (an nsINavHistoryResultNode object) - a node representing > + * the folder. how many add-ons could be broken by this node => id change? @@ +54,1 @@ > * - "folderPicker" - hides both the tree and the menu. did you file the bug to make copying a live bookmark copy the feed uri instead of the site uri? @@ +284,3 @@ > break; > case ACTION_ADD: > + yield this._promiseNewItem(); please don't make _promiseNewItem assign implicitly to this._node, make it return the node object and here assign to this._node explcitly @@ +287,5 @@ > + > + // Edit the new item > + gEditItemOverlay.initPanel({ node: this._node > + , hiddenRows: this._hiddenRows }); > + window.sizeToContent(); please add comment explaining why we need to resize now (and why not in the edit case?) @@ +368,5 @@ > break; > } > }, > > + _endAsyncTransactionsBatch() { }, ditto, if possible use a deferred. @@ +553,5 @@ > var transactions = []; > for (var i = 0; i < this._URIs.length; ++i) { > var uri = this._URIs[i]; > var title = this._getURITitleFromHistory(uri); > + var createTxn = new PlacesCreateBookmarkTransaction(uri, -1, since you're touching this, s/var/let @@ +609,5 @@ > > PlacesUtils.transactionManager.doTransaction(txn); > this._itemId = PlacesUtils.bookmarks.getIdForItemAt(container, index); > + > + this._node = Object.freeze({ as for promiseNewItem, please make the caller assign to this._node explicitly @@ +632,5 @@ > + { [BOOKMARK_FOLDER]: PlacesTransactions.NewFolder, > + [LIVEMARK_CONTAINER]: PlacesTransactions.NewLivemark, > + [BOOKMARK_ITEM]: PlacesTransactions.NewBookmark }[this._itemType]; > + > + let [container, index] = this._getInsertionPointDetails(); containerId ::: browser/components/places/content/controller.js @@ +656,2 @@ > PlacesUIUtils.showBookmarkDialog({ action: "edit" > + , node for shortcuts to roots and tag nodes, we were passing the concrete id before, that does work still correcty in the new code? ::: browser/components/places/content/editBookmarkOverlay.js @@ +58,5 @@ > + bulkTagging, uris, > + visibleRows }; > + }, > + > + get initialized() this._paneInfo != null, expression closures are deprecated, you'll have to get initialized() { return ... } see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Expression_closures @@ +66,5 @@ > + if (!this.initialized || this._paneInfo.bulkTagging) > + return -1; > + return this._paneInfo.itemId; > + }, > + get uri() { please add newline before @@ +73,5 @@ > + if (this._paneInfo.bulkTagging) > + return this._paneInfo.uris[0]; > + return this._paneInfo.uri; > + }, > + get multiEdit() this.initialized && this._paneInfo.bulkTagging, ditto (for newline and closure) @@ +95,5 @@ > + throw new Error("_initNamePicker called unexpectedly"); > + > + // title may by null, which, for us, is the same as an empty string. > + let title = typeof(this._paneInfo.title) == "string" ? > + this._paneInfo.title : ""; this._paneInfo.title || "" ? @@ +115,5 @@ > + }, > + > + _initKeywordField: Task.async(function* (aNewKeyword) { > + if (!this._paneInfo.isBookmark) > + throw new Error("_isBookmark called unexpectedly"); _isBookmark? @@ +209,5 @@ > + > + // Folder picker. > + // Technically we should check that the item is not moveable, but that's > + // not cheap (we don't always have the parent), and there's no use case for > + // this (it's only the Star UI that shows the folderPicker) i think some add-on is showing the picker everywhere, what risk do we take here? @@ +213,5 @@ > + // this (it's only the Star UI that shows the folderPicker) > + if (showOrCollapse("folderRow", isItem, "folderPicker")) { > + Task.spawn(function* () { > + let containerId = PlacesUtils.bookmarks.getFolderIdForItem(itemId); > + this._initFolderMenuList(containerId); what's the Task.spawn here for? @@ +554,3 @@ > } > + catch(ex) { > + // XXX Bug 1089141. please enter a proper TODO comment @@ +676,5 @@ > > if (aEvent.target.id == "editBMPanel_chooseFolderMenuItem") { > // reset the selection back to where it was and expand the tree > // (this menu-item is hidden when the tree is already visible > + let container = PlacesUtils.bookmarks.getFolderIdForItem(this._itemId); containerId @@ +686,5 @@ > return; > } > > // Move the item > + let container = this._getFolderIdFromMenuList(); containerId @@ +754,5 @@ > + } > + > + // Mark folder as recently used > + annotation = this._getLastUsedAnnotationObject(true); > + if (!PlacesUIUtils.useAsyncTransactions) { there's something fishy here, we are already in a !useAsyncTransactions if... ::: browser/components/places/content/places.js @@ +662,5 @@ > else > itemId = PlacesUtils._uri(aSelectedNode.uri); > > + gEditItemOverlay.initPanel({ node: aSelectedNode > + , hiddenRows: ["folderPicker"] }); all of the code above doing concreteId and readOnly guessing look no more needed ::: toolkit/components/places/PlacesUtils.jsm @@ +392,5 @@ > + * @param aNode > + * a result node. > + * @return the concrete item-guid for aNode. > + * @note unlike getConcreteItemId, this doesn't allow retrieving the guid of a > + * ta container. typo: ta container?

There are still a few fixes to do here and there, and I need to provide some answers regarding compatibility concerns.

Comment on attachment 8562011 [details] [diff] [review] address most of the review comments Review of attachment 8562011 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-places.js @@ +2,5 @@ > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +XPCOMUtils.defineLazyModuleGetter(this, "PromiseUtils", > + "resource://gre/modules/PromiseUtils.jsm"); I'd probably move this to browser.js, it's the same thing in the end, so better having all imports in a single place. @@ +270,5 @@ > this._actionOnHide = "remove"; > this.panel.hidePopup(); > }, > > + _batchBlockingPromiseDeferred: null, this still needs documentation, I'm sorry but batch-blocking-promise-deferred is very not self-documenting... I'm not even sure why it is names both as a promise and a deferred? @@ +417,5 @@ > .test(webNav.document.documentURI); > info.title = isErrorPage ? > (yield PlacesUtils.promisePlaceInfo(aBrowser.currentURI)).title : > webNav.document.title; > + info.title = title || url.href; title is not defined anywhere, if I'm not wrong... maybe it should be info.title = info.title || url.href? Do we have any kind of testing on this? ::: browser/components/places/PlacesUIUtils.jsm @@ +1280,1 @@ > if (!("uri" in aNodeLike)) { lots of newlines :) ::: browser/components/places/content/bookmarkProperties.js @@ +373,5 @@ > break; > } > }, > > + _batchBlockingPromiseDeferred: null, ditto on better documenting/naming this

> @@ +1290,5 @@ > > + return Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER; > > + if (this.uri.length == 0) > > + throw new Error("Unexpected item type"); > > + if (/^place:/.test(this.uri)) > > + return Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT; > > ditto... this type helper could maybe be shared. > > @@ +1300,5 @@ > > + /** > > + * promiseNodeLikeFromFetchInfo(Bookmarks.fetch) > > + * @see promiseNodeLikeFromFetchInfo above and Bookmarks.fetch in Bookmarks.jsm. > > + */ > > + fetchNodeLike: Task.async(function* (aGuidOrInfo) { > > considered the single use of this, I think you could omit it and just inline > the code... I think we're going to have more use cases for this, and I'd prefer to re-evaluate this once everything is settled (i.e. after it's clear if we need a shim for addons, and after Bookmarks.jsm is used everywhere). > ::: browser/components/places/content/bookmarkProperties.js > @@ +42,3 @@ > > * - "folder" (also applies to livemarks) > > + * @ node (an nsINavHistoryResultNode object) - a node representing > > + * the folder. > > how many add-ons could be broken by this node => id change? > It's hard to tell. Most consumer use the PUIU utilities for opening the dialog. Let's keep an eye open after this lands, and if it does raise more than a few compatibility issues, we can introduce a compatibility shim. My got feeling is that with all the changes that are ongoing, this one isn't going to be a major issue, relatively speaking.

Comment on attachment 8578461 [details] [diff] [review] updated full patch Review of attachment 8578461 [details] [diff] [review]: ----------------------------------------------------------------- I'm expecting regressions, cause it's very hard to find misses in patches refactoring so much code. but this should not prevent us from proceeding, we can deal with them. Just please do some manual testing of the main UI pieces (toolbar, menu, library addition and editing) and we should be fine. the new keywords API is also about to land (got review today). ::: browser/base/content/browser-places.js @@ +129,5 @@ > _overlayLoaded: false, > _overlayLoading: false, > + showEditBookmarkPopup: Task.async(function* (aNode, aAnchorElement, aPosition) { > + // Support the legacy aItemId signature for the time being. > + // TODO: Deprecate this once async transactions are enabled and the legacy The comments are a bit clashing, the former says that it will be supported for the time being, the latter says it should be deprecated asap... @@ +268,5 @@ > this.panel.hidePopup(); > }, > > + // editBookmarkOverlay implements an instant-apply UI, having no Undo > + // combined Undo/Redo support, matching the way it is used in the Library. I cannot understand what "no Undo combined Undo/Redo support" means @@ +269,5 @@ > }, > > + // editBookmarkOverlay implements an instant-apply UI, having no Undo > + // combined Undo/Redo support, matching the way it is used in the Library. > + // However, in this context (the Star UI) we have a Cancel button that its s/that its/whose/? @@ +270,5 @@ > > + // editBookmarkOverlay implements an instant-apply UI, having no Undo > + // combined Undo/Redo support, matching the way it is used in the Library. > + // However, in this context (the Star UI) we have a Cancel button that its > + // expected behavior is to undo all the operation done in the panel. s/operation/operations/ @@ +402,5 @@ > + > + // TODO: Replace bookmarkPage code with this function once legacy > + // transactions are removed. > + _bookmarkPagePT: Task.async(function* (aBrowser, aParentId, aShowEditUI) { > + let url = new URL(aBrowser.currentURI.spec); nit: fwiw, the bookmarks API also accepts nsIURI, so you could even avoid this and use .spec later ::: browser/components/places/PlacesUIUtils.jsm @@ +1213,5 @@ > return weaveEnabled || cloudSyncEnabled; > }, > + > + /** > + * WARNING TO ADDON AUTHORS: DO NOT USE THIS METHOD. IT"S LIKELY TO BE REMOVED IN A typo: '' @@ +1350,5 @@ > + }), > + > + /** > + * promiseNodeLikeFromFetchInfo(Bookmarks.fetch) > + * @see promiseNodeLikeFromFetchInfo above and Bookmarks.fetch in Bookmarks.jsm. I'm not sure what the first line is documenting... please fix this javadoc ::: browser/components/places/content/bookmarkProperties.js @@ +635,5 @@ > + > + let txnFunc = > + { [BOOKMARK_FOLDER]: PlacesTransactions.NewFolder, > + [LIVEMARK_CONTAINER]: PlacesTransactions.NewLivemark, > + [BOOKMARK_ITEM]: PlacesTransactions.NewBookmark }[this._itemType]; move }[this._itemType]; to a new line, aligned with the opening brace @@ +660,5 @@ > + info.postData = this._postData; > + > + // XXX TODO: this should be in a transaction! > + if (this._charSet && !PrivateBrowsingUtils.isWindowPrivate(window)) > + PlacesUtils.setCharsetForURI(this._uri, this._charSet); well, actually we will probably never put charset in a transaction... So please remove the XXX comment

Backed out due to test failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=7bf3039686a3 The failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=be0e33eb5d83

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=62292840b153

This one caught two true nasty errors.

another try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dc42014aab4

Updated to e10s changes. This /should/ fix the bookmarkProperties failure.

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #44) > Updated to e10s changes. This /should/ fix the bookmarkProperties failure. Can you push this to try please to make sure?

Try was closed at that time.

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=7f9114a84892 I've disabled the falling test in browser_bookmarkProperties, to be dealt with in a follow-up bug. While I couldn't reproduce the issue myself, I've an idea of how to take care of it, but I'd like to get a separate review for it first, so we'll do it there.

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2796476&repo=fx-team

Backout: https://hg.mozilla.org/integration/fx-team/rev/04406a15fbb7

Sigh, I guess I commented out the wrong test.

Back to try with the right test commented out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dd64b9849ea