Closed
Bug 949762
Opened 10 years ago
Closed 10 years ago
Tracer should respect black boxing
Categories
(DevTools :: Debugger, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 34
People
(Reporter: fitzgen, Assigned: jjurovych)
References
Details
Attachments
(3 files, 4 obsolete files)
193.48 KB,
image/png
|
Details | |
404.94 KB,
image/png
|
Details | |
16.73 KB,
patch
|
Details | Diff | Splinter Review |
We should hide or grey out frame enters/exit logs in the traces view when the frame's source is black boxed. This will make it much easier for users to visually parse the logs and find the things they are interested in. I'm leaning towards "grey out", but could be convinced either way.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → nfitzgerald
Reporter | ||
Updated•10 years ago
|
Assignee: nfitzgerald → nobody
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jjurovych
Assignee | ||
Comment 2•10 years ago
|
||
This patch keeps track of what traces come from blackboxed sources and then greys them out. So far: * successive traces from blackboxed sources do not get collapsed * (un)blackboxing a source doesn't affect existing traces I will be working on these next. Do we want to land this now or wait till everything's ready?
Attachment #8458958 -
Flags: review?(nfitzgerald)
Assignee | ||
Comment 3•10 years ago
|
||
This is what it looks like right now after clicking button, then blackboxing source and then clicking button again.
Assignee | ||
Comment 4•10 years ago
|
||
Backbone.js TodoMVC with jquery.js blackboxed.
Reporter | ||
Comment 5•10 years ago
|
||
Comment on attachment 8458958 [details] [diff] [review] bug-949762.patch Review of attachment 8458958 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good, but see comment below about reusing SourceClient and SourceActor rather than reimplementing the management of black boxed sources. > Do we want to land this now or wait till everything's ready? This is definitely an improvement over what we have now, so I think we should ship it (once the patch is fixed up a bit) and do follow ups to make it even better :) ::: browser/devtools/debugger/debugger-controller.js @@ +1077,5 @@ > this._onSourcesAdded = this._onSourcesAdded.bind(this); > this._onBlackBoxChange = this._onBlackBoxChange.bind(this); > this._onPrettyPrintChange = this._onPrettyPrintChange.bind(this); > + > + this.blackBoxedSources = new Set(); // Stores URLs of blackBoxed sources Rather than creating another place we store and maintain sources' black boxed state, we should re-use the source's SourceClient which already maintains this data. To do this, the TraceActor should send the function's SourceActor's form in its "enterFrame" packets. It can get this from the ThreadSources object. ::: browser/devtools/debugger/test/browser_dbg_tracing-07.js @@ +17,5 @@ > + gPanel = aPanel; > + gDebugger = gPanel.panelWin; > + > + waitForSourceShown(aPanel, "code_tracing-01.js") > + .then(() => startTracing(aPanel)) For new tests, we like to use Task.jsm. Should be pretty straightforward to make this test use it.
Attachment #8458958 -
Flags: review?(nfitzgerald)
Assignee | ||
Comment 6•10 years ago
|
||
@fitzgen I have refactored the code. Rather than storing a copy of blackboxed sources, now I ask the ThreadActor. This means that I'm calling a sibling's method, which we wanted to avoid. I think this is still better than extending a parent (TabActor/RootActor) from a children (ThreadActor), though. ThreadSources now live in actors/utils/Sources.js instead of actors/script.js, but unless we actually use them somewhere else besides ThreadActor, this change should probably be reverted. Do you have something particular in mind how to go around accessing a sibling?
Attachment #8458958 -
Attachment is obsolete: true
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8461279 [details] [diff] [review] bug-949762-2-wip.patch Review of attachment 8461279 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Jakub Jurovych from comment #6) > Created attachment 8461279 [details] [diff] [review] > bug-949762-2-wip.patch > > @fitzgen I have refactored the code. Rather than storing a copy of > blackboxed sources, now I ask the ThreadActor. This means that I'm calling a > sibling's method, which we wanted to avoid. I think this is still better > than extending a parent (TabActor/RootActor) from a children (ThreadActor), > though. > > ThreadSources now live in actors/utils/Sources.js instead of > actors/script.js, but unless we actually use them somewhere else besides > ThreadActor, this change should probably be reverted. > > Do you have something particular in mind how to go around accessing a > sibling? I meant to have the parent actor own the sources, not for any kind of extending or inheritence, which I agree would be a bad idea. It's fine though, I think you're right that leaving the sources on the thread is ok. Let's not split it out from script.js. This should have an xpcshell test that creates two sources (say, A and B) which calls A->B->A. Then test black boxing one of the sources and verifying the traced logs. ::: toolkit/devtools/server/Makefile.in @@ +7,5 @@ > libs:: > $(INSTALL) $(IFLAGS1) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/devtools > $(INSTALL) $(IFLAGS1) $(srcdir)/*.js $(FINAL_TARGET)/modules/devtools/server > $(INSTALL) $(IFLAGS1) $(srcdir)/actors/*.js $(FINAL_TARGET)/modules/devtools/server/actors > + $(INSTALL) $(IFLAGS1) $(srcdir)/actors/utils/*.js $(FINAL_TARGET)/modules/devtools/server/actors/utils \ No newline at end of file FYI, bug 1039952 landed in fx-team, so pretty soon you won't need this because that bug includes it. ::: toolkit/devtools/server/actors/script.js @@ +516,5 @@ > return this._threadLifetimePool; > }, > > + /** > + * Sources are stored in a parent to make them accessible to other actors. Nit: "stored in the ThreadActor's parent" ... but it isn't anyways here.
Attachment #8461279 -
Flags: feedback+
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8461279 -
Attachment is obsolete: true
Attachment #8468928 -
Flags: review?(nfitzgerald)
Assignee | ||
Comment 9•10 years ago
|
||
Fixing test title.
Attachment #8468928 -
Attachment is obsolete: true
Attachment #8468928 -
Flags: review?(nfitzgerald)
Attachment #8468973 -
Flags: review?(nfitzgerald)
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8468973 [details] [diff] [review] bug-949762-4.patch Review of attachment 8468973 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! r+ with comments below addressed. ::: browser/devtools/debugger/test/browser_dbg_tracing-07.js @@ +1,5 @@ > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +/** > + * Test that we get the expected frame enter/exit logs in the tracer view. I think this test is more about trace logs + black boxing. Left over from copy+paste? @@ +65,5 @@ > + SpecialPowers.pushPrefEnv({'set': [["devtools.debugger.tracer", true]]}, deferred.resolve); > + return deferred.promise; > +} > + > +function popPref() { These seem like pretty useful things to share in head.js, if we didn't hard code the prefs we were pushing :) ::: toolkit/devtools/server/tests/unit/test_trace_actor-10.js @@ +1,5 @@ > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +/** > + * Create 2 sources, A and B, B is black boxed. When calling functions A->B->A, verify that only Nit: wrap at 80 chars, please @@ +46,5 @@ > + > + yield startTrace(); > + > + yield executeOnNextTickAndWaitForPause(evalSetup, gClient); > + yield resume(gThreadClient); I don't think you need to jump through the executeOnNextTickAndWaitForPause hoops if you're just going to resume again. Instead you can just eval the text on this tick of the event loop and not have a debugger statement at the end of the text. The reason we do that in other cases has to do with the interaction between promises/task.jsm, debugger pauses, and event loops. I don't think it's necessary when we don't need a pause.
Attachment #8468973 -
Flags: review?(nfitzgerald) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e30861ecc5c1
Attachment #8468973 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/93f366d6cb3c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•