961531 - Remove inline script / style in browser/devtools/shadereditor/shadereditor.xul

Status: RESOLVED FIXED

(DevTools Graveyard :: WebGL Shader Editor, defect)

Description

[Brian Grinstead [:bgrins]]

https://mxr.mozilla.org/mozilla-central/source/browser/devtools/shadereditor/shadereditor.xul

There is an inline event handler on the requests-menu-reload-notice-button element.  This should be moved into shadereditor.js.  Victor, please correct me if you think it should be elsewhere, but I'm thinking it should be added and removed in EventsHandler.initialize and EventsHandler.destroy.

Comment 1

[Malintha Fernando [:marlio]]

Thank you Brian. Can you please assign me in?

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Malintha Fernando from comment #1)
> Thank you Brian. Can you please assign me in?

Sure :).  And if you haven't seen it already, take a look at our guide for getting DevTools built and running: https://wiki.mozilla.org/DevTools/Hacking.  You may also need to enable the shader editor panel in the DevTools options pane if you don't see it as an available tab.

Comment 3

[Shubham Somani]

Attachment: Proposing a patch for Bug 961531

Removed scripts/styles from browser/devtools/shadereditor/shadereditor.xul
Please tell me if any changes are required.
Cheers!

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8375412 [details] [diff] [review]
Proposing a patch for Bug 961531

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

FYI, if you'd like feedback on the patch you can mark feedback ? and enter a user name in the box, or leave the box blank if you don't know who.  In this case you can enter :bgrins.

::: browser/devtools/shadereditor/shadereditor.js
@@ +52,5 @@
>   * The current target and the WebGL Editor front, set by this tool's host.
>   */
>  let gToolbox, gTarget, gFront;
> +document.addEventListener("DOMContentLoaded", function() {
> +	let requests-menu-reload-notice-button = document.getElementById("requests-menu-reload-notice-button");

This is not valid JavaScript.  I'd suggest you open the Shader Editor with the patch applied to test out the changes.

Secondly, rather than putting this in a new DOMContentLoaded handler, the button should have an addEventListener call in the EventsHandler.initialize function, and a removeEventListener call in the EventsHandler.destroy function.

You can use $("#requests-menu-reload-notice-button") as a helper function to get ahold of the button element in both places.

Comment 5

[Malintha Fernando [:marlio]]

Sorry for the delay. I'm working on this.

Comment 6

[Mike Hoye [:mhoye]]

Malintha - Are you still working on this?

Comment 7

[Malintha Fernando [:marlio]]

Mike - Sorry, I got engaged in one of the GSoC projects. So may not be able to look in to this for some time. You can take this if you like. :)

Comment 8

[Atique Ahmed Ziad [:atiqueahmedziad]]

Based on comment 7 unassigned the bug.

Comment 9

[Ali Movahedi [:amovah]]

Hi, I'm interested in to fix this issue. May you assign me to this bug?

Comment 10

[Mike Hoye [:mhoye]]

Hi, Ali - Go for it! Feel free to ask Brian if you have questions, by checking the "Need more information" box below, and picking "mentor". 

Thanks!

Comment 11

[Ali Movahedi [:amovah]]

Attachment: Move inline event handle from shadereditor.xul to shadereditor.js

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8652714 [details] [diff] [review]
Move inline event handle from shadereditor.xul to shadereditor.js

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

Looks great, thanks Ali!  I've included a couple of minor notes, but this looks ready to go after those are updated.  Also, in the commit message, can you replace 'event handle' with 'event handler'?  You can re-upload a patch and flag me for review again after that.

::: browser/devtools/shadereditor/shadereditor.js
@@ +64,5 @@
>      EventsHandler.initialize(),
>      ShadersListView.initialize(),
>      ShadersEditorsView.initialize()
>    ]);
>  }

Please re-add the new line between these two functions

@@ +114,5 @@
> +
> +  /**
> +   * Handle a command event on reload button
> +   */
> +  _onCommand() {

Maybe rename this function to _onReloadCommand

Comment 13

[Ali Movahedi [:amovah]]

Attachment: Move inline event handler form shadereditor.xul to shadereditor.js

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8653342 [details] [diff] [review]
Move inline event handler form shadereditor.xul to shadereditor.js

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

Looks good to me.  Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e701d5364fa

Comment 15

[Brian Grinstead [:bgrins]]

I'll push this to fxteam once the tree is opened again

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/61c07e1ebc74

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/61c07e1ebc74