Closed
Bug 1439315
Opened 5 years ago
Closed 5 years ago
defineLazyScriptGetter controller.js
Categories
(Firefox :: Bookmarks & History, enhancement, P2)
Tracking
()
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: mak, Assigned: mak)
References
Details
(Keywords: perf, Whiteboard: [fxsearch])
Attachments
(10 files)
59 bytes,
text/x-review-board-request
|
standard8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
standard8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
standard8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
standard8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
standard8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
standard8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
standard8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
standard8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
standard8
:
review+
|
Details |
1.29 KB,
patch
|
Details | Diff | Splinter Review |
We could probably defineLazyScriptGetter controller.js, with some refactoring. I'd probably rename InsertionPoint to PlacesInsertionPoint, move some shared utils to PlacesUIUtils (goDoPlacesCommand, doGetPlacesControllerForCommand, goUpdatePlacesCommands) and then defineLazyScriptGetter for PlacesController, PlacesControllerDragHelper and PlacesInsertionPoint
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdf8ca2923b86a7ee7de76368ff1f6decbfbd7d8
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•5 years ago
|
||
mozreview-review |
Comment on attachment 8954815 [details] Bug 1439315 - 5 - defineLazyScriptGetter controller.js. https://reviewboard.mozilla.org/r/223962/#review229952 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/places/content/controller.js:219 (Diff revision 1) > if (PlacesUtils.nodeIsHost(this._view.selectedNode)) { > var queries = this._view.selectedNode.getQueries(); > host = queries[0].domain; > } else > host = Services.io.newURI(this._view.selectedNode.uri).host; > ForgetAboutSite.removeDataFromDomain(host) Error: 'forgetaboutsite' is not defined. [eslint: no-undef]
Comment 11•5 years ago
|
||
mozreview-review |
Comment on attachment 8954816 [details] Bug 1439315 - 6 - Move some utils into PlacesController or PlacesUIUtils. https://reviewboard.mozilla.org/r/223964/#review229958 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/places/content/controller.js:219 (Diff revision 1) > if (PlacesUtils.nodeIsHost(this._view.selectedNode)) { > var queries = this._view.selectedNode.getQueries(); > host = queries[0].domain; > } else > host = Services.io.newURI(this._view.selectedNode.uri).host; > ForgetAboutSite.removeDataFromDomain(host) Error: 'forgetaboutsite' is not defined. [eslint: no-undef]
Comment 12•5 years ago
|
||
mozreview-review |
Comment on attachment 8954812 [details] Bug 1439315 - 1 - rename InsertionPoint to PlacesInsertionPoint. https://reviewboard.mozilla.org/r/223956/#review230194 Makes sense, much clearer
Attachment #8954812 -
Flags: review?(standard8) → review+
Comment 13•5 years ago
|
||
mozreview-review |
Comment on attachment 8954813 [details] Bug 1439315 - 2 - Move command utils to PlacesUIUtils. https://reviewboard.mozilla.org/r/223958/#review230196
Attachment #8954813 -
Flags: review?(standard8) → review+
Comment 14•5 years ago
|
||
mozreview-review |
Comment on attachment 8954814 [details] Bug 1439315 - 3 - Don't use NetUtil.newURI. https://reviewboard.mozilla.org/r/223960/#review230198
Attachment #8954814 -
Flags: review?(standard8) → review+
Comment 15•5 years ago
|
||
mozreview-review |
Comment on attachment 8954815 [details] Bug 1439315 - 5 - defineLazyScriptGetter controller.js. https://reviewboard.mozilla.org/r/223962/#review230200
Attachment #8954815 -
Flags: review?(standard8) → review+
Comment 16•5 years ago
|
||
mozreview-review |
Comment on attachment 8954816 [details] Bug 1439315 - 6 - Move some utils into PlacesController or PlacesUIUtils. https://reviewboard.mozilla.org/r/223964/#review230206
Attachment #8954816 -
Flags: review?(standard8) → review+
Comment 17•5 years ago
|
||
mozreview-review |
Comment on attachment 8954817 [details] Bug 1439315 - 7 - Don't import PlacesUtils on PlacesUIUtils import. https://reviewboard.mozilla.org/r/223966/#review230208
Attachment #8954817 -
Flags: review?(standard8) → review+
Comment 18•5 years ago
|
||
mozreview-review |
Comment on attachment 8954818 [details] Bug 1439315 - 8 - Remove some dangling NetUtil usage. https://reviewboard.mozilla.org/r/223968/#review230210
Attachment #8954818 -
Flags: review?(standard8) → review+
Comment 19•5 years ago
|
||
mozreview-review |
Comment on attachment 8954819 [details] Bug 1439315 - 4 - Make ESLint happy. https://reviewboard.mozilla.org/r/223970/#review230214 Nice, thanks for the cleanups as well.
Attachment #8954819 -
Flags: review?(standard8) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•5 years ago
|
||
mozreview-review |
Comment on attachment 8955119 [details] Bug 1439315 - 9 - Remove no more necessary placesOverlayModules. https://reviewboard.mozilla.org/r/224274/#review230234 ::: tools/lint/eslint/eslint-plugin-mozilla/lib/environments/places-overlay.js:19 (Diff revision 1) > // Rule Definition > // ----------------------------------------------------------------------------- > var path = require("path"); > var helpers = require("../helpers"); > var globals = require("../globals"); > var modules = helpers.modulesGlobalData; Please can you also remove this line, we don't need to import modules now.
Attachment #8955119 -
Flags: review?(standard8) → review+
Comment 27•5 years ago
|
||
mozreview-review |
Comment on attachment 8954815 [details] Bug 1439315 - 5 - defineLazyScriptGetter controller.js. https://reviewboard.mozilla.org/r/223962/#review230238 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/places/content/controller.js:219 (Diff revision 2) > if (PlacesUtils.nodeIsHost(this._view.selectedNode)) { > var queries = this._view.selectedNode.getQueries(); > host = queries[0].domain; > } else > host = Services.io.newURI(this._view.selectedNode.uri).host; > ForgetAboutSite.removeDataFromDomain(host) Error: 'forgetaboutsite' is not defined. [eslint: no-undef]
Comment 28•5 years ago
|
||
mozreview-review |
Comment on attachment 8954816 [details] Bug 1439315 - 6 - Move some utils into PlacesController or PlacesUIUtils. https://reviewboard.mozilla.org/r/223964/#review230240 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/places/content/controller.js:219 (Diff revision 2) > if (PlacesUtils.nodeIsHost(this._view.selectedNode)) { > var queries = this._view.selectedNode.getQueries(); > host = queries[0].domain; > } else > host = Services.io.newURI(this._view.selectedNode.uri).host; > ForgetAboutSite.removeDataFromDomain(host) Error: 'forgetaboutsite' is not defined. [eslint: no-undef]
Comment 29•5 years ago
|
||
mozreview-review |
Comment on attachment 8955119 [details] Bug 1439315 - 9 - Remove no more necessary placesOverlayModules. https://reviewboard.mozilla.org/r/224274/#review230242 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: tools/lint/eslint/eslint-plugin-mozilla/lib/environments/places-overlay.js:19 (Diff revision 1) > // Rule Definition > // ----------------------------------------------------------------------------- > var path = require("path"); > var helpers = require("../helpers"); > var globals = require("../globals"); > var modules = helpers.modulesGlobalData; Error: 'modules' is assigned a value but never used. allowed unused vars must match /^exported_symbols$/. [eslint: no-unused-vars]
Comment hidden (mozreview-request) |
Comment 31•5 years ago
|
||
mozreview-review |
Comment on attachment 8954815 [details] Bug 1439315 - 5 - defineLazyScriptGetter controller.js. https://reviewboard.mozilla.org/r/223962/#review230256 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/places/content/controller.js:219 (Diff revision 2) > if (PlacesUtils.nodeIsHost(this._view.selectedNode)) { > var queries = this._view.selectedNode.getQueries(); > host = queries[0].domain; > } else > host = Services.io.newURI(this._view.selectedNode.uri).host; > ForgetAboutSite.removeDataFromDomain(host) Error: 'forgetaboutsite' is not defined. [eslint: no-undef]
Comment 32•5 years ago
|
||
mozreview-review |
Comment on attachment 8954816 [details] Bug 1439315 - 6 - Move some utils into PlacesController or PlacesUIUtils. https://reviewboard.mozilla.org/r/223964/#review230260 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/places/content/controller.js:219 (Diff revision 2) > if (PlacesUtils.nodeIsHost(this._view.selectedNode)) { > var queries = this._view.selectedNode.getQueries(); > host = queries[0].domain; > } else > host = Services.io.newURI(this._view.selectedNode.uri).host; > ForgetAboutSite.removeDataFromDomain(host) Error: 'forgetaboutsite' is not defined. [eslint: no-undef]
Comment 33•5 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 6bf5234d8d439d6252bf1bbabb7f2f730bd8a5a5 -d 993b34424c28: rebasing 450017:6bf5234d8d43 "Bug 1439315 - 1 - rename InsertionPoint to PlacesInsertionPoint. r=standard8" merging browser/base/content/browser-places.js merging browser/components/places/content/bookmarkProperties.js merging browser/components/places/content/browserPlacesViews.js merging browser/components/places/content/controller.js merging browser/components/places/content/editBookmarkOverlay.js merging browser/components/places/content/menu.xml merging browser/components/places/content/tree.xml merging browser/components/places/content/treeView.js warning: conflicts while merging browser/components/places/content/controller.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 43•5 years ago
|
||
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/ee1319f54f8a 1 - rename InsertionPoint to PlacesInsertionPoint. r=standard8 https://hg.mozilla.org/integration/autoland/rev/377b3fd169ba 2 - Move command utils to PlacesUIUtils. r=standard8 https://hg.mozilla.org/integration/autoland/rev/fae3774ebf92 3 - Don't use NetUtil.newURI. r=standard8 https://hg.mozilla.org/integration/autoland/rev/dd986854e840 4 - Make ESLint happy. r=standard8 https://hg.mozilla.org/integration/autoland/rev/90db61f59b1d 5 - defineLazyScriptGetter controller.js. r=standard8 https://hg.mozilla.org/integration/autoland/rev/a06f224bb950 6 - Move some utils into PlacesController or PlacesUIUtils. r=standard8 https://hg.mozilla.org/integration/autoland/rev/891a073b9869 7 - Don't import PlacesUtils on PlacesUIUtils import. r=standard8 https://hg.mozilla.org/integration/autoland/rev/00041cf9f9bc 8 - Remove some dangling NetUtil usage. r=standard8 https://hg.mozilla.org/integration/autoland/rev/b05ca0b26e73 9 - Remove no more necessary placesOverlayModules. r=standard8
Comment 44•5 years ago
|
||
mozreview-review |
Comment on attachment 8954815 [details] Bug 1439315 - 5 - defineLazyScriptGetter controller.js. https://reviewboard.mozilla.org/r/223962/#review230294 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/places/content/controller.js:227 (Diff revision 3) > if (PlacesUtils.nodeIsHost(this._view.selectedNode)) { > var queries = this._view.selectedNode.getQueries(); > host = queries[0].domain; > } else > host = Services.io.newURI(this._view.selectedNode.uri).host; > ForgetAboutSite.removeDataFromDomain(host) Error: 'forgetaboutsite' is not defined. [eslint: no-undef]
Comment 45•5 years ago
|
||
mozreview-review |
Comment on attachment 8954816 [details] Bug 1439315 - 6 - Move some utils into PlacesController or PlacesUIUtils. https://reviewboard.mozilla.org/r/223964/#review230300 Code analysis found 2 defects in this patch: - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/places/PlacesUIUtils.jsm:1429 (Diff revision 3) > + } > + > + // If this is not a copy, check for safety that we can move the > + // source, otherwise report an error and fallback to a copy. > + if (!doCopy && !canMoveUnwrappedNode(item)) { > + Components.utils.reportError("Tried to move an unmovable Places " + Error: Use cu rather than components.utils [eslint: mozilla/use-cc-etc] ::: browser/components/places/content/controller.js:227 (Diff revision 3) > if (PlacesUtils.nodeIsHost(this._view.selectedNode)) { > var queries = this._view.selectedNode.getQueries(); > host = queries[0].domain; > } else > host = Services.io.newURI(this._view.selectedNode.uri).host; > ForgetAboutSite.removeDataFromDomain(host) Error: 'forgetaboutsite' is not defined. [eslint: no-undef]
Comment 46•5 years ago
|
||
mozreview-review |
Comment on attachment 8954817 [details] Bug 1439315 - 7 - Don't import PlacesUtils on PlacesUIUtils import. https://reviewboard.mozilla.org/r/223966/#review230302 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/places/PlacesUIUtils.jsm:1434 (Diff revision 3) > } > > // If this is not a copy, check for safety that we can move the > // source, otherwise report an error and fallback to a copy. > if (!doCopy && !canMoveUnwrappedNode(item)) { > Components.utils.reportError("Tried to move an unmovable Places " + Error: Use cu rather than components.utils [eslint: mozilla/use-cc-etc]
Assignee | ||
Comment 47•5 years ago
|
||
Comment 48•5 years ago
|
||
Pushed by ncsoregi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b5c420d486db eslint followup. r=mak
Comment 49•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ee1319f54f8a https://hg.mozilla.org/mozilla-central/rev/377b3fd169ba https://hg.mozilla.org/mozilla-central/rev/fae3774ebf92 https://hg.mozilla.org/mozilla-central/rev/dd986854e840 https://hg.mozilla.org/mozilla-central/rev/90db61f59b1d https://hg.mozilla.org/mozilla-central/rev/a06f224bb950 https://hg.mozilla.org/mozilla-central/rev/891a073b9869 https://hg.mozilla.org/mozilla-central/rev/00041cf9f9bc https://hg.mozilla.org/mozilla-central/rev/b05ca0b26e73 https://hg.mozilla.org/mozilla-central/rev/b5c420d486db
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
![]() |
||
Comment 50•5 years ago
|
||
The bunch of this bug lacks of reliability. I think this bug should be backed out until automated tests are provided.
Flags: needinfo?(mak77)
Assignee | ||
Comment 51•5 years ago
|
||
Please stop asking for things to be backed out for minor breakage.
Flags: needinfo?(mak77)
You need to log in
before you can comment on or make changes to this bug.
Description
•