Closed Bug 1439315 Opened 5 years ago Closed 5 years ago

defineLazyScriptGetter controller.js

Categories

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

Product:
Firefox
Component:
Bookmarks & History
Version:
55 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: perf, Whiteboard: [fxsearch])

Attachments

(10 files)

Bug 1439315 - 1 - rename InsertionPoint to PlacesInsertionPoint.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1439315 - 2 - Move command utils to PlacesUIUtils.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1439315 - 3 - Don't use NetUtil.newURI.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1439315 - 5 - defineLazyScriptGetter controller.js.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1439315 - 6 - Move some utils into PlacesController or PlacesUIUtils.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1439315 - 7 - Don't import PlacesUtils on PlacesUIUtils import.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1439315 - 8 - Remove some dangling NetUtil usage.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1439315 - 4 - Make ESLint happy.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1439315 - 9 - Remove no more necessary placesOverlayModules.
59 bytes, text/x-review-board-request
standard8
: review+
Details
eslint fix
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: nobody → mak77
Status: NEW → ASSIGNED
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 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 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 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 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 on attachment 8954815 [details]
Bug 1439315 - 5 - defineLazyScriptGetter controller.js.

https://reviewboard.mozilla.org/r/223962/#review230200
Attachment #8954815 - Flags: review?(standard8) → 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 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 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 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 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 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 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 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 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 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)
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 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 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 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]
Attached patch eslint fix — — Splinter Review 

    
  
Depends on: 1442939

    
  
Depends on: 1442942

    
  
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)

    
    

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

    
  
Depends on: 1440644

          You need to log in
          before you can comment on or make changes to this bug.