Closed Bug 1184376 Opened 5 years ago Closed 5 years ago

Remove nsDocShell::AddProfileTimelineMarker

Categories

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

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(2 files)

No description provided.
Attached patch v1Splinter Review
Please have a look at this Tom and feel free to send it over to someone else too if necessary.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #8634824 - Flags: review?(ttromey)
See bug 1183235 for the ObservedDocShell implementation, and bug 1183231 for the observed docshells list.
Comment on attachment 8634824 [details] [diff] [review]
v1

I think someone else must review.

BTW it seems to me that TimelineConsumers adds new static initializers,
which I think is still not supposed to be done.  (And if that isn't the rule
any more, I'd like to know.)
Attachment #8634824 - Flags: review?(ttromey) → review?(bugs)
(In reply to Tom Tromey :tromey from comment #4)
> Comment on attachment 8634824 [details] [diff] [review]
> v1
> 
> I think someone else must review.
> 
> BTW it seems to me that TimelineConsumers adds new static initializers,
> which I think is still not supposed to be done.  (And if that isn't the rule
> any more, I'd like to know.)

I have no idea, I haven't been doing any C++ hacking in a very long time.
Comment on attachment 8634824 [details] [diff] [review]
v1

>+TimelineConsumers::AddMarkerForDocShell(nsDocShell* aDocShell,
>+                                        UniquePtr<TimelineMarker>&& aMarker)
>+{
>+  if (aDocShell->IsObserved()) {
>+    aDocShell->mObserved->AddMarker(Move(aMarker));
>+  }
It is a bit messy that in some patches you explicitly pass reference to
mObserved, and here you use it explicitly.
I wonder if DocShell could have a method to return reference and use that everywhere.
So, please add such and use it here. And there was the other patch where
mObserved was passed as param, and IIRC also docshell. Perhaps only docshell could be passed.
If you think a new review is need for that patch, please ask

>+TimelineConsumers::AddMarkerToDocShellsList(Vector<nsRefPtr<nsDocShell>>& docShells,
aDocShells

>+  static void AddMarkerToDocShellsList(Vector<nsRefPtr<nsDocShell>>& docShells,
aDocShells
Attachment #8634824 - Flags: review?(bugs) → review+
Attached patch v2Splinter Review
Addressed comments.
Attachment #8635494 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/577738eb6097
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
This was backed out in: https://hg.mozilla.org/integration/fx-team/rev/fdff9c45b9c9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.