Closed Bug 1444926 Opened 2 years ago Closed 2 years ago

Rename devtools/shim to devtools/startup

Categories

(DevTools :: General, enhancement, P3)

59 Branch
enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(2 files)

The name devtools/shim was chosen for DevTools gofaster, but since the addon plans are on hold, we could rename this folder to make it less confusing.

Since the folder contains all the startup code that can run before devtools are started or initialized, I think devtools/startup would be a good fit.

We should also cleanup all references to the devtools addon, and remove them.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8958129 [details]
Bug 1444926 - Move devtools/shim to devtools/startup;

https://reviewboard.mozilla.org/r/227074/#review234572

I guess it makes sense, thanks for this cleanup.

::: devtools/client/webconsole/webpack.config.js:92
(Diff revision 2)
>      "devtools/client/shared/vendor/redux": "redux",
>      "devtools/client/shared/vendor/reselect": "reselect",
>  
>      "devtools/shared/system": path.join(__dirname, "../../client/shared/webpack/shims/system-stub"),
>  
> -    "devtools/client/framework/devtools": path.join(__dirname, "../../client/shared/webpack/shims/framework-devtools-shim"),
> +    "devtools/client/framework/devtools": path.join(__dirname, "../../client/shared/webpack/shims/framework-devtools-startup"),

I imagine you should also rename:
https://searchfox.org/mozilla-central/source/devtools/client/shared/webpack/shims/framework-devtools-shim.js
And may be block 1436689? :)

::: devtools/shared/l10n.js:34
(Diff revision 2)
>  const reqShared = require.context("raw!devtools/shared/locales/",
>                                    true, /^.*\.properties$/);
>  const reqClient = require.context("raw!devtools/client/locales/",
>                                    true, /^.*\.properties$/);
> -const reqShim = require.context("raw!devtools/shim/locales/",
> +const reqShim = require.context("raw!devtools/startup/locales/",
>                                    true, /^.*\.properties$/);

You may also want to rename `reqShim`.
Attachment #8958129 - Flags: review?(poirot.alex) → review+
Comment on attachment 8958130 [details]
Bug 1444926 - Remove mentions of DevTools addon or DevTools moving to GitHub;

https://reviewboard.mozilla.org/r/227076/#review234576

Should we also remove the "addon" mode from moz.build?
https://searchfox.org/mozilla-central/source/devtools/moz.build#7,15,24

::: devtools/client/framework/devtools.js
(Diff revision 3)
> -    // devtools are reloaded via the add-on contribution workflow.
> -    if (!shuttingDown) {
> -      for (let [, toolbox] of this._toolboxes) {
> -        toolbox.destroy();
> -      }
> -    }

I would keep this shuttingDown parameter
as it is still invoked from here:
https://searchfox.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#185

Or remove the whole unload feature in a dedicated changeset. As it involves a lot more code.

But note that there is opportunities to use this code in bgrins work to hot reload firefox.
You would need to call:
  Services.obs.notifyObservers(cancelQuit, "devtools-reload", null);
to get devtools to reload its modules and trigger all this `shuttingDown=false` codepath.
Attachment #8958130 - Flags: review?(poirot.alex) → review+
Thanks for the reviews! 

> Should we also remove the "addon" mode from moz.build?
> https://searchfox.org/mozilla-central/source/devtools/moz.build#7,15,24

Good point, removed.

> I would keep this shuttingDown parameter

Fine by me, reverted.

> You may also want to rename `reqShim`.

Done, thanks!

> I imagine you should also rename:
> https://searchfox.org/mozilla-central/source/devtools/client/shared/webpack/shims/framework-devtools-shim.js

Ah good catch! That's the other way around, this one should still be called shim :) 

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ec36075b60fd9489592a636529db1ec7b7b667a
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s ea141f4684df08f61396e31ca4dad096b5c8e44b -d 20e9096156b0: rebasing 453079:ea141f4684df "Bug 1444926 - Move devtools/shim to devtools/startup;r=ochameau"
merging browser/locales/Makefile.in
merging devtools/client/themes/fonts.css
merging devtools/client/webconsole/webpack.config.js
warning: conflicts while merging devtools/client/themes/fonts.css! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5953d908463e
Move devtools/shim to devtools/startup;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/1565b5c867a4
Remove mentions of DevTools addon or DevTools moving to GitHub;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/5953d908463e
https://hg.mozilla.org/mozilla-central/rev/1565b5c867a4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b17690b7a406
Port bug 1444926 to TB/IB/SM: Fix package manifests due to move from devtools/shim to devtools/startup. rs=bustage-fix
Next time you decide to move around these many files, it would be a good idea to CC/ask someone from l10n. 

Unless we move them around, we're basically asking localizers to retranslate everything from scratch.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/008a5e87d53b
Port bug 1444926 to TB/SM: fix locales/Makefile.in etc. due to move from devtools/shim to devtools/startup. rs=bustage-fix
(In reply to Francesco Lodolo [:flod] from comment #20)
> Next time you decide to move around these many files, it would be a good
> idea to CC/ask someone from l10n. 
> 
> Unless we move them around, we're basically asking localizers to retranslate
> everything from scratch.

Sorry about that, if you want we can revert the change and do something to move files for localization teams. 
Are there some ongoing efforts to catch this kind of issues in CI? The process is frustrating on both ends at the moment.
Flags: needinfo?(francesco.lodolo)
We currently have a single repository for all versions of Firefox, which means we need to copy devtools/shim to devtools/startup (not move).

That's not a blocker, we just need to coordinate. Strings from mozilla-central are not exposed directly to tools, there's a buffer of a few days. So, I can prepare scripts to move things around in all >100 repositories before that happens.

Unfortunately this change broke our cross-channel generation, and I need to check with Axel about it. I don't think a back-out it's going to help, because the original changeset remains in the history.

No need to backout for now, I'll get back later if that's the case.
Flags: needinfo?(francesco.lodolo)
Blocks: 1447641
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.