Need a way for platform developers to easily add TimelineMarkers Fitzgen suggested: Simple RAII class to add start/end markers automatically? We'll also need to add probably a "Category" property, as well as a way to store additional metadata. Currently, TimelineMarkers have a name ("ConsoleTime", "Paint", "DOMEvent"), and optionally a stack, and sometimes a "cause" (the label in console.time/timeEnd). This should handle both events with one timestamp (maybe implemented in bug 922221), as well as a start and end time.

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #0) > Need a way for platform developers to easily add TimelineMarkers > > Fitzgen suggested: Simple RAII class to add start/end markers automatically? It's a good idea but was met unfavorably when I tried it last year. But I think it is worth another try. One thing to note is that if the two markers are created in the same tick (which is the case for most existing markers), then we could have this RAII helper automatically link the start and end markers together, and optimize PopProfileTimelineMarkers a bit. > We'll also need to add probably a "Category" property, as well as a way to > store additional metadata. Currently, TimelineMarkers have a name > ("ConsoleTime", "Paint", "DOMEvent"), and optionally a stack, and sometimes > a "cause" (the label in console.time/timeEnd). I'll comment separately on the category bug. Note that any given caller can subclass TimelineMarker and add any sort of data that is relevant. E.g., this is how Paint markers add the rectangle information. If new data is added, all that is needed is an addition to ProfileTimelineMarker.webidl, plus of course a bit of rendering support in marker-details.js.

Decided to just go ahead and make the RAII class before we have proper categories and all that because it makes my life easier in other bugs.

Comment on attachment 8588908 [details] [diff] [review] Add AutoTimelineMarker RAII class Review of attachment 8588908 [details] [diff] [review]: ----------------------------------------------------------------- I'm in favor of it. While working on the event patch, at one point I had unpaired start/end markers and didn't notice... this approach prevents that sort of bug. One minor thing is whether it makes sense to convert some existing calls to use this class. There are two spots in the tree that can use it directly. Another question is whether it makes sense to handle TimelineMarker subclasses using this class. I think subclasses will be the most typical way to interact with the timeline feature, because it's generally best to send along some extra data associated with a marker.

(In reply to Tom Tromey :tromey from comment #4) > One minor thing is whether it makes sense to convert some existing calls to > use this class. There are two spots in the tree that can use it directly. Wherever it makes sense, we should (which is kind of tautological). I wouldn't bend over backwards, but if the start and end markers are in the same scope already, then I think it is worth it so we don't have to remember to end at every return. > Another question is whether it makes sense to handle TimelineMarker > subclasses > using this class. I think subclasses will be the most typical way to > interact > with the timeline feature, because it's generally best to send along some > extra data associated with a marker. So I started with a template that took the derived TimelineMarker and forwarded arguments to to it in the ctor, but then ran into the issue of when the start and end take a different set of parameters, and then realized I didn't know if that was even an issue or not, and then realized I was way too deep in theoretical issues and that I wasn't even going to use TimelineMarker subclasses for the HTML parsing markers. So I just backed up and did the simplest thing possible. So, I think it would make sense to support derived classes, but I'm not sure exactly what it would look like, and I feel like it might involve more changes to the markers themselves as well.

Comment on attachment 8588908 [details] [diff] [review] Add AutoTimelineMarker RAII class I'm worried about not keeping mDocShell alive long enough. So, nsRefPtr<nsDocShell> but only assign anything to that member variable if nsDocShell::gProfileTimelineRecordingsCount > 0

Comment on attachment 8589335 [details] [diff] [review] Add AutoTimelineMarker RAII class >+ DocShellIsRecording(nsDocShell& docShell) aDocShell >+public: >+ explicit AutoTimelineMarker(nsIDocShell* aDocShell, const char* aName >+ MOZ_GUARD_OBJECT_NOTIFIER_PARAM) >+ : mDocShell(nullptr) >+ , mName(aName) >+ { >+ MOZ_GUARD_OBJECT_NOTIFIER_INIT; >+ >+ nsRefPtr<nsDocShell> docShell = static_cast<nsDocShell*>(aDocShell); nsRefPtr does slow virtual Addref/Release, so you need to first check if we're recording, and then just assing docShell to mDocShell. (docShell doesn't need to be nsRefPtr here, only mDocShell) >+ if (docShell && DocShellIsRecording(*docShell)) { >+ mDocShell = Move(docShell); (.swap() tends to be easier to read than Move(), but you don't need either here, if docShell is just DocShell*)

remote: https://hg.mozilla.org/integration/fx-team/rev/47b14c9bfbcb