Closed Bug 786489 Opened 7 years ago Closed 7 years ago

Change settings through proper API

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(2 files)

There's some code manipulating settings through preferences instead of going through a proper API. This is making some refactoring difficult. Patches coming shortly...
Self-explanatory.
Attachment #656244 - Flags: review?(rnewman)
Self-explanatory
Attachment #656245 - Flags: review?(rnewman)
Comment on attachment 656244 [details] [diff] [review]
Part 1: Proper serverURL and clusterURL setting, v1

Review of attachment 656244 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, fine… but I feel like the original intent was to make sure that the rest of Sync works correctly when it encounters this set of prefs on disk.

That is, imagine that Service -- through some dreadful bug, or we all drink too much and go on a committing spree -- changes behavior to always prepend "foo" when it writes a server URL through its setter, and removes the first three characters when it reads.

After this patch, the test will pass, but the actual functionality will break horribly for clients whose server URL is already live in prefs.

r+, but I'd like you to consider this.

Also, weren't you going to rename these URLs?

::: services/sync/tests/unit/test_syncengine_sync.js
@@ +75,5 @@
>    _("SyncEngine._syncStartup resets sync and wipes server data if there's no or an outdated global record");
>  
>    let syncTesting = new SyncTestingInfrastructure();
> +  Service.serverURL = TEST_SERVER_URL;
> +  Service.clusterURL = TEST_CLUSTER_URL;

Can we lift these three (four) lines into a function, rather than leaving the copypasta in the file?
Attachment #656244 - Flags: review?(rnewman) → review+
Comment on attachment 656245 [details] [diff] [review]
Part 2: Proper username setting, v1

Review of attachment 656245 [details] [diff] [review]:
-----------------------------------------------------------------

%s,r\?,r+,g
Attachment #656245 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #3)
> Yes, fine… but I feel like the original intent was to make sure that the
> rest of Sync works correctly when it encounters this set of prefs on disk.
> 
> That is, imagine that Service -- through some dreadful bug, or we all drink
> too much and go on a committing spree -- changes behavior to always prepend
> "foo" when it writes a server URL through its setter, and removes the first
> three characters when it reads.
> 
> After this patch, the test will pass, but the actual functionality will
> break horribly for clients whose server URL is already live in prefs.
> 
> r+, but I'd like you to consider this.

We have tests to ensure that the Service properties work as advertised. test_service_attributes.js IIRC. You'll notice I didn't touch this file.

Also, if this makes you squeamish, the refactor to have a separate settings/config/state type is really going to make you uneasy. If you have concerns, we really need to talk about those soon.

> Also, weren't you going to rename these URLs?

That patch is sitting in my queue :D
 
> ::: services/sync/tests/unit/test_syncengine_sync.js
> @@ +75,5 @@
> >    _("SyncEngine._syncStartup resets sync and wipes server data if there's no or an outdated global record");
> >  
> >    let syncTesting = new SyncTestingInfrastructure();
> > +  Service.serverURL = TEST_SERVER_URL;
> > +  Service.clusterURL = TEST_CLUSTER_URL;
> 
> Can we lift these three (four) lines into a function, rather than leaving
> the copypasta in the file?

0FG.
(In reply to Gregory Szorc [:gps] from comment #5)

> We have tests to ensure that the Service properties work as advertised.
> test_service_attributes.js IIRC. You'll notice I didn't touch this file.

Good, just wanted to make sure you had all this swapped in and weren't impulsively fixinating things :)

> Also, if this makes you squeamish, the refactor to have a separate
> settings/config/state type is really going to make you uneasy. If you have
> concerns, we really need to talk about those soon.

No, that sounds like a nice introduction of isolation, similar to SyncConfiguration on Android.

> > Also, weren't you going to rename these URLs?
> 
> That patch is sitting in my queue :D

*shakes fist*

> > Can we lift these three (four) lines into a function, rather than leaving
> > the copypasta in the file?
> 
> 0FG.

:D

I hate that file so much.
https://hg.mozilla.org/mozilla-central/rev/475cacc53484
https://hg.mozilla.org/mozilla-central/rev/6bc61754b7f0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla19
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.