1355634 - 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

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1355634 - Use MozPromise to make ProfileGatherer more generic.

Review commit: https://reviewboard.mozilla.org/r/129282/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129282/

Comment 2

[Nicholas Nethercote [inactive]]

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.

Comment 3

[Markus Stange [:mstange]]

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 4

[Markus Stange [:mstange]]

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".

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8857314 [details]
Bug 1355634 - Use MozPromise to make ProfileGatherer more generic.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129282/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/330312270523
Use MozPromise to make ProfileGatherer more generic. r=njn

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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