1140481 - Use the StoreMixin in some of the Loop call views

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mark Banner (:standard8)]

Attachment: 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.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 3

[Mark Banner (:standard8)]

Attachment: 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.

Comment 4

[Mark Banner (:standard8)]

Attachment: diff -w version

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 6

[Mark Banner (:standard8)]

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

Comment 7

[Mark Banner (:standard8)]

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

[:shell escalante]

Per Gavin's request - just updating bugs that carried over before they were 100% resolved to the next iteration.

Comment 9

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

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

Comment 10

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

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7324d3da6d28
https://hg.mozilla.org/mozilla-central/rev/f96045350d40