Closed Bug 1355634 Opened 3 years ago Closed 3 years ago

Don't have special cases in ProfileGatherer for ToJSON and file output; only handle the asynchronicity and leave it up to the caller to use the data

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mstange, Assigned: mstange)

Details

Attachments

(1 file)

No description provided.
Attachment #8857314 - Flags: review?(n.nethercote)
Comment on attachment 8857314 [details]
Bug 1355634 - Use MozPromise to make ProfileGatherer more generic.

https://reviewboard.mozilla.org/r/129282/#review131790

Nice.

::: tools/profiler/gecko/ProfileGatherer.cpp:73
(Diff revision 1)
> -ProfileGatherer::Start(double aSinceTime, Promise* aPromise)
> +ProfileGatherer::Start(double aSinceTime)
>  {
>    MOZ_RELEASE_ASSERT(NS_IsMainThread());
>  
>    if (mGathering) {
> -    // If we're already gathering, reject the promise - this isn't going
> +    // If we're already gathering, return a rejected the promise - this isn't

"return a rejected the promise" reads badly.

::: tools/profiler/moz.build:39
(Diff revision 1)
>          'gecko/ProfilerIOInterposeObserver.cpp',
>          'gecko/ThreadResponsiveness.cpp',
>      ]
>      if CONFIG['OS_TARGET'] == 'Darwin':
>          SOURCES += [
> -            'gecko/ProfileGatherer.cpp',
> +            'gecko/nsProfiler.cpp',

Do you know why this can't be built in unified mode? If so, please add a comment explaining why.
Attachment #8857314 - Flags: review?(n.nethercote) → review+
Comment on attachment 8857314 [details]
Bug 1355634 - Use MozPromise to make ProfileGatherer more generic.

https://reviewboard.mozilla.org/r/129282/#review131790

> Do you know why this can't be built in unified mode? If so, please add a comment explaining why.

Will do.

Doing things in this order goes wrong:
#include "TextRange.h"
using namespace mozilla;
#include "nsLocalFile.h"

Because nsLocalFile.h has a Mac system header somewhere in its include tree which uses its own type called TextRange. And one of the uses of that type is ambiguous with mozilla::TextRange.
Comment on attachment 8857314 [details]
Bug 1355634 - Use MozPromise to make ProfileGatherer more generic.

https://reviewboard.mozilla.org/r/129282/#review131790

> "return a rejected the promise" reads badly.

Removed the "the".
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/330312270523
Use MozPromise to make ProfileGatherer more generic. r=njn
https://hg.mozilla.org/mozilla-central/rev/330312270523
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.