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

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Brian Grinstead [:bgrins]]

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

Comment 1

[k]

Attachment: bug-960730.patch

moved submit listener to connect.js

Comment 2

[Brian Grinstead [:bgrins]]

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.

Comment 3

[k]

Attachment: bug-960730_1.patch

done

Comment 4

[Brian Grinstead [:bgrins]]

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

Comment 5

[k]

Okay.
Never worked with Mercurial before, thank you for the link.

Comment 6

[k]

Attachment: bug-960730.patch

now it's one patch with all the changes :)

Comment 7

[Brian Grinstead [:bgrins]]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/80613bb663db

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80613bb663db