1196294 - Remove the mCause from TimelineMarkers

Status: RESOLVED FIXED

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

Description

[Victor Porof [:vporof][:vp]]

This field is only used in a few subclasses, and having it on the base TimelineMarker class doesn't really help with any code duplication. In the interest of trimming down the base class as much as we can, let's separate concerns.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 2

[Tom Tromey :tromey]

Comment on attachment 8649931 [details] [diff] [review]
v1

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

Looks good.

::: docshell/base/timeline/ConsoleTimelineMarker.h
@@ +33,5 @@
>      }
> +    // Console markers must have matching causes as well. It is safe to perform
> +    // a static_cast here as the previous equality check ensures that this is
> +    // a console marker instance.
> +    return mCause == static_cast<const ConsoleTimelineMarker*>(&aOther)->mCause;

This gives me mild pause, because the previous test is just based on the content of a string; so it opens the door to some error in theory.  However I don't think anything better can be done without a relatively heavyweight solution.

Comment 3

[Victor Porof [:vporof][:vp]]

(In reply to Tom Tromey :tromey from comment #2)
> Comment on attachment 8649931 [details] [diff] [review]
> v1
> 
> Review of attachment 8649931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: docshell/base/timeline/ConsoleTimelineMarker.h
> @@ +33,5 @@
> >      }
> > +    // Console markers must have matching causes as well. It is safe to perform
> > +    // a static_cast here as the previous equality check ensures that this is
> > +    // a console marker instance.
> > +    return mCause == static_cast<const ConsoleTimelineMarker*>(&aOther)->mCause;
> 
> This gives me mild pause, because the previous test is just based on the
> content of a string; so it opens the door to some error in theory.  However
> I don't think anything better can be done without a relatively heavyweight
> solution.

Indeed. I feel disgusted for having to write this.

I was thinking of removing the "name" altogether in favor of an enum, or better yet just enforce subclassing whenever a new type of marker needs to be added. What do you think about that?

Comment 4

[Tom Tromey :tromey]

(In reply to Victor Porof [:vporof][:vp] from comment #3)

> I was thinking of removing the "name" altogether in favor of an enum, or
> better yet just enforce subclassing whenever a new type of marker needs to
> be added. What do you think about that?

Yeah, I think that would be an improvement.
It still wouldn't be truly typesafe, but at least it would provide a spot
to document the needed behavior.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ce62db17d0ec

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce62db17d0ec