Closed Bug 1334466 Opened 3 years ago Closed 3 years ago

Merge Sampler and GeckoSampler

Categories

(Core :: Gecko Profiler, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(5 files)

GeckoSampler is the only subclass of Sampler. Might as well merge them.
They're defined separately for each platform, but the definitions are almost
identical and can be commoned up.
Attachment #8831110 - Flags: review?(mstange)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
There's no point having them as separate classes. This removes the need for
some virtual functions, too.
Attachment #8831111 - Flags: review?(mstange)
The patch also moves some Sampler methods from platform.cpp to Sampler.cpp. Now
all Sampler methods are in Sampler.cpp except for a small number of
platform-specific ones, which are in platform-*.cpp.
Attachment #8831113 - Flags: review?(mstange)
Because it's always true.
Attachment #8831114 - Flags: review?(mstange)
It's unused.
Attachment #8831116 - Flags: review?(mstange)
Attachment #8831110 - Flags: review?(mstange) → review+
Attachment #8831111 - Flags: review?(mstange) → review+
Comment on attachment 8831113 [details] [diff] [review]
(part 3) - Rename GeckoSampler.cpp as Sampler.cpp

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

::: tools/profiler/core/Sampler.cpp
@@ +165,5 @@
>    return false;
>  }
>  
> +std::vector<ThreadInfo*>* Sampler::sRegisteredThreads = nullptr;
> +mozilla::UniquePtr< ::Mutex> Sampler::sRegisteredThreadsMutex;

Shouldn't this be mozilla::Mutex? Looks like I missed this in the patch that removed ::Mutex.
Attachment #8831113 - Flags: review?(mstange) → review+
Attachment #8831114 - Flags: review?(mstange) → review+
Attachment #8831116 - Flags: review?(mstange) → review+
> > +std::vector<ThreadInfo*>* Sampler::sRegisteredThreads = nullptr;
> > +mozilla::UniquePtr< ::Mutex> Sampler::sRegisteredThreadsMutex;
> 
> Shouldn't this be mozilla::Mutex? Looks like I missed this in the patch that
> removed ::Mutex.

The declaration in platform.h uses mozilla::Mutex. So ::Mutex must somehow auto-qualify as mozilla::Mutex. But I've changed it locally for clarity.
You need to log in before you can comment on or make changes to this bug.