msucan recently pointed out to me that the debugger tests are horror. I sincerely believe that's a gross understatement. Everything is WET (write-everything-twice), there are a million executeSoon's and private methods and properties are accessed everywhere. Let's make them pretty and a joy to write!

More patches will follow tomorrow, after I'm sure that everything looks good on try and I haven't broken anything.

Comment on attachment 789121 [details] [diff] [review] Part 8: Rewrite head.js use promises and remove useless cruft Review of attachment 789121 [details] [diff] [review]: ----------------------------------------------------------------- This is a very welcome change. ::: browser/devtools/debugger/test/head.js @@ +18,5 @@ > +let { DebuggerPanel } = Cu.import("resource:///modules/devtools/DebuggerPanel.jsm", {}); > +let { BrowserDebuggerProcess } = Cu.import("resource:///modules/devtools/DebuggerProcess.jsm", {}); > +let { DebuggerServer } = Cu.import("resource://gre/modules/devtools/dbg-server.jsm", {}); > +let { DebuggerClient } = Cu.import("resource://gre/modules/devtools/dbg-client.jsm", {}); > +let { SourceEditor } = Cu.import("resource:///modules/source-editor.jsm", {}); oh yeah, that's nice @@ +90,3 @@ > > + aClient.listTabs(aResponse => { > + let tabActor = aResponse.tabs.filter(aGrip => aGrip.url == aUrl).pop(); Nit: Should really be "form" not "grip", but we might be using this incorrectly on the server side as well. But honestly, if I were to write it, I'd probably use "t" for "tab" here since this is such a small, one-off filter. Keep it, change it, do whatever you feel.

Comment on attachment 789110 [details] [diff] [review] Part 1: Convert the debugger frontend to use the EventEmitter instead of relying on DOM events Review of attachment 789110 [details] [diff] [review]: ----------------------------------------------------------------- I don't really see any significant gains from using the EventEmitter API vs. the DOM Event API, but I don't see any downsides either. Did you just prefer the terseness of "on"/"off" and the availability of "once"? ::: browser/devtools/debugger/debugger-controller.js @@ +17,5 @@ > +// The panel's window global is an EventEmitter firing the following events: > +const EVENTS = { > + // When the debugger's source editor instance finishes loading or unloading. > + EDITOR_LOADED: "Debugger:EditorLoaded", > + EDITOR_UNLOADED: "Debugger:EditorUnoaded", "Unoaded"? @@ +813,5 @@ > scope.expand(); > } > } while ((environment = environment.parent)); > > + // Signal that scope environments have been shown and commit the current Typo: mention either "scopes" or "environments". @@ +816,5 @@ > > + // Signal that scope environments have been shown and commit the current > + // variables view hierarchy to briefly flash items that changed between the > + // previous and current scope/variables/properties. > + window.emit(EVENTS.FETCHED_SCOPES); Previously we used to trigger a FETCHED_VARIABLES event, not a FETHCED_SCOPES one. Typo?

Comment on attachment 789117 [details] [diff] [review] Part 5: Add getScopeAtIndex() to make scopes more easily accessible from tests, and fix a few inconsistencies in VariablesView.jsm Review of attachment 789117 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/widgets/VariablesView.jsm @@ +1087,5 @@ > // each of these directly onto any scope, variable or property instance. > this.eval = aView.eval; > this.switch = aView.switch; > this.delete = aView.delete; > + this.preventDisableOnChage = aView.preventDisableOnChage; Maybe take this opportunity to fix the typo in the name?

(In reply to Panos Astithas [:past] from comment #12) once + easier to pass params around. > > @@ +816,5 @@ > > > > + // Signal that scope environments have been shown and commit the current > > + // variables view hierarchy to briefly flash items that changed between the > > + // previous and current scope/variables/properties. > > + window.emit(EVENTS.FETCHED_SCOPES); > > Previously we used to trigger a FETCHED_VARIABLES event, not a > FETHCED_SCOPES one. Typo? I think there should be a clear delimitation between fetching all environments and fetching variables in a scope. This is a win for tests and avoids confusion, since the local scope has all the variables already available.

Comment on attachment 789121 [details] [diff] [review] Part 8: Rewrite head.js use promises and remove useless cruft Review of attachment 789121 [details] [diff] [review]: ----------------------------------------------------------------- Oh yes, this is the part that is truly God's work! ::: browser/devtools/debugger/test/head.js @@ +220,5 @@ > + panelWin.on(aEventName, function onEvent(aEventName, ...aArgs) { > + info("Debugger event '" + aEventName + "' fired: " + (++count) + " time(s)."); > + > + if (count == aEventRepeat) { > + ok(true, "Enough '" + aEventName + "' debugger events have been fired."); In these functions shouldn't we mention the exact number of events for debugging purposes? @@ +285,5 @@ > + > +function navigateActiveTabTo(aPanel, aUrl, aWaitForEventName, aEventRepeat) { > + try { > + return waitForDebuggerEvents(aPanel, aWaitForEventName, aEventRepeat); > + } finally { I find this try/finally clause in the navigate* and reload* functions rather confusing, since we don't really expect exceptions here AFAICT, we are just using it as a way to sequence the block in the "finally" clause after the block in the "try" clause. In that case, why not just do the usual: return waitForFoo().then(formerFinallyBlock); This will also avoid returning a promise of the event arguments that were never requested by the caller (and is an implementation detail). @@ +383,5 @@ > + return; > + } > + if (aDebugger instanceof BrowserDebuggerProcess) { > + // Nothing to do here yet. > + return; "else if" if you really want to keep this, or we could add it when we have something useful to do here.

Comment on attachment 789119 [details] [diff] [review] Part 7: Normalize formatting for non-test files and rename them to follow a nicer pattern Review of attachment 789119 [details] [diff] [review]: ----------------------------------------------------------------- This is missing the changes to Makefile.in (minor, I trust you to get them right) but also the changes to the test files (.js) that go along with the changes to the doc files (.html). Since you seem to be changing test code willy nilly, can I get an updated patch to help make sure we don't miss anything important? You also seem to be moving all script code in HTML files from the head to the document body. I can see how this may be useful against oranges, but I'm afraid it might leave us without any tests that the debugger works in those cases. Let's make sure at least some tests exercise that part.

(In reply to Panos Astithas [:past] from comment #16) Oh, you really haven't even seen half of the patches. :)

(In reply to Panos Astithas [:past] from comment #15)ugger events have been fired."); > > In these functions shouldn't we mention the exact number of events for > debugging purposes? > They are mentioned, before in the info call just above the conditional. Or am I misreading your suggestion? > @@ +285,5 @@ > > + > > +function navigateActiveTabTo(aPanel, aUrl, aWaitForEventName, aEventRepeat) { > > + try { > > + return waitForDebuggerEvents(aPanel, aWaitForEventName, aEventRepeat); > > + } finally { > > I find this try/finally clause in the navigate* and reload* functions rather > confusing, since we don't really expect exceptions here AFAICT, we are just > using it as a way to sequence the block in the "finally" clause after the > block in the "try" clause. In that case, why not just do the usual: > You're right, I'm using the try/finally blocks to structure flow. "Return this promise and then immediately do something". > return waitForFoo().then(formerFinallyBlock); > > This will also avoid returning a promise of the event arguments that were > never requested by the caller (and is an implementation detail). > But that would cause the promise returned by waitForFoo() to never get fulfilled, since its resolution depends strictly on the execution of the formerFinallyBlock. What would need to be done in order to not use the try/finally block would be something like this: let deferred = promise.defer(); waitForFoo().then(deferred.resolve); formerFinallyBlock(); return deferred.promise; Am I missing something?

(In reply to Victor Porof [:vp] from comment #18) > > let deferred = promise.defer(); > waitForFoo().then(deferred.resolve); > formerFinallyBlock(); > return deferred.promise; > ...or: executeSoon(formerFinallyBlock); return waitForFoo();

(In reply to Victor Porof [:vp] from comment #18) > What would need to be done in order to not use the try/finally block would > be something like this: > > let deferred = promise.defer(); > waitForFoo().then(deferred.resolve); > formerFinallyBlock(); > return deferred.promise; > > Am I missing something? Maybe something like: > let promise = waitForFoo(); > ... do stuff ... > return promise;

(In reply to Brandon Benvie [:bbenvie] from comment #20) > Maybe something like: > > > let promise = waitForFoo(); > > ... do stuff ... > > return promise; Yeah, that seems sensible and equivalent.

Addressed comments, removed try/finally blocks to use something less gross.

If it's not too much trouble, it would be awesome if we could get patches that apply to Aurora and Beta too, keeping in mind that Fx24 is going to be an ESR release that we'll be starring for the next year+ :).

There were 3 tests previously disabled, 2 of which on Linux and 1 on OS X. I left them disabled for now (although they *do* seem to pass on try), and will attempt to re-enable them in separate bugs.

Comment on attachment 791353 [details] [diff] [review] Update Makefile Review of attachment 791353 [details] [diff] [review]: ----------------------------------------------------------------- The only check I would suggest for this change is to compare the results from a try run with all of these patches to an fx-team test run and make sure the total number of tests match.

Comment on attachment 791340 [details] [diff] [review] Stackframes tests Review of attachment 791340 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/test/browser_dbg_stack-05.js @@ +132,5 @@ > + "Should have no frames after resume."); > + ok(isCaretPos(gPanel, 5), > + "Editor caret location is correct after resume."); > + is(gEditor.getDebugLocation(), -1, > + "Editor debugg location is correct after resume."); s/debugg/debug/

Comment on attachment 791351 [details] [diff] [review] Add a few more debugger tests Review of attachment 791351 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/test/browser_dbg_reload-preferred-script-03.js @@ +26,5 @@ > + .then(() => switchToSource(SECOND_URL)) > + .then(() => testSource(SECOND_URL)) > + .then(() => switchToSource(FIRST_URL)) > + .then(() => testSource(FIRST_URL)) > + .then(() => closeDebuggerAndFinish(gPanel)); Let's be paranoid and add a then(null, onError) here.

Comment on attachment 791317 [details] [diff] [review] Blackboxing tests Review of attachment 791317 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/test/browser_dbg_blackboxing-02.js @@ +43,5 @@ > + let promise = waitForSourceAndCaretAndScopes(gPanel, ".html", 21).then(() => { > + is(gFrames.itemCount, 3, > + "Should only get 3 frames."); > + is(gDebugger.document.querySelectorAll(".dbg-stackframe-black-boxed").length, 1, > + "And one of them should be the combined black boxed frames."); frames -> frame

Comment on attachment 793429 [details] [diff] [review] Use the debugger's event and promise APIs in webconsole and profiler Review of attachment 793429 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r+! ::: browser/devtools/webconsole/hudservice.js @@ +473,3 @@ > */ > viewSourceInDebugger: > function WC_viewSourceInDebugger(aSourceURL, aSourceLine) Thanks for the cleanup here. Much nicer. @@ +483,5 @@ > + let showSource = ({ DebuggerView }) => { > + if (DebuggerView.Sources.containsValue(aSourceURL)) { > + DebuggerView.setEditorLocation(aSourceURL, aSourceLine); > + return; > + } nit: instead of |return|here why not |} else { selectTool() then viewSource() }| ? @@ +495,5 @@ > + let debuggerAlreadyOpen = toolbox.getPanel("jsdebugger"); > + toolbox.selectTool("jsdebugger").then(({ panelWin: dbg }) => { > + if (debuggerAlreadyOpen) { > + showSource(dbg); > + } else { nit: }\nelse {

Comment on attachment 793429 [details] [diff] [review] Use the debugger's event and promise APIs in webconsole and profiler Review of attachment 793429 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

Comment on attachment 791316 [details] [diff] [review] Sourcemaps tests Nick is more familiar with these tests.

Comment on attachment 791316 [details] [diff] [review] Sourcemaps tests Review of attachment 791316 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

Comment on attachment 791318 [details] [diff] [review] Breakpoints tests Review of attachment 791318 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/test/browser_dbg_breakpoints-actual-location.js @@ +55,5 @@ > + is(aBreakpointClient.requestedLocation.line, 4, > + "Requested location line is correct"); > + > + is(aBreakpointClient.location.url, gSources.selectedValue, "URL is the same"); > + is(aBreakpointClient.location.line, 6, "Line number is new"); You are making these two checks twice, here and above. Copy & paste mix-up? ::: browser/devtools/debugger/test/browser_dbg_breakpoints-contextmenu.js @@ +25,5 @@ > + .then(() => performTestWhilePaused()) > + .then(() => resumeDebuggerThenCloseAndFinish(gPanel)) > + .then(null, aError => { > + ok(false, "Got an error: " + aError.message + "\n" + aError.stack); > + }); Minor simplification, but in these builder-pattern invocations you could do .then(foo) when you don't care about passing a specific parameter to foo. Your call of course. Also, I know how tiresome this patch has been, so I am not going to mention Task.spawn here. ::: browser/devtools/debugger/test/browser_dbg_breakpoints-new-script.js @@ +30,4 @@ > > + gPanel.addBreakpoint({ url: TAB_URL, line: 20 }).then(() => { > + testResume(); > + }); .then(testResume);

So this is way harder than it could be, so let me describe the problem now, before I dive in again tomorrow to finish the review. The conventional wisdom for good patch management says that we never make both functional and style changes in the same patch. That is because the less important style changes confuse the reader trying to detect the functional changes and build a mental model about the modified architecture. This is not a rule I am really attached to, because the occasional style nit doesn't cost me much in review overhead, so I've been (and still am) breaking it occasionally. The problem with this bug though is that a lot of style changes are combined with a lot of modified APIs due to the events -> promises change. The style changes include splitting long lines to shorter ones, combining short lines to a single one, renaming arguments form "foo" to "aFoo" (the old Firefox style), switching between equivalent APIs (e.g. Array.forEach -> for..of), substituting one variable for an equivalent one (e.g. gDebuggee.window -> gDebuggee), and many more. Granted, all of these changes considered on their own merit are arguably right, and the resulting files are undoubtedly simpler to understand, but the problem is that they affect a huge number of files. Reviewing each one file requires to keep in mind the change matrix at all times, but unfortunately the human mind can only keep 7 items in its memory cache, and cache misses require consulting other patches, which in turn leads to week-long review cycles :-) So I'm going to do my best in this case, but what I would find really helpful next time is to either avoid doing style changes along with sweeping functional ones, or to keep them in separate patches so that reviewing them can be done independently. For short patches, or huge patches of one kind that only contain a handful of changes of the other kind, I wouldn't really mind if you chose to combine them. Thanks!

Comment on attachment 791339 [details] [diff] [review] Searching tests Review of attachment 791339 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/test/browser_dbg_search-global-01.js @@ +24,5 @@ > > + waitForSourceAndCaretAndScopes(gPanel, "-02.js", 6) > + .then(() => firstSearch()) > + .then(() => secondSearch()) > + .then(() => clearSearch()) If you care: .then(foo) .then(bar) (here and elsewhere)

Comment on attachment 791344 [details] [diff] [review] Location changes, sources switching and sources cache tests Review of attachment 791344 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Comment on attachment 791348 [details] [diff] [review] Remaining debugger tests Review of attachment 791348 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/test/browser_dbg_debugger-statement.js @@ +50,4 @@ > > // Now attach and resume... > + gClient.request({ to: aResponse.threadActor, type: "attach" }, () => { > + gClient.request({ to: aResponse.threadActor, type: "resume" }, () => { You could modernize this test if you wanted with gClient.attachThread & gClient.resume. @@ +62,5 @@ > +function testDebuggerStatement([aGrip, aResponse]) { > + let deferred = promise.defer(); > + > + gClient.addListener("paused", (aEvent, aPacket) => { > + gClient.request({ to: aResponse.threadActor, type: "resume" }, () => { Same here. ::: browser/devtools/debugger/test/browser_dbg_propertyview-filter-04.js @@ -2,5 @@ > -/* Any copyright is dedicated to the Public Domain. > - http://creativecommons.org/publicdomain/zero/1.0/ */ > - > -/** > - * Make sure that the property view filter prefs work properly. Why was this removed? ::: browser/devtools/debugger/test/browser_dbg_propertyview-filter-07.js @@ -2,5 @@ > -/* Any copyright is dedicated to the Public Domain. > - http://creativecommons.org/publicdomain/zero/1.0/ */ > - > -/** > - * Make sure that the property view correctly filters nodes. What about this one?

Comment on attachment 791341 [details] [diff] [review] VariablesView tests Review of attachment 791341 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Phew, I'm done here!

\o/

(In reply to Panos Astithas [:past] from comment #47) > > Why was this removed? > Those two were redundant checks that didn't exercrise any different/relevant/interesting behavior. I carefully looked into the other tests to make sure. (In reply to Panos Astithas [:past] from comment #45) > If you care: > > .then(foo) > .then(bar) > > (here and elsewhere) Yup, I agree this looks better. It'd look even better with some yields inside a Task, but oh man, I can't even begin to express how much don't care at this point :)

(In reply to Panos Astithas [:past] from comment #43) > Comment on attachment 791318 [details] [diff] [review] > Breakpoints tests > > You are making these two checks twice, here and above. Copy & paste mix-up? > Not really the same checks. The first couple verifies client.requestedLocation, while the second one verifies client.location.

(In reply to Victor Porof [:vp] from comment #52) > (In reply to Panos Astithas [:past] from comment #43) > > You are making these two checks twice, here and above. Copy & paste mix-up? > > > > Not really the same checks. The first couple verifies > client.requestedLocation, while the second one verifies client.location. I'm actually referring to lines 48-51, not 53-56. They look like identical checks to the ones in lines 58-59, albeit with different messages.

(In reply to Panos Astithas [:past] from comment #53) You're right. Fixed.

https://hg.mozilla.org/integration/fx-team/rev/12928639d2bf https://hg.mozilla.org/integration/fx-team/rev/b4f56e9c5f96 https://hg.mozilla.org/integration/fx-team/rev/4c1d5a059c6f https://hg.mozilla.org/integration/fx-team/rev/9cc32e5a67ea https://hg.mozilla.org/integration/fx-team/rev/ecdd787c9eff https://hg.mozilla.org/integration/fx-team/rev/1757ae16bf8a https://hg.mozilla.org/integration/fx-team/rev/0cb94386602a https://hg.mozilla.org/integration/fx-team/rev/04c318f79c84 https://hg.mozilla.org/integration/fx-team/rev/1f66a755177f https://hg.mozilla.org/integration/fx-team/rev/c55396293b49 https://hg.mozilla.org/integration/fx-team/rev/647b5d5dc081 https://hg.mozilla.org/integration/fx-team/rev/708bac7a4c99 https://hg.mozilla.org/integration/fx-team/rev/0d004fdd1fdb https://hg.mozilla.org/integration/fx-team/rev/e721f3ee92bf https://hg.mozilla.org/integration/fx-team/rev/4d259d8db441 https://hg.mozilla.org/integration/fx-team/rev/4559ea542aa9 https://hg.mozilla.org/integration/fx-team/rev/77b93e70c30a https://hg.mozilla.org/integration/fx-team/rev/39471a717494 https://hg.mozilla.org/integration/fx-team/rev/5f626c9dd066 https://hg.mozilla.org/integration/fx-team/rev/862a066d90d5

https://hg.mozilla.org/mozilla-central/rev/12928639d2bf https://hg.mozilla.org/mozilla-central/rev/b4f56e9c5f96 https://hg.mozilla.org/mozilla-central/rev/4c1d5a059c6f https://hg.mozilla.org/mozilla-central/rev/9cc32e5a67ea https://hg.mozilla.org/mozilla-central/rev/ecdd787c9eff https://hg.mozilla.org/mozilla-central/rev/1757ae16bf8a https://hg.mozilla.org/mozilla-central/rev/0cb94386602a https://hg.mozilla.org/mozilla-central/rev/04c318f79c84 https://hg.mozilla.org/mozilla-central/rev/1f66a755177f https://hg.mozilla.org/mozilla-central/rev/c55396293b49 https://hg.mozilla.org/mozilla-central/rev/647b5d5dc081 https://hg.mozilla.org/mozilla-central/rev/708bac7a4c99 https://hg.mozilla.org/mozilla-central/rev/0d004fdd1fdb https://hg.mozilla.org/mozilla-central/rev/e721f3ee92bf https://hg.mozilla.org/mozilla-central/rev/4d259d8db441 https://hg.mozilla.org/mozilla-central/rev/4559ea542aa9 https://hg.mozilla.org/mozilla-central/rev/77b93e70c30a https://hg.mozilla.org/mozilla-central/rev/39471a717494 https://hg.mozilla.org/mozilla-central/rev/5f626c9dd066 https://hg.mozilla.org/mozilla-central/rev/862a066d90d5

For future reference, please make sure the patches in the bug as well as the landed patches have a consistent numbering.