Stop using TracingMetadata from GeckoProfiler.h

RESOLVED FIXED in Firefox 43

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: vporof, Assigned: vporof)

Tracking

unspecified
Firefox 43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

It's just an enum, with some fields we don't even need. There's no reason to include/reuse it everywhere.
Blocks: otmt-markers
Posted patch v1 (obsolete) — Splinter Review
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #8649955 - Flags: review?(ttromey)
Comment on attachment 8649955 [details] [diff] [review]
v1

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

The patch overall looks good.  However I am going to r- it for now because it is missing a crucial file.

From the way the code is written I'm going to guess that the new enum is not an "enum class".  I think the class form is generally preferable in new code because it introduces a new name space and thus avoids possible name clashes.  So I would recommend this; though unfortunately it means another pass through all the files.  Another equivalent approach would be to make the non-class enum a member of TimelineMarker.

Also, could you see if nsDocShell.h can now drop the GeckoProfiler.h include?  I vaguely recall that the enum was the only reason for this, but I'm not certain.

::: docshell/base/timeline/TimelineMarker.h
@@ +8,5 @@
>  #define mozilla_TimelineMarker_h_
>  
>  #include "nsString.h"
>  #include "nsContentUtils.h"
> +#include "TimelineMarkerEnums.h"

This file is missing from the patch.
Attachment #8649955 - Flags: review?(ttromey) → review-
> 
> ::: docshell/base/timeline/TimelineMarker.h
> @@ +8,5 @@
> >  #define mozilla_TimelineMarker_h_
> >  
> >  #include "nsString.h"
> >  #include "nsContentUtils.h"
> > +#include "TimelineMarkerEnums.h"
> 
> This file is missing from the patch.

OOOPS
Posted patch v2Splinter Review
Attachment #8649974 - Flags: review?(ttromey)
Comment on attachment 8649955 [details] [diff] [review]
v1

Mechanical change.
Attachment #8649955 - Attachment is obsolete: true
Comment on attachment 8649974 [details] [diff] [review]
v2

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

Looks good, thanks.

::: dom/base/Console.cpp
@@ +1105,5 @@
>            nsAutoJSString key;
>            if (key.init(aCx, jsString)) {
>              mozilla::UniquePtr<TimelineMarker> marker =
> +              MakeUnique<ConsoleTimelineMarker>(key, aMethodName == MethodTime 
> +                ? MarkerTracingType::START 

Splinter shows some trailing whitespace on these two lines.
Attachment #8649974 - Flags: review?(ttromey) → review+
https://hg.mozilla.org/mozilla-central/rev/e8615fd9bb21
Status: ASSIGNED → RESOLVED
Closed: 4 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.