defineLazyScriptGetter controller.js

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mak, Assigned: mak)

Tracking

({perf})

55 Branch
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(10 attachments)

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
Assignee

Description

a year ago
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

a year ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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]
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

a year 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

a year 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

a year 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

a year 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

a year ago
Posted patch eslint fixSplinter Review

Updated

a year ago
Depends on: 1442939

Updated

a year ago
Depends on: 1442942

Updated

a year ago
Depends on: 1442945
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

a year ago
Please stop asking for things to be backed out for minor breakage.
Flags: needinfo?(mak77)
Assignee

Updated

a year ago
Depends on: 1440644
You need to log in before you can comment on or make changes to this bug.