Closed Bug 1200119 Opened 5 years ago Closed 5 years ago

Add a way to create usable markers from different threads

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(1 file, 1 obsolete file)

We're able to instantiate them, but that's useless. Need a way to tie them to an ObservedDocShell or TimelineConsumers.
Attached patch v1 (obsolete) — Splinter Review
WIP, testing, probably finished
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attached patch v2Splinter Review
We're going to have to update the docs after this.
Attachment #8658131 - Attachment is obsolete: true
Attachment #8659244 - Flags: review?(ttromey)
Comment on attachment 8659244 [details] [diff] [review]
v2

Review of attachment 8659244 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: docshell/base/timeline/AbstractTimelineMarker.cpp
@@ +31,5 @@
> +UniquePtr<AbstractTimelineMarker>
> +AbstractTimelineMarker::Clone()
> +{
> +  MOZ_ASSERT(false, "Clone method not yet implemented on this marker type.");
> +  return nullptr;

This is the only code that I am less than certain about.  Without requiring all subclasses to implement Clone, it means that in the future it would be possible for some other erroneous change to wind up calling this function, causing a crash.  I wonder if you considered just making it "= 0" and fixing up the subclass(es) instead?

That said there's obviously no danger now.
Attachment #8659244 - Flags: review?(ttromey) → review+
That was intentional. Not all markers need to be clonable, as most of them will only be added from the main thread. Whenever there's a need to add a marker off the main thread, one'll hit that crash and obviously see that the method needs to be implemented.
https://hg.mozilla.org/mozilla-central/rev/0d9327e758e5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.