Closed Bug 1247270 Opened 6 years ago Closed 6 years ago

The reload addon doesn't reload JSON-viewer tabs

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch v1 (obsolete) — Splinter Review
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?
Assignee: nobody → poirot.alex
Depends on: 1245462
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.
Attachment #8717949 - Flags: review?(jryans)
s/first attachment/second attachment.
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.
Attachment #8717949 - Flags: review?(jryans) → feedback+
Blocks: 1241050
Attached patch patch v2Splinter Review
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!!!
Attachment #8718578 - Flags: review?(jryans)
Attachment #8717949 - Attachment is obsolete: true
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.
Attachment #8718578 - Flags: review?(jryans) → review+
(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.
https://hg.mozilla.org/integration/fx-team/rev/4db039bf7d4e2fca6981ff414e87c96fa7d2e1ec
Bug 1247270 - Ensure reloading every devtools-related documents when hitting the reload shortcut. r=jryans
https://hg.mozilla.org/mozilla-central/rev/4db039bf7d4e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.