Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aoprea, Assigned: jib)

Tracking

unspecified
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=1 [gecko])

User Story

- Implement setLoopCharPref
- Implement mochitests for get/setLoopCharPref on MozLoopAPI

Attachments

(1 attachment, 1 obsolete attachment)

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.
Blocks: 1023934
No longer blocks: 1023933
Whiteboard: p=1
Target Milestone: --- → 33 Sprint 1- 6/23
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?
Assignee: nobody → jib
Flags: needinfo?(dmose)
Whiteboard: p=1 → p=1 [gecko]
It is; it needs changes in (mostly) MozLoopService.jsm and MozLoopAPI.jsm.
Flags: needinfo?(dmose)
Target Milestone: 33 Sprint 1- 6/23 → 33 Sprint 2- 7/7
It seems like bug 1020876 would make getLoopCharPref() and setLoopCharPref() unused, right? Shouldn't we rather get rid of that then?
Flags: needinfo?(jib)
(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.
User Story: (updated)
Flags: needinfo?(jib)
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?
(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).
Attachment #8447202 - Flags: review?(standard8)
(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?
Flags: needinfo?(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).
Attachment #8447202 - Flags: review?(ttaubert)
Attachment #8447202 - Flags: review?(standard8)
Attachment #8447202 - Flags: feedback+
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.
Attachment #8447202 - Flags: review?(ttaubert) → review+
Flags: needinfo?(standard8)
Incorporated feedback. Carrying forward r=ttaubert.
Attachment #8447202 - Attachment is obsolete: true
Attachment #8448330 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5966f6003077
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: 33 Sprint 2- 7/7 → mozilla33
https://hg.mozilla.org/mozilla-central/rev/5966f6003077
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
I don't think this needs QA attention before we release. Please needinfo me to request specific testing.
Whiteboard: p=1 [gecko] → p=1 [gecko][qa-]
Flags: qe-verify-
QA Contact: anthony.s.hughes
Whiteboard: p=1 [gecko][qa-] → p=1 [gecko]
You need to log in before you can comment on or make changes to this bug.