Closed Bug 960730 Opened 9 years ago Closed 9 years ago

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

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: bgrins, Assigned: k)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 2 obsolete files)

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
Attached patch bug-960730.patch (obsolete) — Splinter Review
moved submit listener to connect.js
Attachment #8361603 - Flags: review?(bgrinstead)
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)
Assignee: nobody → k
Status: NEW → ASSIGNED
Attached patch bug-960730_1.patch (obsolete) — Splinter Review
done
Attachment #8361699 - Flags: review?(bgrinstead)
Attachment #8361603 - Attachment is obsolete: true
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)
Okay.
Never worked with Mercurial before, thank you for the link.
Attached patch bug-960730.patchSplinter Review
now it's one patch with all the changes :)
Attachment #8361699 - Attachment is obsolete: true
Attachment #8362445 - Flags: review?(bgrinstead)
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+
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
Closed: 9 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
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js] → [mentor=bgrins][lang=html][good first bug][lang=js][qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.