Closed Bug 1418226 Opened 3 years ago Closed 3 years ago

Create a store for payment dialog unprivileged UI state

Categories

(Firefox :: WebPayments UI, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: MattN, Assigned: MattN)

Details

Attachments

(1 file)

Create a store for payment dialog unprivileged UI state that custom elements can subscribe to updates on. The store should batch subscriber updates to reduce partial state updates. See bug 1410321 for some more info on the approach.
Comment on attachment 8929355 [details]
Bug 1418226 - Create a store for payment dialog unprivileged UI state.

https://reviewboard.mozilla.org/r/200680/#review209832

Looks fine but I disagree about the subscribe method triggering a state change callback. r+ if you're willing to remove this side effect.

::: toolkit/components/payments/res/paymentsStore.js:57
(Diff revision 2)
> +    // Let any synchronous setState calls that happen after the current setState call
> +    // complete first.
> +    // Their effects on the state will be batched up before the callback is actually called below.
> +    await Promise.resolve();
> +
> +

nit, remove this extra blank line

::: toolkit/components/payments/res/paymentsStore.js:88
(Diff revision 2)
> +    if (this._subscribers.has(component)) {
> +      return;
> +    }
> +
> +    this._subscribers.add(component);
> +    component.stateChangeCallback(this.getState());

Similar styles of subscribers/listeners don't often trigger a notification/callback upon subscription. 

The caller could subscribe and also call the public getState method. I think this should be removed.

::: toolkit/components/payments/test/unit/test_PaymentsStore.js:32
(Diff revision 2)
> +
> +  ps.setState({
> +    one: "one",
> +  });
> +  let state = ps.getState();
> +  do_check_eq(state.one, "one", "Check .one");

Could add `do_check_eq(Object.keys(state).length, 1, "Should only have 1 prop. set");`

::: toolkit/components/payments/test/unit/test_PaymentsStore.js:39
(Diff revision 2)
> +  ps.setState({
> +    two: 2,
> +  });
> +  state = ps.getState();
> +  do_check_eq(state.one, "one", "Check .one");
> +  do_check_eq(state.two, 2, "Check .two");

Same Object.keys(state).length check here and lower too.
Attachment #8929355 - Flags: review?(jaws) → review+
Thanks. I made the change and autolanded to try.
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/50c894d30de0
Create a store for payment dialog unprivileged UI state. r=jaws
https://hg.mozilla.org/mozilla-central/rev/50c894d30de0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Do you remember which files made you add the broad "resource://payments/" exception at https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/browser/base/content/test/static/browser_all_files_referenced.js#23 ? I tried removing that line locally to see what would happen, and I didn't see any test failure.
Flags: needinfo?(MattN+bmo)
(In reply to Florian Quèze [:florian] from comment #10)
> Do you remember which files made you add the broad "resource://payments/"
> exception at
> https://searchfox.org/mozilla-central/rev/
> 03877052c151a8f062eea177f684a2743cd7b1d5/browser/base/content/test/static/
> browser_all_files_referenced.js#23 ? I tried removing that line locally to
> see what would happen, and I didn't see any test failure.

The problem was that we use relative paths in paymentRequest.xhtml[1] so that we can develop via a file: URI without building. The test wasn't smart enough to resolve the relative paths to resource URIs but I don't remember which one specifically had the issue. Example: 
> <script src="debugging.js"></script>
in debugging.html refers to resource://payments/debugging.js

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/payments/res/paymentRequest.xhtml
Flags: needinfo?(MattN+bmo)
Relative urls like this shouldn't be a problem. Can we remove the exception if it's green on try?
oh ok, well that's what I thought the problem was but I could have been wrong. 

Feel free to remove it.
Product: Toolkit → Firefox
Target Milestone: mozilla59 → Firefox 59
You need to log in before you can comment on or make changes to this bug.