Closed Bug 1405722 Opened 6 years ago Closed 6 years ago

Remove IsLivemark() from PlacesUIUtils

Categories

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

Product:
Firefox
Component:
Bookmarks & History
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: perf, Whiteboard: [fxsearch])

Attachments

(1 file)

Bug 1405722 - Remove the IsLivemark() bookmarks observer from PlacesUIUtils.
59 bytes, text/x-review-board-request
standard8
: review+
Details 
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.
Whiteboard: [fxsearch]
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Summary: Figure out how to remove IsLivemark() from PlacesUtils → Remove IsLivemark() from PlacesUIUtils
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 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.
Attachment #8917807 - Flags: review?(standard8)
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!
you can check the 3-5 interdiff.
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
Attachment #8917807 - Flags: review?(standard8) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/af8fdfac66fa
Remove the IsLivemark() bookmarks observer from PlacesUIUtils. r=standard8
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
Flags: needinfo?(mak77)
ah well, that test is testing the code that I removed! I didn't think we had such a test.
Flags: needinfo?(mak77)
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/8981c30ec849
Remove the IsLivemark() bookmarks observer from PlacesUIUtils. r=standard8
https://hg.mozilla.org/mozilla-central/rev/8981c30ec849
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.