Closed Bug 1287910 Opened 7 years ago Closed 7 years ago

replace components.stack

Categories

(DevTools :: Framework, enhancement, P1)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Iteration:
51.1 - Aug 15
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [reserve-html])

Attachments

(1 file)

Bug 1287910 - move devtools stack-related APIs to per-platform require;
58 bytes, text/x-review-board-request
jryans
: review+
Details 
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.
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: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
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)
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 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+
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.
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/782f590541b8
move devtools stack-related APIs to per-platform require; r=jryans
https://hg.mozilla.org/mozilla-central/rev/782f590541b8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.