Rewrite inline script / style in devtools/framework/connect/connect.xhtml

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Developer Tools
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bgrins, Assigned: k)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js][qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Let's move any inline JavaScript, styles, and event handlers out of this file: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/connect/connect.xhtml
(Assignee)

Comment 1

4 years ago
Created attachment 8361603 [details] [diff] [review]
bug-960730.patch

moved submit listener to connect.js
Attachment #8361603 - Flags: review?(bgrinstead)
(Reporter)

Comment 2

4 years ago
Comment on attachment 8361603 [details] [diff] [review]
bug-960730.patch

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

Thanks for the patch!  I've made a couple of notes.  Also, can you add ;r=bgrins at the end of the commit message?

::: browser/devtools/framework/connect/connect.js
@@ +185,5 @@
>      window.close();
>    });
>  }
> +
> +window.onload = function(){

You can move this logic into the existing DOMContentLoaded event instead of adding the onload

@@ +186,5 @@
>    });
>  }
> +
> +window.onload = function(){
> +  let form = window.document.getElementById('connection-form-form');

In this case, I would just use document.querySelector("#connection-form form") and get rid of the ID in the markup, since it is unlikely to move out of this element.
Attachment #8361603 - Flags: review?(bgrinstead)
(Reporter)

Updated

4 years ago
Assignee: nobody → k
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created attachment 8361699 [details] [diff] [review]
bug-960730_1.patch

done
Attachment #8361699 - Flags: review?(bgrinstead)
(Reporter)

Updated

4 years ago
Attachment #8361603 - Attachment is obsolete: true
(Reporter)

Comment 4

4 years ago
Comment on attachment 8361699 [details] [diff] [review]
bug-960730_1.patch

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

This patch looks like it was based off of the original patch you submitted, not against the latest code base.  Can you rebuild it against the latest code?  It may be that you added a new patch on the queue, instead of modifying the existing one - there are instructions on MDN about how to put two patches together: https://developer.mozilla.org/en-US/docs/Mercurial_Queues#Folding_multiple_patches_into_one.  Feel free to ping me (bgrins) on IRC in #devtools if you need some help.

::: browser/devtools/framework/connect/connect.js
@@ +35,5 @@
>  
>    if (port) {
>      document.getElementById("port").value = port;
>    }
> +  

Looks like there is some trailing whitespace here
Attachment #8361699 - Flags: review?(bgrinstead)
(Assignee)

Comment 5

4 years ago
Okay.
Never worked with Mercurial before, thank you for the link.
(Assignee)

Comment 6

4 years ago
Created attachment 8362445 [details] [diff] [review]
bug-960730.patch

now it's one patch with all the changes :)
Attachment #8361699 - Attachment is obsolete: true
Attachment #8362445 - Flags: review?(bgrinstead)
(Reporter)

Comment 7

4 years ago
Comment on attachment 8362445 [details] [diff] [review]
bug-960730.patch

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

Looks good, thanks!  You can mark checkin-needed to get it checked in.
Attachment #8362445 - Flags: review?(bgrinstead) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/80613bb663db
Keywords: checkin-needed
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js] → [mentor=bgrins][lang=html][good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/80613bb663db
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js][fixed-in-fx-team] → [mentor=bgrins][lang=html][good first bug][lang=js]
Target Milestone: --- → Firefox 29

Updated

4 years ago
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js] → [mentor=bgrins][lang=html][good first bug][lang=js][qa-]
You need to log in before you can comment on or make changes to this bug.