Alexandre Poirot [:ochameau] Assignee
	Description • 9 years ago

When contribution to devtools/client/json-viewer
and hitting the reload shortcut, nothing happens.
At least visually, the devtools actually reloads but you have to reload the tab to see your changes.
Ideally, the document would just reload to see the changes.
At least we should have a notification somehow that it got reloaded to then manually reload the tab.

	Alexandre Poirot [:ochameau] Assignee
	Comment 1 • 9 years ago

This patch moves the reload logic to the addon.
I don't think this code was very well hosted in Loader.jsm.
This has nothing to do with the loader and is more about more highlevel devtools tricks.
May be that's something to be put in devtools-browser?

	Alexandre Poirot [:ochameau] Assignee
	Comment 2 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cc4f6c32ef2

	Alexandre Poirot [:ochameau] Assignee
	Comment 3 • 9 years ago

Comment on attachment 8717949 [details] [diff] [review]
patch v1

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

This patch feel like surgery, but it provides a good experience while using the addon.
I haven't had time to see how we could possibly convert gDevToolsBrowser
to use Addon APIs. But I imagine this could would (almost) go away if I manage to do that.

I'm piling up on top of that patch in bug 1241050, first attachment, to be able to reload gcli.

	Alexandre Poirot [:ochameau] Assignee
	Comment 4 • 9 years ago

s/first attachment/second attachment.

	Alexandre Poirot [:ochameau] Assignee
	Comment 5 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27512b949fba

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 6 • 9 years ago

Comment on attachment 8717949 [details] [diff] [review]
patch v1

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

Overall makes sense, but looks like still something to fix.

::: devtools/bootstrap.js
@@ +105,5 @@
> +          }
> +        }, false);
> +      }
> +    } else if (windowtype === "devtools:webide") {
> +      dump("reload window: "+windowtype+"\n");

Remove dump

@@ +111,5 @@
> +    } else if (windowtype === "devtools:webconsole") {
> +      // Browser console document can't just be reloaded.
> +      // HUDService is going to close it on unload.
> +      // Instead we have to manually toggle it.
> +      dump("reload browser console\n");

Remove dump

@@ +127,5 @@
> +    // Wait for a second before opening the toolbox to avoid races
> +    // between the old and the new one.
> +    let {setTimeout} = Cu.import("resource://gre/modules/Timer.jsm", {});
> +    setTimeout(() => {
> +      let { gBrowser } = window;

I get `window is not defined` here when I try this, and the toolbox never reopens.

	Alexandre Poirot [:ochameau] Assignee
	Comment 7 • 9 years ago

Hum... there was even more issues.
I ended up focusing on jsonview, about:debugging and gcli reload
and forgot to test simple toolbox contribution.

I wish I could write a test for this, but that looks even more challenging
than testing the browser toolbox :/
The main issue is that tests are run without the source tree available.
So. No reload addon. And no source to modify.
I could somehow ship the addon as support-files,
but what about the sources? It will break if the addon doesn't live within the whole sources.
There is so many things that would be so much easier if tests were run with the source tree!!!

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 8 • 9 years ago

Comment on attachment 8718578 [details] [diff] [review]
patch v2

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

Great, this version works well!  Unfortunately, I am not sure about testing it.

I also noticed that with the add-on applied, the debugger show no sources, and many errors in Browser Console:

TypeError: 'get window' called on an object that does not implement interface Window. react-dev.js:19569:0
ReferenceError: Task is not defined event-listeners.js:52:0
TypeError: DebuggerView is undefined
 workers-view.js:52:24
TypeError: DebuggerController is undefined
 variable-bubble-view.js:18:3
TypeError: DebuggerController is undefined
 watch-expressions-view.js:18:3
TypeError: DebuggerController is undefined
 global-search-view.js:18:3
TypeError: DebuggerController is undefined
 toolbar-view.js:19:3
TypeError: DebuggerView is undefined
 options-view.js:215:24
TypeError: DebuggerController is undefined
 stack-frames-view.js:18:3
TypeError: DebuggerView is undefined
 stack-frames-classic-view.js:140:39
TypeError: DebuggerController is undefined
 filter-view.js:19:3
TypeError: this._view is undefined

I think this is case from before this patch as well.  Is it a known issue?  devtools.loader.hotreload is set to false, but maybe it interferes with the base BrowserLoader?  The memory tool appears to work though, and also uses BrowserLoader, so maybe it's specific to debugger.

	Alexandre Poirot [:ochameau] Assignee
	Comment 9 • 9 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> Comment on attachment 8718578 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8718578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, this version works well!  Unfortunately, I am not sure about testing
> it.

You should see stuff being automatically reloaded *with* your change applied when hitting the magic key stroke. for example, if you modify webconsole.xul, you should see the browser toolbox being reloaded with your change. Now it isn't just the regular web toolbox that is reloaded, but any devtools related UI (web toolbox, browser console, json views, about:debugging, webide, developer toolbar)

> I also noticed that with the add-on applied, the debugger show no sources,
> and many errors in Browser Console:

Yes, it looks unrelated. I need to take a look at that in bug 1248609.

	Alexandre Poirot [:ochameau] Assignee
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/4db039bf7d4e2fca6981ff414e87c96fa7d2e1ec
Bug 1247270 - Ensure reloading every devtools-related documents when hitting the reload shortcut. r=jryans

	Carsten Book [:Tomcat]
	Comment 11 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4db039bf7d4e