Closed
Bug 961165
Opened 10 years ago
Closed 10 years ago
Remove inline scripts from devtools/scratchpad/scratchpad.xul
Categories
(DevTools Graveyard :: Scratchpad, defect, P3)
DevTools Graveyard
Scratchpad
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 30
People
(Reporter: bbenvie, Assigned: mgoodwin)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
9.71 KB,
patch
|
bbenvie
:
review+
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
There's an inline script and a bunch of inline handlers (`oncommand="Scratchpad.doSomething()").
Reporter | ||
Updated•10 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•10 years ago
|
||
Also, there's (at least) an instance of setAttribute("oncommand.... which will probably break when CSP covers XUL commands
Assignee: nobody → mgoodwin
Assignee | ||
Comment 2•10 years ago
|
||
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)
Reporter | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Reporter | ||
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
Nits addressed.
Attachment #8386762 -
Attachment is obsolete: true
Attachment #8387537 -
Flags: review?(bbenvie)
Assignee | ||
Comment 7•10 years ago
|
||
Even more nits addressed...
Attachment #8387537 -
Attachment is obsolete: true
Attachment #8387537 -
Flags: review?(bbenvie)
Attachment #8387549 -
Flags: review?(bbenvie)
Reporter | ||
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8387549 [details] [diff] [review] Remove inline scripts from devtools/scratchpad/scratchpad.xul Thanks benvie
Attachment #8387549 -
Flags: checkin?(bbenvie)
Reporter | ||
Comment 10•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=79bc79734898
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/80ea77dd1eac
Whiteboard: [fixed-in-fx-team]
Comment 12•10 years ago
|
||
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+
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/80ea77dd1eac
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Updated•10 years ago
|
QA Whiteboard: [qa-]
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•