Closed
Bug 1405722
Opened 6 years ago
Closed 6 years ago
Remove IsLivemark() from PlacesUIUtils
Categories
(Firefox :: Bookmarks & History, enhancement, P2)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: mak, Assigned: mak)
References
Details
(Keywords: perf, Whiteboard: [fxsearch])
Attachments
(1 file)
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.
Assignee | ||
Updated•6 years ago
|
Whiteboard: [fxsearch]
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee | ||
Updated•6 years ago
|
Summary: Figure out how to remove IsLivemark() from PlacesUtils → Remove IsLivemark() from PlacesUIUtils
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•6 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 6•6 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•6 years ago
|
||
you can check the 3-5 interdiff.
Comment hidden (mozreview-request) |
Comment 11•6 years ago
|
||
mozreview-review |
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+
Comment 12•6 years ago
|
||
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/af8fdfac66fa Remove the IsLivemark() bookmarks observer from PlacesUIUtils. r=standard8
![]() |
||
Comment 13•6 years ago
|
||
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)
Assignee | ||
Comment 14•6 years ago
|
||
ah well, that test is testing the code that I removed! I didn't think we had such a test.
Flags: needinfo?(mak77)
Comment hidden (mozreview-request) |
Comment 16•6 years ago
|
||
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/8981c30ec849 Remove the IsLivemark() bookmarks observer from PlacesUIUtils. r=standard8
![]() |
||
Comment 17•6 years ago
|
||
bugherder |
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.
Description
•