Closed Bug 1204812 Opened 9 years ago Closed 9 years ago

Consider leaving Console.jsm behind in /toolkit

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: jryans, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

Console.jsm is used in many places outside DevTools, so it may make more sense to leave it back in toolkit.

If we want to do that, it's probably simplest to put it in /toolkit/modules (no reason for a DevTools folder with one file) and just install it to its previous path.
I agree!
This is quite messy as we just moved it,
but better move it now, in the same cycle (Fx44).
I moved the shims to toolkit/devtools/
And moved Console.jsm to toolkit/modules/

I could have kept the shims in /devtools/shared/shims/
and also kept/move Console.jsm back to toolkit/devtools/.

But hopefully, after some deprecation time, we could drop the shims and the toolkit/devtools/ folder completely.
Assignee: nobody → poirot.alex
Comment on attachment 8671352 [details] [diff] [review]
Move Console.jsm to toolkit/modules/

What do you think about my various choices?
Attachment #8671352 - Flags: feedback?(jryans)
Comment on attachment 8671352 [details] [diff] [review]
Move Console.jsm to toolkit/modules/

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

* Moving the real file to toolkit/modules makes sense to me
* You also rewrote the path to match other toolkit modules, also makes sense (even though it's now a 3rd path for this module)
* I might have put the shim in toolkit/shims

Overall, seems good.

::: toolkit/moz.build
@@ +6,5 @@
>  
>  DIRS += [
>      'components',
>      'content',
> +    'devtools',

Maybe we could call this "shims" here?

It feels a little odd to revive toolkit/devtools for one compatibility file.
Attachment #8671352 - Flags: feedback?(jryans) → feedback+
Moved the shim from /toolkit/devtools/shims/ to /toolkit/modules/shims/
as it looks weird to introduce a new folder in /toolkit for just one js file
and it feels logic to put it in modules.

Mossop, Are you ok in moving devtool's Console.jsm to toolkit/modules/?
This module is used by a lot of code in /browser, but also in /services
and addons. This module doesn't have a strong dependency on devtools,
it is more dependent to dom/ console written in C++ than code from /devtools.
Attachment #8671352 - Attachment is obsolete: true
Attachment #8672646 - Flags: review?(dtownsend)
Comment on attachment 8672646 [details] [diff] [review]
Move Console.jsm in toolkit/modules/ v2

Yes, moving Console.jsm to toolkit/modules is fine (I'd expect devtools to still be responsible for it in general though). I would rather leave the shim in devtools/shared/shims for now, no need to make a whole new directory just for that one file.

There are a bunch of places in tree that use the old path, are you going to update them here too?
Attachment #8672646 - Flags: review?(dtownsend) → feedback+
(In reply to Dave Townsend [:mossop] from comment #8)
> Comment on attachment 8672646 [details] [diff] [review]
> Move Console.jsm in toolkit/modules/ v2
> 
> Yes, moving Console.jsm to toolkit/modules is fine

Great!

> (I'd expect devtools to still be responsible for it in general though).

Sounds fine.

> I would rather leave the
> shim in devtools/shared/shims for now, no need to make a whole new directory
> just for that one file.

Done. I did that in the expectation that devtools would become a system addon,
but we are far from there yet.

> 
> There are a bunch of places in tree that use the old path, are you going to
> update them here too?

Yes, that the second patch attached to this bug ;)
Attachment #8672646 - Attachment is obsolete: true
Attachment #8673126 - Flags: review?(jryans)
Attachment #8673126 - Flags: review?(dtownsend)
Rebased.
Attachment #8671350 - Attachment is obsolete: true
Attachment #8673126 - Flags: review?(jryans) → review+
Attachment #8673126 - Flags: review?(dtownsend) → review+
Comment on attachment 8673127 [details] [diff] [review]
Rewrite the URLs v2

This is just a sed #devtools/shared/Console.jsm#Console.jsm# over the whole codebase.
Are you fine being the only reviewer for this?

Try looks good, I spawn some retry on some orange. There is some failure on videopuppeteer test suite, but I imagine that's not related to this patch.
Attachment #8673127 - Flags: review?(jryans)
Comment on attachment 8673127 [details] [diff] [review]
Rewrite the URLs v2

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

I believe it's fine for me to review this, since DevTools implicitly reviewed the same change I made when moving the files originally.  Also, this change is quite mechanical, since they should all be updated.
Attachment #8673127 - Flags: review?(jryans) → review+
Comment on attachment 8673126 [details] [diff] [review]
Move Console.jsm in toolkit/modules/ v3

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

::: devtools/shared/shims/moz.build
@@ +15,5 @@
>      'Loader.jsm',
>      'Simulator.jsm',
>  ]
> +
> +# Extra compability layer for transitional URL used in middle of fx44 cycle

Nit: compatibility
Attached patch landed patchSplinter Review
Merged the two previous patch with the nit addressed.
https://hg.mozilla.org/mozilla-central/rev/fcd050cd03e3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: