Closed Bug 1434839 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: WebPayments UI, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: MattN, Assigned: MattN)

Details

(Whiteboard: [webpayments])

Attachments

(3 files)

* 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.
No longer blocks: 1427953
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 on attachment 8947369 [details]
Bug 1434839 - Rename PaymentDialog to paymentDialogWrapper to reduce confusion.

https://reviewboard.mozilla.org/r/217100/#review223026
Attachment #8947369 - Flags: review?(jaws) → review+
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
Attachment #8947370 - Flags: review+
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.
Attachment #8947369 - Flags: review?(sfoster)
Attachment #8947370 - Flags: review?(sfoster)
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 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.
Attachment #8947371 - Flags: review?(jaws) → review+
Pushed by mozilla@noorenberghe.ca:
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
Priority: P3 → P1
Whiteboard: [webpayments]
Product: Toolkit → Firefox
Target Milestone: mozilla60 → Firefox 60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: