Closed Bug 1167300 Opened 4 years ago Closed 4 years ago

Consolidate the performance tool directory

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

defect
Not set

Tracking

(firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(3 files)

We have some "shared" files all over the place, leftovers from when we still had an old profiler and timeline. Need to clean up all of this.
Blocks: perf-tool-v2
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #8609040 - Flags: review?(jsantell)
Thoughts on still having subdirs under performance? We still have many utils that can be classified as working upon marker or profiler data, and in the future, a protocol dir for actor fronts and leaving the current modules dir just utils.
I like subdirs. Helps be see what's going on in a folder and where to look for things.
Great -- I'm doing some moving of the actor/front stuff in bug 1161199 that's similar, so if we have markers, profiler, protocol/actors, utils or something, I think that'll be pretty solid
Comment on attachment 8609041 [details] [diff] [review]
Part 2: Allow ViewHelpers.L10N to work with multiple files

Review of attachment 8609041 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/widgets/ViewHelpers.jsm
@@ +387,5 @@
>  /**
> + * A helper for having the same interface as ViewHelpers.L10N, but for
> + * more than one file. Useful for abstracting l10n string locations.
> + */
> +ViewHelpers.MergedL10N = function(aStringBundleNames) {

Can L10N handle this if the first argument is an array, rather than a separate method? Would love for the L10N object to be in its own module (but no need for that now). If we can't/too much work/too weird to have the L10N handle the array case, how about naming it `MultiL10N` -- `Merged` seems strange in this context
Attachment #8609041 - Flags: review?(jsantell) → review+
Comment on attachment 8609042 [details] [diff] [review]
Part 3: Fix file references and use only a single loader for all files

Review of attachment 8609042 [details] [diff] [review]:
-----------------------------------------------------------------

Only thing I feel strongly about is lowercase promise. Not sure whether it makes me more upset than implicit scope injection via `Cu.import(url)`. Unless there's a good reason for using that, I'd really prefer a module with an explicit path, and be consistent with DOM Promise casing (I assume it can also be used as a constructor, right? `new promise()`? I honestly don't even know what implementation this is)

Also, I dig the markers/profiler/utils/whatever separation rather than putting everything into `modules`, but if it's too much of a PITA, we can do that later

::: browser/devtools/performance/modules/logic/actors.js
@@ +7,2 @@
>  
> +loader.lazyRequireGetter(this, "promise");

I strongly dislike lowercase `promise` -- It is based off of the `Promise` API, I don't know why we have it named like this, especially when `let { promise, resolve }  = Promise.defer()`, so we have cases where lowecase `promise` is an actual promise and other cases it's the constructor. Is this the DOM promise? or Promise.jsm? Either way, I hate `promise`, and with our 1000 promise implementations, I'd like this to be explicit (Promise.jsm path, or something), and lowercase `promise` needs to go away and never return

::: browser/devtools/performance/performance-controller.js
@@ +53,2 @@
>  
> +loader.lazyImporter(this, "SideMenuWidget",

Since toolkit/loader can handle full resource URIs and JSMs, does it make sense to use different kinds of import statements?

::: browser/devtools/performance/system.js
@@ +9,5 @@
>   */
>  
> +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> +const { devtools: loader } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +const require = loader.require;

If this is used as the "header" of the script tags, this should probably be moved back to PerformanceController, because ideally we'd get rid of this once e10s is everywhere (but I don't feel strongly)
Attachment #8609042 - Flags: review?(jsantell) → review+
Comment on attachment 8609040 [details] [diff] [review]
Part 1: move files, combine global.js

Review of attachment 8609040 [details] [diff] [review]:
-----------------------------------------------------------------

I never understood the name "global" -- I think it made less and less sense as complexity everywhere grew, and it's kinda something different now. My dream world would have L10N file (super small), a markers definition file (since that has grown a lot in only a few days) and profiler categories. More files more files! But again, this is something we can do later if it's a PITA now.
Attachment #8609040 - Flags: review?(jsantell) → review+
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #8)
> ::: browser/devtools/performance/modules/logic/actors.js
> @@ +7,2 @@
> >  
> > +loader.lazyRequireGetter(this, "promise");
> 
> I strongly dislike lowercase `promise` -- It is based off of the `Promise`
> API, I don't know why we have it named like this, especially when `let {
> promise, resolve }  = Promise.defer()`, so we have cases where lowecase
> `promise` is an actual promise and other cases it's the constructor. Is this
> the DOM promise? or Promise.jsm? Either way, I hate `promise`, and with our
> 1000 promise implementations, I'd like this to be explicit (Promise.jsm
> path, or something), and lowercase `promise` needs to go away and never
> return
> 

I'm very much with you here. I always hated lowercase promise, but here's a bit of history:

Circa 2012: We started using a sync promise implementation about 3 years ago. It was uppercase `Promise`. We were all jolly about it and always talking about the meaning of life and promises especially at workweeks.

Early 2013: Shortly after we realized that sync promises are really quite terrible, and after much discourse and debacle, we decided to use an async lib. It was also uppercase `Promise`. However, we also knew that DOM promises are coming (also uppercase and async!), so the reasoning to use lowercase was simply to avoid conflicts and confusion (not sure if that really helped, because sheriffs started hating us).

Late 2013: As part of our initial efforts to move to an async promise implementation, we change all of our imports to use lowercase promise, and changed as much code as we could to use the new version. For a short while, we had both uppercase and lowercase (sync and async respectively) promises used throughout our code, even in the same file, which led us into terrible times with amazing almost-undebuggable test failures, but we thusly discovered narnia.

Early 2014: Almost all of our code is now using async promises. The future was looking good. They're all lowercase. However, Promise.jsm is a standard thing now, and it's async, but the only thing it does is import Promise-backend.js. 

Mid 2014: Brand new code being written started just importing Promise.jsm as uppercase, so we're back again to a point where we use lowercase and uppercase promises interchangeably, however at least they're both async.

There's some discourse to be had now about DOM promises, but those are sufficiently different (constructor with callbacks vs. function returning object) that it really doesn't matter if we use lowercase or uppercase.

AFAIK, the "standard" is still to either
* import "promise" (lowercase, async) and use it in devtools, since late 2013 or
* use DOM promises (uppercase, async).

We/I've been very relaxed when it comes to reviewing this stuff, because it doesn't really matter, but if you compare things, a very very large protion of devtools uses lowercase promise via require, or uppercase DOM promises.

This `devtools.lazyRequireGetter(this, "promise")` was supposed to be the new standard way we import promises where we can't use (or shouldn't yet use) DOM promises. All it does is import Promise.jsm, and it's not implicit scoping at all, because of the first argument.

I realize that lowercase promise is disgusting. We're on the same page here, and I've been having gastrointestinal discomfort since 2013 when we started using them. However, it's all because DOM promises are also uppercase.

tl;dr: lowercase promise always when importing; uppercase DOM promises as much as we can; the future is all about using DOM promises.

> ::: browser/devtools/performance/performance-controller.js
> @@ +53,2 @@
> >  
> > +loader.lazyImporter(this, "SideMenuWidget",
> 
> Since toolkit/loader can handle full resource URIs and JSMs, does it make
> sense to use different kinds of import statements?
> 

For lazy imports, there's a distinction now. Only vanilla `require` is smart enough to do the right thing. Probably not in the scope of this bug to make that better.
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/913b9761539b
https://hg.mozilla.org/mozilla-central/rev/b7b3a033fc2f
https://hg.mozilla.org/mozilla-central/rev/c8f0eb51c620
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Flags: qe-verify-
Comment on attachment 8609040 [details] [diff] [review]
Part 1: move files, combine global.js


Approval Request Comment
[Feature/regressing bug #]: 1167252, the new performance tool
[User impact if declined]: Won't ship the performance tool
[Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift
[Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake.  Risks are generally contained within devtools, specifically within the performance panel.
[String/UUID change made/needed]: None
Attachment #8609040 - Flags: approval-mozilla-aurora?
Comment on attachment 8609041 [details] [diff] [review]
Part 2: Allow ViewHelpers.L10N to work with multiple files


Approval Request Comment
[Feature/regressing bug #]: 1167252, the new performance tool
[User impact if declined]: Won't ship the performance tool
[Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift
[Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake.  Risks are generally contained within devtools, specifically within the performance panel.
[String/UUID change made/needed]: None
Attachment #8609041 - Flags: approval-mozilla-aurora?
Comment on attachment 8609042 [details] [diff] [review]
Part 3: Fix file references and use only a single loader for all files


Approval Request Comment
[Feature/regressing bug #]: 1167252, the new performance tool
[User impact if declined]: Won't ship the performance tool
[Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift
[Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake.  Risks are generally contained within devtools, specifically within the performance panel.
[String/UUID change made/needed]: None
Attachment #8609042 - Flags: approval-mozilla-aurora?
Note: I had verbal confirmation for these uplifts from Sylvestre even before he's flagged them as a+.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1167252#c26
Comment on attachment 8609042 [details] [diff] [review]
Part 3: Fix file references and use only a single loader for all files

Change approved to skip one train as part of the spring campaign.
Attachment #8609042 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8609040 [details] [diff] [review]
Part 1: move files, combine global.js

Change approved to skip one train as part of the spring campaign.
Attachment #8609040 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8609041 [details] [diff] [review]
Part 2: Allow ViewHelpers.L10N to work with multiple files

Change approved to skip one train as part of the spring campaign.
Attachment #8609041 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.