Closed Bug 1261665 Opened 6 years ago Closed 6 years ago

Cannot reload Developer Tool with DevTools Reload

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: r_kato, Assigned: ochameau)

References

Details

(Whiteboard: [btpp-backlog])

Attachments

(3 files, 5 obsolete files)

Attached image 2016-04-03.png
Nightly: 48.0a1 (2016-04-02)
gecko-dev repository: 1bb567e
https://github.com/mozilla/gecko-dev/commit/1bb567e688ffbcb0939974b6717bd97dbecee319

Step to reproduce:
1. Load DevTools Reload's install.rdf
2. Hit F12 to open devtool, and hit Ctrl+Alt+R

Actual result:
Nothing happened.
My Browser Console says an error thrown (please see the attached file).
Summary: Cannot reload DevTools Reload → Cannot reload Developer Tool with DevTools Reload
Flags: needinfo?(sole)
Thanks for the ping - I will reroute to the developers working on it which will take good care of it :-)
Flags: needinfo?(sole) → needinfo?(poirot.alex)
Thanks for the report, I can repro. Something regressed recently.
Assignee: nobody → poirot.alex
Flags: needinfo?(poirot.alex)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch wip (obsolete) — Splinter Review
Work in progress. It looks like JSM have a very different behavior when used
in packaged builds. It seems to have changed. May be the addon manager flush
JSM/chrome caches more agressively.
From bootstrap.js, we no longer get access to the previous Loader.jsm instance
and always get access to the new one.
Priority: -- → P3
Whiteboard: [btpp-backlog]
My patchs depend on bug 1258546, which itself depends on bug 1258496.
If the platform patch from bug 1258496 doesn't move forward quickly, I'll try to reverse this dependency.
Depends on: 1258546
A followup from a previous cleanup. bug 1247203 comment 30.
It looks like main.js is more disturbing then really helpful.
It is especially disturbing in Loader.jsm. It is mostly used to defined
what is exported *for compatiblity reasons* out of Loader.jsm.
It is less disturbing for the SDK loader but still I think
it is more related to addon concept than our usage.
We couldn't really reload these main modules as-is.
There is a lot of stuff around them that needs to be set/unset.

What do you think? If you don't get a good feeling about this patch I'll just drop it and focus on what is actually really necessary.
Here the way to unload/reload the previously loaded module loader is what fixes the original issue behind this bug report.
(devtools-unload event)
Attachment #8739212 - Flags: review?(jryans)
Attachment #8738092 - Attachment is obsolete: true
This is another important fix after this regression.
If we try to use Loader.jsm in bootstrap.js, it will actually load
a brand new instance with the fresh sources reference by chrome.manifest of the addon.
It wasn't doing that when I introduced the addon...

So we shouldn't depend on any devtools module before doing the reload!
Attachment #8739214 - Flags: review?(jryans)
The browser console also needs to be closed and reloaded differently.
I think it would be better if HUDService closes it automatically when
the modules are unloaded, but it doesn't do that for now.

Anyway, I think it is still bootstrap.js responsibility to reopen it.
Attachment #8739215 - Flags: review?(jryans)
Comment on attachment 8739215 [details] [diff] [review]
fix browser console reload via the addon - v1

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

::: devtools/bootstrap.js
@@ +168,5 @@
>  
> +  // Browser console document can't just be reloaded.
> +  // HUDService is going to close it on unload.
> +  // Instead we have to manually toggle it.
> +  if (reopenBrowserConsole) {

Shouldn't this patch be merged with the previous one?  `reopenBrowserConsole` is introduced in the previous patch, and is not used anywhere there...
Attachment #8739215 - Flags: review?(jryans) → review+
Comment on attachment 8739212 [details] [diff] [review]
Remove unecessary Loader.jsm main in favor of require() - v1

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

I think this looks good overall, but who is using the "reload" data?

::: devtools/bootstrap.js
@@ +99,5 @@
>    }, false);
>  
> +  // As we can't get a reference to existing Loader.jsm instances, we send them
> +  // an observer service notification to unload them.
> +  Services.obs.notifyObservers(null, "devtools-unload", "reload");

Who is using this "reload" data?

::: devtools/server/content-server.jsm
@@ +19,5 @@
>    // in the same process.
>    let devtools = new DevToolsLoader();
>    devtools.invisibleToDebugger = true;
> +  let main = devtools.require("devtools/server/main");
> +  let { DebuggerServer, ActorPool } = main;

Might as well combine these lines?

::: devtools/shared/Loader.jsm
@@ +130,5 @@
>  var gNextLoaderID = 0;
>  
>  /**
>   * The main devtools API.
> + * The standard instance of this loader is

Nit: rewrap the remaining lines below
Attachment #8739212 - Flags: review?(jryans) → feedback+
Attachment #8739214 - Attachment is obsolete: true
Attachment #8739215 - Attachment is obsolete: true
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10)
> Comment on attachment 8739212 [details] [diff] [review]
> Remove unecessary Loader.jsm main in favor of require() - v1
> 
> Review of attachment 8739212 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this looks good overall, but who is using the "reload" data?
> 
> ::: devtools/bootstrap.js
> @@ +99,5 @@
> >    }, false);
> >  
> > +  // As we can't get a reference to existing Loader.jsm instances, we send them
> > +  // an observer service notification to unload them.
> > +  Services.obs.notifyObservers(null, "devtools-unload", "reload");
> 
> Who is using this "reload" data?

Loader.jsm: DevToolsLoader.prototype.observe which passes it to `provider.unload(data)`.
It is then passed to unload listeners, but they don't care about this argument.
(devtools.js and devtools-browser.js at end of files: `unload(function () {... })`);
So it is more to respect SDK module loader convention than being really useful.
Attachment #8740932 - Flags: review?(jryans)
Attachment #8739212 - Attachment is obsolete: true
Comment on attachment 8740932 [details] [diff] [review]
Remove unecessary Loader.jsm main in favor of require() - v2

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

::: devtools/shared/Loader.jsm
@@ +309,5 @@
>  this.devtools = this.loader = new DevToolsLoader();
>  
>  this.require = this.devtools.require.bind(this.devtools);
> +
> +// For compatiblity reasons, exposes these symbols on "devtools":

Nit: compatibility, expose
Attachment #8740932 - Flags: review?(jryans) → review+
Attachment #8740932 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/cd9c7f0216c4b19d79e607ca7ff04302c6375add
Bug 1261665 - Remove unecessary Loader.jsm main in favor of require(). r=jryans

https://hg.mozilla.org/integration/fx-team/rev/ef461673c13f81ff4bbb162f739227ab75844941
Bug 1261665 - Do not use devtools module in devtools addon before reloading. r=jryans
https://hg.mozilla.org/mozilla-central/rev/cd9c7f0216c4
https://hg.mozilla.org/mozilla-central/rev/ef461673c13f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.