1869522 - Add a basic "moz:profiler" module for starting and stopping the profiler

Status: ASSIGNED

(Remote Protocol :: WebDriver BiDi, enhancement, P3)

Description

[Nazım Can Altınova [:canova]]

This is the bug where we will experiment very basic functionality for "moz:tracing" WebDriver BiDi module.

Comment 1

[Nazım Can Altınova [:canova]]

Attachment: Bug 1869522 - [bidi] Add a moz:profiler module with start/stop methods r?jdescottes

Depends on D196161

Comment 2

[Julian Descottes [:jdescottes]]

Hi Nazim,

Do you plan to resume the work here?
The question about having a moz-specific profiler module popped up during a recent devtools meeting :)

cc :ochameau

Comment 3

[Nazım Can Altınova [:canova]]

Ah, I had forgotten about this patch. I'm a bit busy with lots of things at the moment, but on the other hand, it's not a big effort to land this. If you can land bug 1868828 it should be easy to rebase and land it after small changes.

It's been a while since I wrote this patch, and looking at it back, I'm not happy with the moz:tracing naming. I think it would be better to rename this module to moz:profiling instead. I think I initially wanted to name it tracing thinking that it's good to keep it similar to CDP. But it doesn't make sense for Firefox.
Other than this, the patch is pretty self contained and small.

Comment 4

[Julian Descottes [:jdescottes]]

(In reply to Nazım Can Altınova [:canova][:canaltinova on phabricator] from comment #3)

Ah, I had forgotten about this patch. I'm a bit busy with lots of things at the moment, but on the other hand, it's not a big effort to land this. If you can land bug 1868828 it should be easy to rebase and land it after small changes.

It's been a while since I wrote this patch, and looking at it back, I'm not happy with the moz:tracing naming. I think it would be better to rename this module to moz:profiling instead. I think I initially wanted to name it tracing thinking that it's good to keep it similar to CDP. But it doesn't make sense for Firefox.
Other than this, the patch is pretty self contained and small.

Bug 1868828 is on autoland now, feel free to resume work here. Note that we quickly discussed about the folder structure for vendored modules and it sounds good to keep them under a moz/ subfolder (as you did in your current patch)

Comment 5

[Nazım Can Altınova [:canova]]

(In reply to Julian Descottes [:jdescottes] from comment #4)

Bug 1868828 is on autoland now, feel free to resume work here.

Nice, thanks for working on it!

Note that we quickly discussed about the folder structure for vendored modules and it sounds good to keep them under a moz/ subfolder (as you did in your current patch)

I see, sounds good. So would the path formoz:profiler be remote/webdriver-bidi/modules/root/moz/mozProfiler.sys.mjs or remote/webdriver-bidi/modules/root/moz/profiler.sys.mjs (or uppercase P in profiler?)

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So given that we want to use the profiler with sampling here (as I understand) and not for tracing, lets rename the bug so that it reflects the goal for this bug.

(In reply to Nazım Can Altınova [:canova][:canaltinova on phabricator] from comment #5)

I see, sounds good. So would the path formoz:profiler be remote/webdriver-bidi/modules/root/moz/mozProfiler.sys.mjs or remote/webdriver-bidi/modules/root/moz/profiler.sys.mjs (or uppercase P in profiler?)

Similar to the other modules it should be remote/webdriver-bidi/modules/root/moz/profiler.sys.mjs. The vendor prefix is already encoded in the folder. But nevertheless it opens a question what should happen if we have two identical module names (one public and one as vendored module). Should we encode the moz prefix in the class name?

Comment 7

[Julian Descottes [:jdescottes]]

The only requirement from the framework's point of view is to export it as export const profiler. Then internally the module class can be ProfilerModule, MozProfilerModule, etc...

Note that for root/windowglobal modules we don't explicitly call the class eg RootNetworkModule/WindowglobalNetworkModule, but just NetworkModule. So we already have duplicate classnames in that case.

Comment 8

[Nazım Can Altınova [:canova]]

Attachment: Bug 1869522 - [wdspec] Add wdspec tests for moz:profiler module r?jdescottes

Adds wdspec tests for the moz:profiler.start and moz:profiler.stop
commands, including:

• Basic start/stop behavior, including stopping the profiler when
  it isn't running and calling start/stop multiple times.
• Invalid argument tests for all start parameters (entries, interval,
  features, threads, context, duration).
• NoSuchFrame error when an unknown context id is passed to start.
• Writing a profile to disk via stop({ filePath }), including a
  JSON sanity check on the resulting file.
• UnknownError when the profile file path cannot be written.
• Invalid argument tests for the stop filePath parameter.