1026782 - Implement setLoopCharPref

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Andrei Oprea [mention me on [e-mail]]]

This is analogous to mozLoopAPI.getLoopCharPref. The immediate need is so that we can store when the people have seen the legal links and don't need them to be shown again.

Comment 1

[Maire Reavy [:mreavy]]

Dan -- I know we talked about this bug on Wednesday.  I just wanted to verify that this is a gecko bug.  (I just put gecko in the whiteboard.)  If so, I think jib makes the most sense as the owner.  Can you coordinate with him later today on the solution for this?

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

It is; it needs changes in (mostly) MozLoopService.jsm and MozLoopAPI.jsm.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

It seems like bug 1020876 would make getLoopCharPref() and setLoopCharPref() unused, right? Shouldn't we rather get rid of that then?

Comment 4

[Mark Banner (:standard8)]

(In reply to Tim Taubert [:ttaubert] (away July 7th-18th) from comment #3)
> It seems like bug 1020876 would make getLoopCharPref() and setLoopCharPref()
> unused, right? Shouldn't we rather get rid of that then?

It will do, but the bugs that are dependent on this bug will then reintroduce uses for them.


Jib: I'd like this bug to start introducing specific tests for MozLoopAPI, I think these can probably done with browser chrome mochitests. If you need help, I'm willing to work through some ideas with you.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Wouldn't a better idea be to store that data in an Indexed DB store (bug 1028187) that takes care of prefs instead of having to transfer it back and forth only to introduce more preferences?

Comment 6

[Mark Banner (:standard8)]

(In reply to Tim Taubert [:ttaubert] (away July 7th-18th) from comment #5)
> Wouldn't a better idea be to store that data in an Indexed DB store (bug
> 1028187) that takes care of prefs instead of having to transfer it back and
> forth only to introduce more preferences?

Using indexed db does make more sense. We're slightly concerned about how long it will take to get the origins fixed up properly, so rather than block all these bugs on having this done, we're happy to do this small amount of work to temp add the function for prefs, and then transition to indexed db later (assuming work schedules match up).

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: implements MozLoopService.setLoopCharPref

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Mark Banner (:standard8) from comment #4)
> Jib: I'd like this bug to start introducing specific tests for MozLoopAPI, I
> think these can probably done with browser chrome mochitests. If you need
> help, I'm willing to work through some ideas with you.

In the patch I added xpcshell tests similar to that of getLoopCharPref. Is that sufficient, or are we looking for mochitests to test more of the framework?

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8447202 [details] [diff] [review]
implements MozLoopService.setLoopCharPref

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

This looks reasonable to me, although I can't actually review it (I can do things inside content/).

Re tests, I think as this is unblocking other bugs, just the xpcshell is fine for now. However, I'd really like to get MozLoopAPI tested via mochitest at some stage, so we test its functionality as well as the functionality of the services (some parts of MozLoopAPI are more than just call forwarding).

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8447202 [details] [diff] [review]
implements MozLoopService.setLoopCharPref

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

r=me with nits fixed.

::: browser/components/loop/MozLoopAPI.jsm
@@ +137,5 @@
> +     *
> +     * Any errors thrown by the Mozilla pref API are logged to the console
> +     * and cause false to be returned.
> +     *
> +     * @return true on success, false on failure.

This doesn't return anything.

@@ +143,5 @@
> +    setLoopCharPref: {
> +      enumerable: true,
> +      configurable: true,
> +      writable: true,
> +      value: function(prefName,value) {

Nit: |prefName, value|

::: browser/components/loop/MozLoopService.jsm
@@ +646,5 @@
> +   * @param {String} prefName The name of the pref without the preceding "loop."
> +   * @param {String} value The value to set.
> +   *
> +   * Any errors thrown by the Mozilla pref API are logged to the console
> +   * and cause false to be returned.

This doesn't return anything.

@@ +648,5 @@
> +   *
> +   * Any errors thrown by the Mozilla pref API are logged to the console
> +   * and cause false to be returned.
> +   *
> +   * @return true on success, false on failure.

Same.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: implements MozLoopService.setLoopCharPref (2) r=ttaubert

Incorporated feedback. Carrying forward r=ttaubert.

Comment 12

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5966f6003077

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5966f6003077

Comment 14

[u279076]

I don't think this needs QA attention before we release. Please needinfo me to request specific testing.