Closed Bug 1194707 Opened 4 years ago Closed 4 years ago

Remove the docshell param from TimelineMarker constructors

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)

This param is only used to get the current time, but the changes done in bug 1050498 allow the time itself to be passed in specifically. Furthermore, having a docshell as a mandatory param in the base marker class is nonsensical considering our move towards non-docshell dependent markers.
Blocks: otmt-markers
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attached patch v1Splinter Review
Attachment #8649212 - Flags: review?(ttromey)
Comment on attachment 8649212 [details] [diff] [review]
v1

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

Looks good, just some minor comments.

::: docshell/base/timeline/TimelineMarker.h
@@ +12,3 @@
>  #include "GeckoProfiler.h"
> +
> +using mozilla::TimeStamp;

The coding standards say not to use "using" in header files.  See the "C++ namespaces" section of https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +103,3 @@
>  
>    // While normally it is not a good idea to make a persistent root,
>    // in this case changing nsDocShell to participate in cycle

Now that there's no connection to the docshell, I think this comment needs a minor update to follow.
Attachment #8649212 - Flags: review?(ttromey) → review+
(In reply to Tom Tromey :tromey from comment #2)
> Comment on attachment 8649212 [details] [diff] [review]
> v1
> 
> Review of attachment 8649212 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just some minor comments.
> 
> ::: docshell/base/timeline/TimelineMarker.h
> @@ +12,3 @@
> >  #include "GeckoProfiler.h"
> > +
> > +using mozilla::TimeStamp;
> 
> The coding standards say not to use "using" in header files.  See the "C++
> namespaces" section of
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
> 

Huh! Good to know. It's been a while since I looked at that page.

> @@ +103,3 @@
> >  
> >    // While normally it is not a good idea to make a persistent root,
> >    // in this case changing nsDocShell to participate in cycle
> 
> Now that there's no connection to the docshell, I think this comment needs a
> minor update to follow.

True.
https://hg.mozilla.org/mozilla-central/rev/970598158255
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.