1405722 - Remove IsLivemark() from PlacesUIUtils

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement, P2)

Description

[Marco Bonardo [:mak]]

This simple check adds a bookmarks observer, that by its own nature is expensive.

There are 2 possibilities I may think of here:
1. Re-use the livemarks service internal cache and provide a synchronous isLivemark from it. It will still be cheaper even if not great.
2. if isContentsReadOnly always gets a Places node as input, we could maybe try to use controller.hasCachedLivemarkInfo(placesNode) instead of isLivemark().

The latter would be preferrable.

Depends on bug 1405687 mostly for bitrotting reasons.

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1405722 - Remove the IsLivemark() bookmarks observer from PlacesUIUtils.

Review commit: https://reviewboard.mozilla.org/r/188758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188758/

Comment 2

[Marco Bonardo [:mak]]

some failures in b-c tests:
browser/components/places/tests/browser/browser_bookmarkProperties_addFolderDefaultButton.js | Test timed out [log…]
browser/components/places/tests/browser/browser_bookmarkProperties_addLivemark.js | Test timed out [log…]
browser/components/places/tests/browser/browser_bookmarkProperties_bookmarkAllTabs.js | Name field is writable - false == true

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8917807 [details]
Bug 1405722 - Remove the IsLivemark() bookmarks observer from PlacesUIUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188758/diff/1-2/

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8917807 [details]
Bug 1405722 - Remove the IsLivemark() bookmarks observer from PlacesUIUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188758/diff/2-3/

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8917807 [details]
Bug 1405722 - Remove the IsLivemark() bookmarks observer from PlacesUIUtils.

https://reviewboard.mozilla.org/r/188758/#review194442

Generally looking good, I'd just like to see it with the other test fixed.

::: browser/components/places/PlacesUIUtils.jsm:819
(Diff revision 3)
> -   *        any item id or result node.
> -   * @throws if aNodeOrItemId is neither an item id nor a folder result node.
>     * @note livemark "folders" are considered read-only (but see bug 1072833).
> -   * @return true if aItemId points to a read-only folder, false otherwise.
> +   * @return true if aPlacesNode is a read-only folder, false otherwise.
>     */
> -  isContentsReadOnly(aNodeOrItemId) {
> +  isFolderReadOnly(aPlacesNode, aView) {

nit: drop the 'a' prefix from the variable names like you're starting to do elsewhere?

::: browser/components/places/content/controller.js:1562
(Diff revision 3)
> -  canMoveUnwrappedNode(aUnwrappedNode) {
> -    return aUnwrappedNode.id > 0 &&
> -           !PlacesUtils.isRootItem(aUnwrappedNode.id) &&
> -           (!aUnwrappedNode.parent || !PlacesUIUtils.isContentsReadOnly(aUnwrappedNode.parent)) &&
> -           aUnwrappedNode.parent != PlacesUtils.tagsFolderId &&
> -           aUnwrappedNode.grandParentId != PlacesUtils.tagsFolderId;
> +  canMoveUnwrappedNode(unwrappedNode) {
> +    if (unwrappedNode.id <= 0 || PlacesUtils.isRootItem(unwrappedNode.id)) {
> +      return false;
> +    }
> +    let parentId = unwrappedNode.parent;
> +    if (parentId > 0) {

You could negate this if, and `return false` early, then the function would be a straight list of if statements. Or better still, you could incorporate it in the following if statement.

Comment 6

[Marco Bonardo [:mak]]

I think I figured out the test problem, and it's interesting. Pratically head.js restored the leftPaneFolderId getter after every test... but it doesn't restore the allBookmarksFolderId getter. So some tests may be working on zombie nodes.
I actually wonder if this is somehow related to bug 658744. Maybe not, who knows. I could try on Try!

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8917807 [details]
Bug 1405722 - Remove the IsLivemark() bookmarks observer from PlacesUIUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188758/diff/3-4/

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8917807 [details]
Bug 1405722 - Remove the IsLivemark() bookmarks observer from PlacesUIUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188758/diff/4-5/

Comment 9

[Marco Bonardo [:mak]]

you can check the 3-5 interdiff.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8917807 [details]
Bug 1405722 - Remove the IsLivemark() bookmarks observer from PlacesUIUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188758/diff/5-6/

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8917807 [details]
Bug 1405722 - Remove the IsLivemark() bookmarks observer from PlacesUIUtils.

https://reviewboard.mozilla.org/r/188758/#review194910

Thanks for the updates, looks good. r=Standard8

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af8fdfac66fa
Remove the IsLivemark() bookmarks observer from PlacesUIUtils. r=standard8

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing xpcshell's browser/components/places/tests/unit/test_PUIU_livemarksCache.js, at least on OS X 10.10 opt:

https://hg.mozilla.org/integration/autoland/rev/3e08e502df69a2a4044f184adc8c807dcb2bda99

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=af8fdfac66fa0d6eed52bcdbbeccbc539996a69f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=137303928&repo=autoland

15:48:18  WARNING -  TEST-UNEXPECTED-FAIL | browser/components/places/tests/unit/test_PUIU_livemarksCache.js | xpcshell return code: 0
15:48:18     INFO -  TEST-INFO took 368ms
15:48:18     INFO -  >>>>>>>
15:48:18     INFO -  PID 6653 | JavaScript strict warning: resource:///modules/PlacesUIUtils.jsm, line 31: ReferenceError: reference to undefined property "processType"
15:48:18     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
15:48:18     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
15:48:18     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
15:48:18     INFO -  running event loop
15:48:18     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "processType"" {file: "resource:///modules/PlacesUIUtils.jsm" line: 31}]"
15:48:18     INFO -  browser/components/places/tests/unit/test_PUIU_livemarksCache.js | Starting test_livemark_cache_builtin_folder
15:48:18     INFO -  (xpcshell/head.js) | test test_livemark_cache_builtin_folder pending (2)
15:48:18     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
15:48:18     INFO -  Unexpected exception TypeError: IsLivemark is not a function at /Users/cltbld/tasks/task_1508192869/build/tests/xpcshell/tests/browser/components/places/tests/unit/test_PUIU_livemarksCache.js:8
15:48:18     INFO -  test_livemark_cache_builtin_folder@/Users/cltbld/tasks/task_1508192869/build/tests/xpcshell/tests/browser/components/places/tests/unit/test_PUIU_livemarksCache.js:8:14
15:48:18     INFO -  asyncFunction@resource://gre/modules/Task.jsm:241:18
15:48:18     INFO -  Task_spawn@resource://gre/modules/Task.jsm:166:12
15:48:18     INFO -  _run_next_test@/Users/cltbld/tasks/task_1508192869/build/tests/xpcshell/head.js:1488:9
15:48:18     INFO -  run@/Users/cltbld/tasks/task_1508192869/build/tests/xpcshell/head.js:701:9
15:48:18     INFO -  _do_main@/Users/cltbld/tasks/task_1508192869/build/tests/xpcshell/head.js:221:3
15:48:18     INFO -  _execute_test@/Users/cltbld/tasks/task_1508192869/build/tests/xpcshell/head.js:544:5
15:48:18     INFO -  @-e:1:1
15:48:18     INFO -  exiting test

Comment 14

[Marco Bonardo [:mak]]

ah well, that test is testing the code that I removed! I didn't think we had such a test.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8917807 [details]
Bug 1405722 - Remove the IsLivemark() bookmarks observer from PlacesUIUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188758/diff/6-7/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8981c30ec849
Remove the IsLivemark() bookmarks observer from PlacesUIUtils. r=standard8

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8981c30ec849