We need to start source mapping the location of function grips.

Making the function grips return a promise, makes createValueGrip return a promise, makes half of the debugger server need to be rewritten in a gross way. So much churn for something so little. Instead, allowing you to return a packet with a (nested) property be a promise and we can deep resolve packets before sending them.

test_profiler.js seems to be failing locally, haven't dug too deep in yet.

Talked with Jim and Panos, going to split this info out into its own request.

This is one of two options. The second option is coming soon. So this patch just adds the definition site to the function grips in the "eventListeners" response. Pros: * No need to change frontend code * No extra round trip request/response for the definition site info Cons: * Bit of code bloat * Why even have a "definitionSite" request type if we are going to go around it all the time in the server?

So this patch makes the debugger controller request the definitionSite of each listener's function after it gets the list of event listeners, but before it adds them to the view. -------------------- Option 3 would be for the view to partially render without the url, and have it request the definitionSite and incrementally update the UI when it gets a response back. This would give the fastest initial UI response for the user, but a) it might be jarring to have the UI update incrementally (or maybe not) b) it would require enough changes to browser_dbg_break-on-events-03.js that I didn't want to do it unless we decided this was definitely the correct path forward.

The more I think about it, the more I am leaning towards option 3... but I'd like to hear more opinions. Victor, what do you think?

(In reply to Nick Fitzgerald [:fitzgen] from comment #6) > Created attachment 827735 [details] [diff] [review] > > So this patch makes the debugger controller request the definitionSite of > each listener's function after it gets the list of event listeners, but > before it adds them to the view. > This looks like the easiest thing to implement, code-wise, breaking the least amount of tests, making our lifes as programmers easy. However, it might make the Events tab look like (actually, even *be*, on remote connections) "slow", for large numbers of event listeners. > -------------------- > > Option 3 would be for the view to partially render without the url, and have > it request the definitionSite and incrementally update the UI when it gets a > response back. This would give the fastest initial UI response for the user, > but > This definitely requires a bit more work, and being careful about things, but the UI won't be jumpy (no reflows, due to how the widget works, except for setting labels). Tests will be tricky, but I don't think they'll be that hard to fix. I am ok with both options, however slightly leaning towards option 3. Can definitely be postponed for a followup if time is an issue.

Comment on attachment 827735 [details] [diff] [review] part 2 option 2 - request definitionSite before adding listeners to the view Review of attachment 827735 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/debugger-controller.js @@ +1403,5 @@ > + DevToolsUtils.reportException("scheduleEventListenersFetch", msg); > + return; > + } > + > + promise.all(aResponse.listeners.map(listener => { This is very cute.

Going to implement option 3.

Comment on attachment 829000 [details] [diff] [review] part 2: frontend changes Review of attachment 829000 [details] [diff] [review]: ----------------------------------------------------------------- Pretty dense, but it looks like it'll do the trick. I'll need to have another pass through it, please address the comments and ask me again for r?. ::: browser/devtools/debugger/debugger-controller.js @@ +49,5 @@ > CONDITIONAL_BREAKPOINT_POPUP_HIDING: "Debugger:ConditionalBreakpointPopupHiding", > > // When event listeners are fetched or event breakpoints are updated. > EVENT_LISTENERS_FETCHED: "Debugger:EventListenersFetched", > + EVENT_LISTENER_URL_FETCHED: "Debugger:EventListenerUrlFetched", Is this event necessary? I don't see it being used in tests. @@ +1439,5 @@ > // Add all the listeners in the debugger view event linsteners container. > for (let listener of aResponse.listeners) { > + let deferred = promise.defer(); > + urlsFetched.push(deferred.promise); > + gThreadClient.pauseGrip(listener.function).getDefinitionSite(aResponse => { I'd prefer having all of this gripping and getting outside of the loop and after the view items were committed, to make sure things are not stutterring while the view is populated. Something like this: const outstanding = []; // ... for (let listener of aResponse.listeners) { let deferred = promise.defer(); listener.url = deferred.promise; outstanding.push([listener, deferred]); DebuggerView.EventListeners.addListener(listener, { staged: true }); } // ... DebuggerView.EventListeners.commit(); // explain here what's happening with urls and why for (let [listener, deferred] of outstanding) { gThreadClient.pauseGrip(listener.function).getDefinitionSite(... => { deferred.resolve/reject(...); }); } return outstanding.map(([, deferred]) => deferred.promise); Do you think the above makes more sense? @@ +1453,5 @@ > > // Flushes all the prepared events into the event listeners container. > DebuggerView.EventListeners.commit(); > + > + return promise.all(urlsFetched); Hmm, this is indeed resolved when all the urls are fetched, but does that necessarily mean the view was updated as well? Are the tests depending on EVENT_LISTENERS_FETCHED assuming the UI already contains all the necessary information? (things used to be sync, now they're not etc.). It's probably fine, but just making sure you've thought about it. ::: browser/devtools/debugger/debugger-panes.js @@ +1590,1 @@ > aItem.attachment.type == type); Nit: you can keep this predicate on a single line now. @@ +1598,5 @@ > + if (this._updateExistingItem(url, type, selector)) { > + return; > + } > + // No item for this event and url; we need to create it. > + const thisItem = this._addItem(type, selector, promise.resolve(url), Why do you need promise.resolve(url)? Can't you just send the url string? @@ +1609,5 @@ > + }); > + return; > + } > + > + // There is not already an item for this type. We will go ahead and create a A bit of funny wording here imo. "There is no item for this type yet" sounds better? @@ +1617,5 @@ > + aOptions.staged); > + > + url.then(url => { > + if (this._updateExistingItem(url, type, selector)) { > + this.removeChild(thisItem); Where is removeChild defined? Why is this necessary now? @@ +1626,5 @@ > + window.emit(EVENTS.EVENT_LISTENER_URL_FETCHED, thisItem); > + }).then(null, error => { > + DevToolsUtils.reportException("EventListenersView#addListener", > + error); > + this.removeChild(thisItem); Ditto. @@ +1628,5 @@ > + DevToolsUtils.reportException("EventListenersView#addListener", > + error); > + this.removeChild(thisItem); > + }); > + }, Whooh. This function became a bit all over the place now... Can you dry things up a bit, especially the "add new item or update existing item then set the url attachment" bits? @@ +1711,5 @@ > + const pushedItem = this.push([itemView.container, aActor, group], { > + staged: aStaged, /* stage the item to be appended later? */ > + attachment: { > + url: null, > + urlPromise: aUrl, urlPromise isn't being used anywhere afaict. @@ +1712,5 @@ > + staged: aStaged, /* stage the item to be appended later? */ > + attachment: { > + url: null, > + urlPromise: aUrl, > + actor: aActor, Also, why is the actor both added as the item's value and a property in the item's attachment object? Now, hovering the item would show a tooltup with the actor's id :) @@ +1723,5 @@ > + }); > + > + // If the item was staged, it wasn't returned by |push|, so we have to find > + // it again. > + return pushedItem || this.getItemForPredicate(aItem => { Why is this necessary? Why should the event item be returned to the outside world from this function? ps: getItemForPredicate should return items even if they're staged. ::: browser/devtools/shared/widgets/ViewHelpers.jsm @@ +281,5 @@ > getStr: function(aName) { > + try { > + return this.stringBundle.GetStringFromName(aName); > + } catch (e) { > + e.stack = e.stack || Error().stack; Can't use safeErrorString or whatever it's called here?

Comment on attachment 827712 [details] [diff] [review] part 1 - RDP changes Review of attachment 827712 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/script.js @@ +2754,5 @@ > + */ > + onDefinitionSite: function OA_onDefinitionSite(aRequest) { > + if (this.obj.class != "Function") { > + return { > + from: this.actorID, Generally you don't need to add the "from" property to the return packet, it will be added automatically. ::: toolkit/devtools/server/tests/unit/test_objectgrips-12.js @@ +47,5 @@ > +} > + > +function test_definition_site(func, obj) { > + func.getDefinitionSite(({ error, url, line, column }) => { > + do_check_true(!error); Even: do_check_false(error);

Comment on attachment 829000 [details] [diff] [review] part 2: frontend changes After talking with victor in irc, we are going back to option 2.

Comment on attachment 827735 [details] [diff] [review] part 2 option 2 - request definitionSite before adding listeners to the view All tests still pass locally :)

(In reply to Panos Astithas [:past] from comment #14) > ::: toolkit/devtools/server/tests/unit/test_objectgrips-12.js > @@ +47,5 @@ > > +} > > + > > +function test_definition_site(func, obj) { > > + func.getDefinitionSite(({ error, url, line, column }) => { > > + do_check_true(!error); > > Even: do_check_false(error); Can't because it isn't false, its undefined.

Oops, you're right. Mea culpa.

https://hg.mozilla.org/integration/fx-team/rev/a976c1645d70 https://hg.mozilla.org/integration/fx-team/rev/854623def9c6

Backed out in https://hg.mozilla.org/integration/fx-team/rev/fd3869f9c83f for Windows and Linux failing like https://tbpl.mozilla.org/php/getParsedLog.php?id=30381255&tree=Fx-Team

note that part 1 is in and also merged to mozilla-central https://hg.mozilla.org/mozilla-central/rev/a976c1645d70 but part 2 was backed out

Tomcat, this is both patches that were backed out.

Can't repro locally (as expected, since I am on OSX), so here is a try push to make sure I am not crazy: https://tbpl.mozilla.org/?tree=Try&rev=f78cf50bec32

The event-listeners test needed the same fix as debugger-controller, since it bypasses the UI and tests the protocol interaction directly.

Try: https://tbpl.mozilla.org/?tree=Try&rev=0dddf95ffa7b

Comment on attachment 8334872 [details] [diff] [review] part 3: fix the event-listeners test; Review of attachment 8334872 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for taking a look at this on your linux machine, Panos! Looking forward to those try results...

Only unrelated oranges in the try run.

https://hg.mozilla.org/integration/fx-team/rev/b0217b81bee2 https://hg.mozilla.org/integration/fx-team/rev/bb2535b52c74 https://hg.mozilla.org/integration/fx-team/rev/15e0dbb08a2d

https://hg.mozilla.org/mozilla-central/rev/b0217b81bee2 https://hg.mozilla.org/mozilla-central/rev/bb2535b52c74 https://hg.mozilla.org/mozilla-central/rev/15e0dbb08a2d