set/getFolderReadOnly is not something related to bookmarks, it should be moved to PlacesUtils... the consumers are mostly js, historyresults are using it to set childrenReadOnly, but that's crazy, the childrenReadonly consumers could just use PlacesUtils.

So, even in PlacesUtils, this seems wrong. Consider: 1. Non-folder query nodes are read-only be definition. 2. Only the following folder nodes are read-only: (a) livemarks; (b) the left-pane query. 3. The nature of such an API must be synchronous, because its getters would be the dragover event and isCommandEnabled. So, I'd rather add some synchronous canUserModifyContents(node) utility to Placs_UI_Utils, that's by no means extendible (i.e. it would just check for the node type, and if it's a folder node, it'd also check if it's id is leftPaneFolderId or if it's a livemark).

I think originally the pan was to allow add-ons to set folders as read-only, but I don't think we care about that. Something that only supports our use-cases should be fine here.

I think also allBookmarksfolderId should be read-only, not just the left pane.

Yeah, will do. I think this was actually about remote containers and such useful concepts.

I didn't write a test for the new behavior of excludeReadOnlyFolders. Feels wrong, but I could do it if necessary. I didn't pay much attention to browser_423515.js, just made sure it passes. It messes too much with implementation internals.

Upgrading points to reflect reality; also setting qa+ to ensure C&P and D&D are tested across all Places UI.

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #6) > Upgrading points to reflect reality; also setting qa+ to ensure C&P and D&D > are tested across all Places UI. In that case, please provide steps to verify. From all that is in the bug right now, it sounds like something that is just about where some code lives and that's something we don't need to verify. Also note that we are more and more moving to only verify the harder cases and not the easy-to-see stuff as we have too few resources and the easy-to-see stuff is covered by casual Nightly testing anyhow.

The "bug" itself cannot be verified, because there's no bug. But QA should focus on on Places d&d, copy-paste and UI-commands to ensure they have not regressed. That means, for instance: (1) The "All bookmarks root" cannot be manipulated at all, in all views. (2) default drag action (copy vs. move) is unchanged for all container types (folders, livemarks, tags, the tags root). (3) In read-only containers commands are properly enabled/disabled (e.g. New Bookmark is disabled under a live bookmark folder). Ideally there would be sort sort of test suit I could point to...

(In reply to Mano from comment #6) > Upgrading points to reflect reality We don't do this generally - the estimate being wrong in retrospect is not a problem.

Comment on attachment 8496768 [details] [diff] [review] Something like this... Review of attachment 8496768 [details] [diff] [review]: ----------------------------------------------------------------- There are missing checks, afaict ::: browser/components/places/PlacesUIUtils.jsm @@ +564,5 @@ > + * Check whether or not the given node represents a removable entry (either in > + * history or in bookmarks). > + * > + * @param aNode > + * a result node (that's not the root node of a query). "a node, except the root node of a query" @@ +602,5 @@ > + let itemId = aNode.itemId; > + return itemId != this.leftPaneFolderId && > + itemId != this.allBookmarksFolderId && > + !PlacesUtils.annotations.itemHasAnnotation(aNode.itemId, > + PlacesUtils.LMANNO_FEEDURI); so, I know we decided this is fine for now, but looking at the consumers, we are calling this during dragover, that means we'd run a main-thread query continuously during the drag. I fear to address this concern for now we'll need a cache like the old _readOnly, using getItemsWithAnnotation and keeping the results up-to-date through listeners. ::: browser/components/places/content/controller.js @@ +199,5 @@ > case "placesCmd_sortBy:name": > var selectedNode = this._view.selectedNode; > return selectedNode && > PlacesUtils.nodeIsFolder(selectedNode) && > + PlacesUIUtils.isEditableBookmarksFolderNode(selectedNode) && you can remove nodeIsFolder from here @@ +1547,5 @@ > return aUnwrappedNode.id > 0 && > !PlacesUtils.isRootItem(aUnwrappedNode.id) && > aUnwrappedNode.parent != PlacesUtils.placesRootId && > aUnwrappedNode.parent != PlacesUtils.tagsFolderId && > + aUnwrappedNode.grandParentId != PlacesUtils.tagsFolderId; you removed the parentReadOnly check from here but you didn't replace it with any different check... why is this a safe change? Shouldn't it check at least allBookmarksFolderId and leftPaneFolderId? @@ +1563,5 @@ > // Can't move query root. > + let parentNode = aNode.parent; > + return parentNode && > + !PlacesUtils.nodeIsTagQuery(parentNode) && > + !PlacesUIUtils.isEditableBookmarksFolderNode(parentNode); the previous version was using canMoveContainer, that was doing more checks that are lacking here: 1. it was returning false if itemId == -1 for a container. I think this was for queries inside query, we still have those. 2. was disallowing moving roots while we don't expose roots, one could create place:folder=PLACES_ROOT and this would expose roots. 3. it was checking read-only for the CONCRETE id of the folder. The fact is that the new code acts on ids, but not on concrete ids, so you can easily create a folder shortcut to a read-only folder... and that would not be read-only. 4. was returning false if parent had .childrenReadOnly (through nodeIsReadOnly). now that is not just folders since .childrenReadOnly defaulted to true for queries (but RESULTS_AS_TAG_CONTENTS was explicitly excluded) ::: browser/components/places/content/treeView.js @@ +1662,2 @@ > PlacesUtils.nodeIsSeparator(node)) > return false; the previous version was using nodeIsReadOnly that used .childrenReadOnly that was true for queries (apart from RESULTS_AS_TAG_CONTENTS) That doesn't look very useful though, here we don't want the user to edit titles of leftPaneFolderId and allBookmarksFolderId children, and eventually the roots (in case one creates a shortcut to placesRoot) so, at a minimum should also check isEditableBookmarksFolderNode(node.parent)

Comment on attachment 8497353 [details] [diff] [review] take 2 Review of attachment 8497353 [details] [diff] [review]: ----------------------------------------------------------------- crossing fingers! ::: browser/components/places/PlacesUIUtils.jsm @@ +636,5 @@ > + * Only folder item should be checked by this method, and that is enforced > + * when aNodeOrItemId is a node. To avoid DB queries, it's not enforced when > + * aNodeOrItemId is an item id (false is returned in that case), but > + * To avoid an additional db query, if an item id of a bookmark or a sep > + * False is returned for all non-folder items. This also applies to folder I read this twice and still trying to guess what it means :) Rephrase please? ::: browser/components/places/content/controller.js @@ +1569,5 @@ > + let parentNode = aNode.parent; > + return parentNode != null && > + !(PlacesUtils.nodeIsFolder(parentNode) && > + PlacesUIUtils.isContentsReadOnly(parentNode)) && > + !PlacesUtils.nodeIsTagQuery(aNode) && why this check? tag queries should have itemId == -1, thus already filtered out above. It's correct to check the parentNode instead since children of tag queries have itemIds (for now) @@ +1674,5 @@ > // Allow dropping into Tag containers. > if (PlacesUtils.nodeIsTagQuery(aContainer)) > return false; > + > + return PlacesUIUtils.isContentsReadOnly(aContainer); won't this throw if aContainer is a query? Or are we guaranteed that when we call disallowInsertion we are always inside a folder? I don't have that much trust into our code... could you still please return !PlacesUtils.nodeIsFolder(aContainer) || isContentsReadOnly? ::: browser/components/places/content/treeView.js @@ +1666,1 @@ > // * separators Actually, also livemarks children are non editable, please add a comment above that we are managing that through the itemId == -1 check? @@ +1672,3 @@ > return false; > > + let parentId = node.parent.itemId; should we null check node.parent? I assume it's impossible the ui will show a query root though... But what if we have a shortcut to All Bookmarks or to LeftPaneFolderId? then one should still not be able to edit its children. So I guess _here_ you might want the parent concrete id.

> But what if we have a shortcut to All Bookmarks or to LeftPaneFolderId? > then one should still not be able to edit its children. IsEditable is not about the children, it's about the in-tree editing of titles.

> should we null check node.parent? I assume it's impossible the ui will show a query root > though... Yep. IsEditable is called by treeboxobject for tree rows, meaning it infers nodes from rows and thus cannot accidentally access the root node, which, indeed, is always invisible.

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #13) > > But what if we have a shortcut to All Bookmarks or to LeftPaneFolderId? > > then one should still not be able to edit its children. > > IsEditable is not about the children, it's about the in-tree editing of > titles. yes, I know, I'm saying you are checking node.parent.itemId, and disallowing editing the title if parent is leftPaneFolderId for example. so you disallow editing (the title of) children of leftPaneFolderId. I'm saying that one may create a shortcut to leftPaneFolderId, and then your patch would allow to edit titles of its children.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #15) > (In reply to Mano (::mano, needinfo? for any questions; not reading general > bugmail) from comment #13) > > > But what if we have a shortcut to All Bookmarks or to LeftPaneFolderId? > > > then one should still not be able to edit its children. > > > > IsEditable is not about the children, it's about the in-tree editing of > > titles. > > yes, I know, I'm saying you are checking node.parent.itemId, and disallowing > editing the title if parent is leftPaneFolderId for example. so you disallow > editing (the title of) children of leftPaneFolderId. > I'm saying that one may create a shortcut to leftPaneFolderId, and then your > patch would allow to edit titles of its children. Sigh, I saw this comment only after I checked in the patch.

Fix that + two typos: https://hg.mozilla.org/integration/fx-team/rev/5a6b95c47b55

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

I cannot reproduce the test failure. :-/

Double checking it was not something else: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=31afcbcde0ca I don't really get what's that failing line trying to do, but my guess is that some command-updating that happens in the background of another test (doesn't have to be - and probably isn't - a places-related test at all) results in a a call to isContentsReadOnly, which invokes the lazy leftPaneFolderId getter (that getter replaces itself with a simple value property), and so when the places tests try to cache the getter (cachedLeftPaneFolderIdGetter), it's too late for them. The getter is gone already.

Now, this means that a side-effect of my patch is that command-updating can result in earlier creation of the left pane folder. That's not so great, regardless of tests.

so yes, the failure means cachedLeftPaneFolderIdGetter is invalid, probably it's undefined, that means by the time the first places head runs, something else has already invoked leftPaneFolderId lazy getter. I wonder if we could check if they are getters before using them for our checks here, and if they are skip the check. The problem is that they could exist already (created by a previous session) and then our method wouldn't return the right result if called wrongly, though those methods should only be used by the ui, that means when they matter, they should have been already resolved.

the earlier reviewed (comments addressed), the additional checkin and the additional fix I just posted. I'll push it to try in a bit.

Comment on attachment 8504628 [details] [diff] [review] the fix Review of attachment 8504628 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/PlacesUIUtils.jsm @@ +658,5 @@ > else { > throw new Error("invalid value for aNodeOrItemId"); > } > > + if (IsLivemark(itemId) || itemId == PlacesUtils.placesRootId) cheaper check should go first (even if it's less likely) @@ +669,5 @@ > + // isCommandEnabled, to ever be the one that invokes it first, especially > + // because isCommandEnabled may be called way before the left pane folder is > + // even created (for example, if the user only uses the bookmarks menu or > + // toolbar for managing bookmarks). To do so, we avoid comparing to those > + // special folder if the lazy getter is still in place. This is safe merely The comment is so long that I got annoyed after half of it and I stopped reading it :) can we stop at "is still in place." please? The other details are not very useful, add-ons can break us in so many ways it's not even worth enumerating them.

Looks good: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7ea42d735d28

I started performing regression testing on this fix per Comment 8, but there are quite a few scenarios to look at. Mano, here's a list [1] of potential issues I found so far. [1] https://etherpad.mozilla.org/Bug1068671

one of those failures is bug 1083376.

Mano, could you please take bug 1083376 and verify if the list Andrei provided has some valid regressions we can fix in follow-ups? A couple looks like are effective regressions.

Any updates on this? All testing activities performed for this fix have been completed a few days ago, with all the issues specified in the etherpad from Comment 30.

Andrei, thanks a ton for the QA work done here. I'm looking into your list of issues (some of them are probably around for a while now) and will file bugs as necessary. Closing this for now.

Just learned about this change from an add-on dev who was affected by it. Is there a recommendation for what add-ons should be doing instead?

the first question is why the add-on needs this in first stance, is it trying to set a folder as readonly? is it trying to copy our views code and now missing this? likely it's using it for the wrong reasons. btw, queries always have readonly content, folders can be checked through PlacesUtils.nodeIsFolder(node) && PlacesUIUtils.isContentsReadOnly(node)

for a single node, there is also PlacesUIUtils.canUserRemove(node)

Might be helpful to update the documentation that nodeIsReadOnly() doesn't exist any more: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Places_utilities_for_JavaScript

Comment 35 refers to Bookmark Favicon Changer. See the 4th FAQ item on its home page: https://sites.google.com/site/sonthakit/bookmarkfaviconchanger See also "Bookmark Favicon Changer and all of my add-ons were banned" at mozillaZine: http://forums.mozillazine.org/viewtopic.php?f=19&t=2855837 Quoting sonthakit (the add-on author) on that forum, > Removing this function do not make firefox faster or more stable. Removing this > function make only one thing, BUG. I fight the AMO for this idea for a long time… > Developer need backward compatibility as much as possible.!!! Removing or changing > existing function should not happen except it is absolutely critical necessary. If > AMO need new thing, just make new function, don't modified the old one. Hopefully the response in comment 36 can help the add-on developer, though the dev's patience for AMO may have already been worn too thin to see a return of the add-on.