Closed Bug 961165 Opened 6 years ago Closed 6 years ago

Remove inline scripts from devtools/scratchpad/scratchpad.xul

Categories

(DevTools :: Scratchpad, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: bbenvie, Assigned: mgoodwin)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 3 obsolete files)

There's an inline script and a bunch of inline handlers (`oncommand="Scratchpad.doSomething()").
Priority: -- → P3
Also, there's (at least) an instance of setAttribute("oncommand.... which will probably break when CSP covers XUL commands
Assignee: nobody → mgoodwin
Blocks: 978115
Firstly, there was existing use of setAttribute("oncommand",...) - once bug 957980 is resolved, this won't work so I replaced this with an event listener.

Secondly, goUpdateSourceEditorMenuItems was inline in scratchpad.xul. I moved this to scratchpad.js.

Thirdly, there were a number of inline oncommand attributes within scratchpad.xul - I removed these and replaced them with event listeners, added via setupCommands / setupCommand in scratchpad.js.  I left oncommand=";" for each one pending resolution of bug 371900

Finally, I moved the script element for scratchpad.js in scratchpad.xul so they followed the two commandsets containing the above commands (so the elements exist when scratchpad.js attempts to add event listeners to them).
Attachment #8383770 - Flags: feedback?(bbenvie)
Comment on attachment 8383770 [details] [diff] [review]
Remove inline scripts from devtools/scratchpad/scratchpad.xul

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

Aside from comments below, there's two places in scratchpad.xul that look like they have inline js. Both of them are `onpopupshowing="goUpdateSourceEditorMenuItems()"`. I'm guessing you want to change those as well and add them to setupCommands (or wherever it is moved to). If that's the case, then the top level `goUpdateSourceEditorMenuItems` function can be removed in favor of just inlining that function in the setupCommands code.

::: browser/devtools/scratchpad/scratchpad.js
@@ +1171,5 @@
> +        menuitem.addEventListener("command", function(idx) {
> +          return function() {
> +            Scratchpad.openFile(idx);
> +          };
> +        }(i));

To make it more obvious what's being done here, let's do:

  menuitem.addEventListener("command", Scratchpad.openFile.bind(Scratchpad, i));

@@ +2168,5 @@
> +    elem.addEventListener("command", fn);
> +  }
> +}
> +
> +function setupCommands() {

This should probably be moved to it's own `Scratchpad._setupCommands` or something of that nature.

@@ +2171,5 @@
> +
> +function setupCommands() {
> +  setupCommand("cmd_gotoLine", function() {
> +    goDoCommand('cmd_gotoLine');
> +  });

To make this more declarative, let's do something like:

  let commands = {
    "cmd_gotoLine": () => {
      goDoCommand('cmd_gotoLine');
    },
    "sp-cmd-newWindow": () => {
      Scratchpad.openScratchpad();
    },
    /* etc */
  };

  for (let command in commands) {
    /* body of setupCommand in here *
  }

@@ +2231,5 @@
> +    Scratchpad.sidebar.hide();
> +  });
> +}
> +
> +setupCommands();

Invoking setupCommands (or Scratchpad._setupCommands) probably belongs in Scratchpad.onLoad unless there's a specific reason it needs to happen immediately here.

::: browser/devtools/scratchpad/scratchpad.xul
@@ +61,3 @@
>  </commandset>
>  
> +<script type="application/javascript" src="chrome://browser/content/devtools/scratchpad.js"/>

I believe if the command setup is changed to run during Scratchpad.onLoad, then you won't have to move the script down below the commandset (not sure though).
Attachment #8383770 - Flags: feedback?(bbenvie)
Removed the onpopupshowing attributes, replaced them with Scratchpad._setupPopupShowingListeners. goUpdateSourceEditorMenuItems was only called from the two onpopupshowing attrs so I moved the contents of that to inside the body of _setupPopupShowingListeners too.

Fixed Scratchpad.populateRecentFilesMenu to use bind, as advised.

Moved setupCommands to Scratchpad._setupCommandListeners. Used a more declarative style for command setup, as advised.

Moved invocation of Scratchpad._setupCommandListeners to Scratchpad.onLoad.

Moved script element for scratchpad.js back to it's original position.
Attachment #8383770 - Attachment is obsolete: true
Attachment #8386762 - Flags: feedback?(bbenvie)
Comment on attachment 8386762 [details] [diff] [review]
Remove inline scripts from devtools/scratchpad/scratchpad.xul

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

This is good, just some nits.

::: browser/devtools/scratchpad/scratchpad.js
@@ +129,5 @@
>  
>      return obj;
>    },
>  
> +  _setupPopupShowingListeners: function SP_setupPopupShowing() {

Needs a comment quickly describing what this does.

@@ +132,5 @@
>  
> +  _setupPopupShowingListeners: function SP_setupPopupShowing() {
> +    let elementIDs = ['sp-menu_editpopup', 'scratchpad-text-popup'];
> +
> +    for(let elementID of elementIDs) {

nit: space between "for" and "(" (and same for a few places below).

@@ +144,5 @@
> +      }
> +    }
> +  },
> +
> +  _setupCommandListeners: function SP_setupCommands() {

Needs a comment.
Attachment #8386762 - Flags: feedback?(bbenvie) → feedback+
Nits addressed.
Attachment #8386762 - Attachment is obsolete: true
Attachment #8387537 - Flags: review?(bbenvie)
Even more nits addressed...
Attachment #8387537 - Attachment is obsolete: true
Attachment #8387537 - Flags: review?(bbenvie)
Attachment #8387549 - Flags: review?(bbenvie)
Comment on attachment 8387549 [details] [diff] [review]
Remove inline scripts from devtools/scratchpad/scratchpad.xul

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

LGTM!
Attachment #8387549 - Flags: review?(bbenvie) → review+
Comment on attachment 8387549 [details] [diff] [review]
Remove inline scripts from devtools/scratchpad/scratchpad.xul

Thanks benvie
Attachment #8387549 - Flags: checkin?(bbenvie)
Comment on attachment 8387549 [details] [diff] [review]
Remove inline scripts from devtools/scratchpad/scratchpad.xul

In the future, please just use the checkin-needed keyword :)
Attachment #8387549 - Flags: checkin?(bbenvie) → checkin+
https://hg.mozilla.org/mozilla-central/rev/80ea77dd1eac
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.