Closed Bug 1392148 Opened 7 years ago Closed 7 years ago

Remove aliases to CommonUtils in util.js

Categories

(Firefox :: Sync, enhancement)

Product:
Firefox
Component:
Sync
Version:
57 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: nicolaso, Assigned: nicolaso)

Details

Attachments

(1 file)

Bug 1392148 - Remove aliases to CommonUtils in util.js
59 bytes, text/x-review-board-request
eoger
: review+
markh
: review+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170818100226



Actual results:

From services/sync/modules/util.js:

> // Alias in functions from CommonUtils. These previously were defined here.
> // In the ideal world, references to these would be removed.
http://searchfox.org/mozilla-central/source/services/sync/modules/util.js#47

We could fix this by removing the aliases and calling CommonUtils directly. Or, we could remove the last part of that comment if we find that it's not an issue after all.
Severity: normal → enhancement
Component: Untriaged → Sync
I'm inclined to say we should reference CommonUtils directly as it makes things much clearer, but I guess I'd need to see the patch to know for sure :) Ed has done some work around fixing some of this namespace confusion so he might have thoughts.
Sounds about right, we don't really add anything by doing that redirection.
By the way, my goal is to remove this |Svc| object at the end of this file (bug 1375233), if you want to give a hand be my guest :)
Attachment #8901037 - Flags: review?(markh)
Attachment #8901037 - Flags: review?(eoger)
Comment on attachment 8901037 [details]
Bug 1392148 - Remove aliases to CommonUtils in util.js

https://reviewboard.mozilla.org/r/172504/#review178032

Thanks for the patch, LGTM. I felt a comment bellow.

::: services/sync/modules/browserid_identity.js:220
(Diff revision 2)
>          Weave.Status.login = LOGIN_SUCCEEDED;
>          if (isInitialSync) {
>            this._log.info("Doing initial sync actions");
>            Svc.Prefs.set("firstSync", "resetClient");
>            Services.obs.notifyObservers(null, "weave:service:setup-complete");
> -          Weave.Utils.nextTick(Weave.Service.sync, Weave.Service);
> +          Weave.CommonUtils.nextTick(Weave.Service.sync, Weave.Service);

Do we need the Weave. prefix here?
Attachment #8901037 - Flags: review?(eoger)
Comment on attachment 8901037 [details]
Bug 1392148 - Remove aliases to CommonUtils in util.js

https://reviewboard.mozilla.org/r/172504/#review178032

> Do we need the Weave. prefix here?

Doesn't look like we need it. See latest revision.
Comment on attachment 8901037 [details]
Bug 1392148 - Remove aliases to CommonUtils in util.js

https://reviewboard.mozilla.org/r/172504/#review178466

This looks great to me, thanks! Please adjust the order of some of the imports and I think we'll be good to go!

::: services/sync/modules/engines/history.js:15
(Diff revision 3)
>  var Cr = Components.results;
>  
>  const HISTORY_TTL = 5184000; // 60 days in milliseconds
>  const THIRTY_DAYS_IN_MS = 2592000000; // 30 days in milliseconds
>  
> +Cu.import("resource://services-common/utils.js");

I think we should move this line down, so gre/modules come first, then services-common grouped together followed by services-sync

::: services/sync/modules/policies.js:12
(Diff revision 3)
>    "SyncScheduler",
>  ];
>  
>  var {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>  
> +Cu.import("resource://services-common/utils.js");

ditto here (and most other imports below - we haven't beein consistent in the past with this, but we shouldn't make it worse)

::: services/sync/tests/unit/test_hmac_error.js:4
(Diff revision 3)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +Cu.import("resource://services-common/utils.js");

I think you can skip this import entirely as resource://testing-common/services/sync/utils.js looks like it already imports the module.
Attachment #8901037 - Flags: review?(markh)
Comment on attachment 8901037 [details]
Bug 1392148 - Remove aliases to CommonUtils in util.js

https://reviewboard.mozilla.org/r/172504/#review178802

::: services/sync/tests/unit/test_hmac_error.js:4
(Diff revision 3)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +Cu.import("resource://services-common/utils.js");

I'll admit I didn't do anything to avoid redundant imports. There may be other cases where we could remove the import, and the code would still be correct. 

Should we try to fix redundant imports everywhere in this patch? Many files now import both of:
resource://services-common/utils.js
resource://services-sync/util.js

I also didn't remove imports that have become useless.

Both of those problems should be fairly easy to fix if we want to.

Anyways, fixed this occurrence for now.
(In reply to Nicolas Ouellet-Payeur [:nicolaso] from comment #9)
> > +Cu.import("resource://services-common/utils.js");
> 
> I'll admit I didn't do anything to avoid redundant imports. There may be
> other cases where we could remove the import, and the code would still be
> correct. 

huh - actually, I think I'm totally wrong about not needing that import :( There are some "head*.js" files used by tests, and any imports made by those files are put into the global scope. However, in this case, the test does a normal import of "resource://testing-common/services/sync/utils.js" which *does not* do anything magic to the test's global scope. So I believe I was wrong about that comment and your patch was correct. Sorry about that (and note that it should be trivial to verify - just running the tests will either fail early or work)
 
> Should we try to fix redundant imports everywhere in this patch? Many files
> now import both of:
> resource://services-common/utils.js
> resource://services-sync/util.js

I think that both imports will be necessary in all cases (but as above, running the tests will tell you for sure)

> I also didn't remove imports that have become useless.
> 
> Both of those problems should be fairly easy to fix if we want to.

If there are cases where that's true, I'm happy to have them fixed in this patch - but also happy to *not* have them fixed here.

So I think the only issues that remain is the purely cosmetic one about ordering the imports, so that the gre/modules ones come first, and the "services" ones tend to be grouped sanely.

Thanks, and sorry for the misleading info!
(In reply to Mark Hammond [:markh] from comment #10)
Whoops, forgot to hit the "publish" button.

> huh - actually, I think I'm totally wrong about not needing that import :(
> There are some "head*.js" files used by tests, and any imports made by those
> files are put into the global scope. However, in this case, the test does a
> normal import of "resource://testing-common/services/sync/utils.js" which
> *does not* do anything magic to the test's global scope. So I believe I was
> wrong about that comment and your patch was correct. Sorry about that (and
> note that it should be trivial to verify - just running the tests will
> either fail early or work)

test_hmac_error.js still passes if I remove the include.

> So I think the only issues that remain is the purely cosmetic one about
> ordering the imports, so that the gre/modules ones come first, and the
> "services" ones tend to be grouped sanely.

Please take a look at the latest patch. ☺️
Assignee: nobody → nicolaso
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8901037 [details]
Bug 1392148 - Remove aliases to CommonUtils in util.js

https://reviewboard.mozilla.org/r/172504/#review178840

Looks great, thanks!

::: services/sync/tests/unit/head_helpers.js:14
(Diff revision 4)
>  // This file expects Service to be defined in the global scope when EHTestsCommon
>  // is used (from service.js).
>  /* global Service */
>  
>  Cu.import("resource://services-common/async.js");
> +Cu.import("resource://services-common/utils.js");

ah, these imports are ones that end up in each test's scope, so in theory, every single import listed here is redundant if it also appears in tests. But that problem already existed, so I wont ask you to fix it here. A followup wouldn't hurt, but TBH I don't really care that much.
Attachment #8901037 - Flags: review?(markh) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/3e4b06fa48cd
Remove aliases to CommonUtils in util.js r=markh
https://hg.mozilla.org/mozilla-central/rev/3e4b06fa48cd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment on attachment 8901037 [details]
Bug 1392148 - Remove aliases to CommonUtils in util.js

https://reviewboard.mozilla.org/r/172504/#review179202
Attachment #8901037 - Flags: review?(eoger) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: