Status

enhancement
P1
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
Firefox 51
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [reserve-html])

Attachments

(1 attachment)

Assignee

Description

3 years ago
At least one spot in devtools/shared uses components.stack.
This is only used for logging, so probably not very important.
However at the moment it does mean an unavoidable require("chrome"),
so something will have to be done.
Assignee

Comment 1

3 years ago
There's one in protocol.js as well, plus a use of Cu.callFunctionWithAsyncStack
Whiteboard: [devtools-html] → [devtools-html] [triage]
Hopefully we can keep the stack and Cu.callFunctionWithAsyncStack when running in Firefox.  It really makes debugging protocol issues much, much easier.
Flags: qe-verify-
Priority: -- → P3
Whiteboard: [devtools-html] [triage] → [reserve-html]
Priority: P3 → P2
Assignee

Updated

3 years ago
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
Comment hidden (mozreview-request)

Comment 4

3 years ago
mozreview-review
Comment on attachment 8779037 [details]
Bug 1287910 - move devtools stack-related APIs to per-platform require;

https://reviewboard.mozilla.org/r/70086/#review67720

Overall, I like how this approach feels so far.  It seems nice to keep the chrome vs. content needs isolated to separate modules like they are here for future static auditing of which paths are used where.

There are some additional places that use Components.stack (capital C), mostly tests.  Should we change those to use the new module as well?

https://dxr.mozilla.org/mozilla-central/search?q=omponents.stack+path%3Adevtools&redirect=false

::: devtools/shared/Loader.jsm:38
(Diff revision 1)
>        // ⚠ DISCUSSION ON DEV-DEVELOPER-TOOLS REQUIRED BEFORE MODIFYING ⚠
>        "": "resource://gre/modules/commonjs/",
>        // ⚠ DISCUSSION ON DEV-DEVELOPER-TOOLS REQUIRED BEFORE MODIFYING ⚠
> +      // Modules here are intended to have one implementation for
> +      // chrome, and a separate implementation for content.
> +      "devtools/platform": "resource://devtools/shared/platform/chrome",

Shouldn't it be `devtools/shared/platform` to reduce diversion from the actual file path?

Maybe throw a quick README.md or something into devtools/shared/platform to explain that the folders are mapped differently by different loaders?  (Not sure anyone would stumble into the file though...)

I am just thinking that we're breaking the convention that the module ID is the source tree path for these files, so more docs are better...?

Overall the approach seems reasonable to me, just want to be sure it's clear to others as best as we can make it be so.

Maybe also extend this source comment to mention that a content loader would instead map this to the sibling content directory.

Should we add a follow up lint to check that we don't (accidentally or otherwise...) require the chrome version directly?  Such as require("devtools/shared/platform/chrome/foo")?

::: devtools/shared/event-emitter.js:60
(Diff revision 1)
>      };
>      factory.call(this, require, this, { exports: this }, console);
>      this.EXPORTED_SYMBOLS = ["EventEmitter"];
>    }
>  }).call(this, function (require, exports, module, console) {
>    // ⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠

Yay, someone added more warning icons! :)

::: devtools/shared/event-emitter.js:69
(Diff revision 1)
>  
>    // See comment in JSM module boilerplate when adding a new dependency.
> -  const { components } = require("chrome");
>    const Services = require("Services");
>    const defer = require("devtools/shared/defer");
> +  const { describeNthCaller } = require("devtools/platform/stack");

Well, you've already done the work to make this available even to JSM importers, so we can keep that if you want...

My personal preference would be to avoid infecting more files with the JSM supporting module header if we don't need it for DevTools.  For this case, we could choose to omit this feature for JSM users.  One way would be for stack module helper to return an empty object and then test for `describeNthCaller`.  Then the JSM header from stack can be removed.

Or you can leave it like it is if you're not as bothered by the extra gunk.

::: devtools/shared/platform/content/test/xpcshell.ini:6
(Diff revision 1)
> +[DEFAULT]
> +tags = devtools
> +head =
> +tail =
> +firefox-appdir = browser
> +skip-if = toolkit == 'android' || toolkit == 'gonk'

Drop the skip-if until there's data to say we need it (I assume it was copied from an existing folder).
Attachment #8779037 - Flags: review?(jryans)
Assignee

Comment 5

3 years ago
mozreview-review-reply
Comment on attachment 8779037 [details]
Bug 1287910 - move devtools stack-related APIs to per-platform require;

https://reviewboard.mozilla.org/r/70086/#review67720

I went through all the calls (actually before, which I should have mentioned).

We aren't usually modifying the tests or devtools/server.
From my search this leaves just network-monitor.js -- but in code that is
only run on the server.

So, I think this is ok as-is.

> Shouldn't it be `devtools/shared/platform` to reduce diversion from the actual file path?
> 
> Maybe throw a quick README.md or something into devtools/shared/platform to explain that the folders are mapped differently by different loaders?  (Not sure anyone would stumble into the file though...)
> 
> I am just thinking that we're breaking the convention that the module ID is the source tree path for these files, so more docs are better...?
> 
> Overall the approach seems reasonable to me, just want to be sure it's clear to others as best as we can make it be so.
> 
> Maybe also extend this source comment to mention that a content loader would instead map this to the sibling content directory.
> 
> Should we add a follow up lint to check that we don't (accidentally or otherwise...) require the chrome version directly?  Such as require("devtools/shared/platform/chrome/foo")?

> Shouldn't it be devtools/shared/platform to reduce diversion from the actual file path?

I couldn't decide.  I'll change it.

I'll add the readme and the comment.

It's easy to do the eslint check so I'm sticking that in this patch too.

> Well, you've already done the work to make this available even to JSM importers, so we can keep that if you want...
> 
> My personal preference would be to avoid infecting more files with the JSM supporting module header if we don't need it for DevTools.  For this case, we could choose to omit this feature for JSM users.  One way would be for stack module helper to return an empty object and then test for `describeNthCaller`.  Then the JSM header from stack can be removed.
> 
> Or you can leave it like it is if you're not as bothered by the extra gunk.

I think I'll just leave it.
Comment hidden (mozreview-request)

Comment 7

3 years ago
mozreview-review
Comment on attachment 8779037 [details]
Bug 1287910 - move devtools stack-related APIs to per-platform require;

https://reviewboard.mozilla.org/r/70086/#review68074

Thanks, this looks good to me!
Attachment #8779037 - Flags: review?(jryans) → review+
Assignee

Comment 8

3 years ago
It turns out I also had to add the path to the worker loader.
And, we can't just re-export Cu.callFunctionWithAsyncStack there either, because Cu
isn't defined; so for safety I'm going to define a simple shim function.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

3 years ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/782f590541b8
move devtools stack-related APIs to per-platform require; r=jryans

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/782f590541b8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.