Use the StoreMixin in some of the Loop call views

RESOLVED FIXED in Firefox 39

Status

P2
normal
Rank:
23
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla39
Points:
1
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [tech-debt])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
As part of the work for bug 1088672, I want to tidy the call views up a bit first and use the store mixin to simplify the props we're passing and the duplicated code.
(Assignee)

Comment 1

4 years ago
Created attachment 8574035 [details] [diff] [review]
Use the StoreMixin in some of the Loop conversation views.

This uses the StoreMixin in a few more places.

It also fixes a bug in the StoreMixin where it was stopping listening on unmount for all views attached to the store rather than just stopping for the view being unmounted.
Attachment #8574035 - Flags: review?(dmose)
Status: NEW → ASSIGNED
Flags: qe-verify-
Flags: firefox-backlog+
Comment on attachment 8574035 [details] [diff] [review]
Use the StoreMixin in some of the Loop conversation views.

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

r=dmose, with the one added unit test.

::: browser/components/loop/content/shared/js/store.js
@@ +135,5 @@
>            this.setState(this.getStoreState());
>          }, this);
>        },
>        componentWillUnmount: function() {
> +        this.getStore().off("change", null, this);

This would appear to fix a bug, so I'd suggest a unit test here.
Attachment #8574035 - Flags: review?(dmose) → review+
(Assignee)

Comment 3

4 years ago
Created attachment 8575502 [details] [diff] [review]
Use the StoreMixin in some of the Loop conversation views.

I had to write a new test section (and indent the existing one) to be able to test the store mixin, hence re-requesting review.
Attachment #8574035 - Attachment is obsolete: true
Attachment #8575502 - Flags: review?(dmose)
(Assignee)

Comment 4

4 years ago
Created attachment 8575505 [details] [diff] [review]
diff -w version
Comment on attachment 8575505 [details] [diff] [review]
diff -w version

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

Looks good with one minor tweak; thanks!  r=dmose

::: browser/components/loop/test/shared/store_test.js
@@ +163,5 @@
> +      testStore = loop.store.createStore({});
> +
> +      store = new testStore(dispatcher);
> +
> +      loop.store.StoreMixin.register({store: store});

There a lot of similarly named things here (e.g. it's not obvious on first reading how testStore and store are different).   I think part of the problem is the API itself hard-to-approach, but no need to fix that here.  If you could at least rename things here a bit more explicitly i think that would help test readability notably.
Attachment #8575505 - Flags: review+

Updated

4 years ago
Attachment #8575502 - Flags: review?(dmose) → review+
(Assignee)

Comment 6

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/35827fc86c8
backlog: --- → tech-debt
Whiteboard: [tech-debt]
Target Milestone: --- → mozilla39
(Assignee)

Comment 7

4 years ago
I'd missed the ui-showcase, here's the bustage fix with rs=dmose over irc:

https://hg.mozilla.org/integration/fx-team/rev/c4d7c9f94f61

Comment 8

4 years ago
Per Gavin's request - just updating bugs that carried over before they were 100% resolved to the next iteration.
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar

Updated

4 years ago
Rank: 23
Priority: -- → P2
Backed both parts out under suspicion of making the web-platform-tests-reftests intermittently fail:

https://hg.mozilla.org/integration/fx-team/rev/1cf3f2f38f4e
https://hg.mozilla.org/integration/fx-team/rev/0b2ed04e67ca

https://treeherder.mozilla.org/logviewer.html#?job_id=2235777&repo=fx-team
Flags: needinfo?(standard8)
There were Wr failures after those backouts (and more retriggers prior to when these patches first landed showed some Wr failures), so relanding these:
https://hg.mozilla.org/integration/fx-team/rev/7324d3da6d28
https://hg.mozilla.org/integration/fx-team/rev/f96045350d40
Flags: needinfo?(standard8)
https://hg.mozilla.org/mozilla-central/rev/7324d3da6d28
https://hg.mozilla.org/mozilla-central/rev/f96045350d40
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.