1183231 - Maintain a list of timeline-observed docshells outside of nsDocShell

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: v1

Comment 2

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

Next step is to move the markers storage outside of nsDocShell.

Comment 4

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

Attachment: v2

Argh forgot to qref again. Sorry about the spam.

Comment 5

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

Patch queue: https://pastebin.mozilla.org/8839722

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8634453 [details] [diff] [review]
v2

># HG changeset patch
># User Victor Porof <[e-mail]>
># Date 1437001680 14400
>#      Wed Jul 15 19:08:00 2015 -0400
># Node ID d3eac967f48e6d7918df1fe92318fe97c6e085b7
># Parent  aeea07206837241d81a487c739399d44d84ee24f
>Bug 1183231 - Maintain a list of timeline-observed docshells outside of nsDocShell, r=tromey
>
>diff --git a/docshell/base/moz.build b/docshell/base/moz.build
>--- a/docshell/base/moz.build
>+++ b/docshell/base/moz.build
>@@ -45,8 +45,9 @@ EXPORTS.mozilla += [
>     'IHistory.h',
>     'LoadContext.h',
>     'timeline/AutoGlobalTimelineMarker.h',
>     'timeline/AutoTimelineMarker.h',
>+    'timeline/ObservedDocShell.h',
>     'timeline/TimelineConsumers.h',
> ]
> 
> UNIFIED_SOURCES += [
>@@ -63,8 +64,9 @@ UNIFIED_SOURCES += [
>     'nsWebNavigationInfo.cpp',
>     'SerializedLoadContext.cpp',
>     'timeline/AutoGlobalTimelineMarker.cpp',
>     'timeline/AutoTimelineMarker.cpp',
>+    'timeline/ObservedDocShell.cpp',
>     'timeline/TimelineConsumers.cpp',
>     'timeline/TimelineMarker.cpp',
> ]
> 
again, this part will look a bit different.




>+#ifndef ObservedDocShell_h__
>+#define ObservedDocShell_h__


mozilla_ObservedDocShell_h_

>+TimelineConsumers::GetKnownDocShells(Vector<nsRefPtr<nsDocShell>>& aStore)
>+{
>+  const LinkedList<ObservedDocShell>& docShells = GetOrCreateObservedDocShellsList();
>+
>+  for (const ObservedDocShell* rds = docShells.getFirst();
>+       rds != nullptr;
>+       rds = rds->getNext()) {
what does rds mean? especially 'r'. I think ds == docshell

>+    if (!aStore.append(**rds)) {
>+      return false;
>+    }
>+  }
>+
>+  return true;
>+}
We should be using nsTArray, but since the old code is using Vector too, fine.


>   // Counter for how many timelines are currently interested in markers.
>   static unsigned long activeConsumers;
>+  static LinkedList<ObservedDocShell>* observedDocShells;
sObserverDocShells;

>+  static LinkedList<ObservedDocShell>& GetOrCreateObservedDocShellsList();
> 
> public:
>-  static void AddConsumer();
>-  static void RemoveConsumer();
>+  static void AddConsumer(nsDocShell* aDocShell,
>+                          UniquePtr<ObservedDocShell>& aRDSPtr);
>+  static void RemoveConsumer(nsDocShell* aDocShell,
>+                             UniquePtr<ObservedDocShell>& aRDSPtr);
What is RDSPtr? DocShellPtr, but R?
Perhaps some better argument name here?

Comment 7

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

Attachment: v3

Addressed comments.

Comment 8

[Pulsebot]

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

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 10

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

This was backed out in: https://hg.mozilla.org/integration/fx-team/rev/fdff9c45b9c9

Comment 11

[Pulsebot]

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

Comment 12

[Carsten Book [:Tomcat]]

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