1434839 - Rename some payments variables and files and add a button to debug the remote frame to ease development

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

* There are a few files/globals which are similar in name and even though there are in separate directories (content vs. res) and having different contexts it still causes confusion.

* Currently one needs to run 4 lines of code from the docs to debug the remove unprivileged frame. Now that we have a debug frame, we should just have a button to open it instead and save a lot of time.

* When developing using a file: URI (docs added), I often want to keep the debugging panel open so I added a ?debug=1 query parameter to force it to open by default. I currently waste a lot of time focusing the window and hitting the keyboard shortcut after every refresh.

* Rename the `PaymentRequest` global in the unprivileged frame to `paymentRequest` so it doesn't shadow the standard window.PaymentRequest from the Payment Request spec and avoid confusing the two. I considered renaming these files and the global but `paymentRequest.cancel()` just makes sense to me.

Comment 1

[Matthew N. [:MattN]]

Attachment: Bug 1434839 - Rename PaymentDialog to paymentDialogWrapper to reduce confusion.

Review commit: https://reviewboard.mozilla.org/r/217100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217100/

Comment 2

[Matthew N. [:MattN]]

Attachment: Bug 1434839 - Rename 'PaymentRequest' variable to camelCase to reduce confusion with the standard API.

Also export paymentRequest so it's accessible to debugging.js in the next commit

Review commit: https://reviewboard.mozilla.org/r/217102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217102/

Comment 3

[Matthew N. [:MattN]]

Attachment: Bug 1434839 - Button to debug the remote payment frame. ?debug=1 to toggle the debug console.

Review commit: https://reviewboard.mozilla.org/r/217104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217104/

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8947371 [details]
Bug 1434839 - Button to debug the remote payment frame. ?debug=1 to toggle the debug console.

https://reviewboard.mozilla.org/r/217104/#review222920


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/payments/content/paymentDialogWrapper.js:223
(Diff revision 1)
> +    if (!Services.prefs.getBoolPref("devtools.chrome.enabled", false)) {
> +      this.log.error("devtools.chrome.enabled must be enabled to debug the frame");
> +      return;
> +    }
> +    let chromeWindow = Services.wm.getMostRecentWindow(null);
> +    let {gDevToolsBrowser} = ChromeUtils.import("resource://devtools/client/framework/gDevTools.jsm",

Error: Line 223 exceeds the maximum line length of 100. [eslint: max-len]

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8947369 [details]
Bug 1434839 - Rename PaymentDialog to paymentDialogWrapper to reduce confusion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217100/diff/1-2/

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8947370 [details]
Bug 1434839 - Rename 'PaymentRequest' variable to camelCase to reduce confusion with the standard API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217102/diff/1-2/

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8947371 [details]
Bug 1434839 - Button to debug the remote payment frame. ?debug=1 to toggle the debug console.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217104/diff/1-2/

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8947369 [details]
Bug 1434839 - Rename PaymentDialog to paymentDialogWrapper to reduce confusion.

https://reviewboard.mozilla.org/r/217100/#review223026

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8947370 [details]
Bug 1434839 - Rename 'PaymentRequest' variable to camelCase to reduce confusion with the standard API.

https://reviewboard.mozilla.org/r/217102/#review223028

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8947371 [details]
Bug 1434839 - Button to debug the remote payment frame. ?debug=1 to toggle the debug console.

https://reviewboard.mozilla.org/r/217104/#review223030

::: toolkit/components/payments/res/debugging.js:196
(Diff revision 2)
> +  if (window.location.protocol == "resource:") {
> +    // Only show the debug frame button if we're running from a resource URI
> +    // so it doesn't show during development over file: or http: since it won't work.
> +    // Note that the button still won't work if resource://payments/paymentRequest.xhtml
> +    // is manually loaded in a tab but will be shown.
> +    document.getElementById("debugFrame").hidden = false;

Where does `#debugFrame` have hidden=true by default? It seems that this line is a no-op right now.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8947371 [details]
Bug 1434839 - Button to debug the remote payment frame. ?debug=1 to toggle the debug console.

https://reviewboard.mozilla.org/r/217104/#review223030

> Where does `#debugFrame` have hidden=true by default? It seems that this line is a no-op right now.

I guess you didn't see it but it's there using standard HTML format since it's not an XHTML document: ```html
<button id="debugFrame" hidden>Debug frame</button>
```

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8947371 [details]
Bug 1434839 - Button to debug the remote payment frame. ?debug=1 to toggle the debug console.

https://reviewboard.mozilla.org/r/217104/#review223120

::: toolkit/components/payments/res/debugging.js:196
(Diff revision 2)
> +  if (window.location.protocol == "resource:") {
> +    // Only show the debug frame button if we're running from a resource URI
> +    // so it doesn't show during development over file: or http: since it won't work.
> +    // Note that the button still won't work if resource://payments/paymentRequest.xhtml
> +    // is manually loaded in a tab but will be shown.
> +    document.getElementById("debugFrame").hidden = false;

Haha, yep I see it now. I actually went back and looked for it, but I guess I should have ctrl+F looked for it.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8947369 [details]
Bug 1434839 - Rename PaymentDialog to paymentDialogWrapper to reduce confusion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217100/diff/2-3/

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8947370 [details]
Bug 1434839 - Rename 'PaymentRequest' variable to camelCase to reduce confusion with the standard API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217102/diff/2-3/

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8947371 [details]
Bug 1434839 - Button to debug the remote payment frame. ?debug=1 to toggle the debug console.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217104/diff/2-3/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c61a9b897dc7
Rename PaymentDialog to paymentDialogWrapper to reduce confusion. r=jaws
https://hg.mozilla.org/integration/autoland/rev/236d6f06ca7f
Rename 'PaymentRequest' variable to camelCase to reduce confusion with the standard API. r=jaws
https://hg.mozilla.org/integration/autoland/rev/a512ed3dcbd3
Button to debug the remote payment frame. ?debug=1 to toggle the debug console. r=jaws

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c61a9b897dc7
https://hg.mozilla.org/mozilla-central/rev/236d6f06ca7f
https://hg.mozilla.org/mozilla-central/rev/a512ed3dcbd3