Closed Bug 1291049 Opened 8 years ago Closed 8 years ago

Attempt to bundle the inspector js into a webpack package

Categories

(DevTools :: Framework, enhancement)

Product:
DevTools
Component:
Framework
Version:
46 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED DUPLICATE of bug 1321509

People

(Reporter: bgrins, Unassigned)

References

Details

Attachments

(1 file, 27 obsolete files)

inspector-html-proto-screenshot.jpg
147.65 KB, image/jpeg
Details 
By trying to bundle our current js into a package, it should start to give a perspective on unknown work in track 3.  And then later once we can load inspector into an HTML doc we can check at runtime.
Blocking phase 2 meta for now so it doesn't get lost, but there's no reason this couldn't be done by anyone
Blocks: devtools-html-phase2
Whiteboard: [devtools-html]
Flags: qe-verify?
Priority: -- → P2
Really rough first pass. This should produce a bundle:

  cd devtools/client
  npm run build
  npm install
  webpack

Note I've put anything that was throwing build errors as 'externals', and those should ultimately be removed.  No way really to run this yet, but hopefully the build errors will help track down issues.
https://reviewboard.mozilla.org/r/69378/#review66714

A few notes from reading through the patch.  Thanks for doing this.

::: devtools/client/shared/shim/promise.js:11
(Diff revision 1)
> + * Promise.jsm is mostly the Promise web API with a `defer` method. Just drop this in here,
> + * and use the native web API (although building with webpack/babel, it may replace this
> + * with it's own version if we want to target environments that do not have `Promise`.
> + */
> +Promise.defer = function defer() {
> +  var resolve, reject;

If this is needed, could you file a track 3 bug?  We tried to remove all Promise.defer uses in bug 1273941.  If we missed some, we want to clean them up.

::: devtools/client/webpack.config.js:4
(Diff revision 1)
> +
> +"use strict";
> +
> +/*

Something in here (I don't know what) should make sure we pull in the raw loader.  See https://github.com/webpack/raw-loader and bug 1287915

::: devtools/client/webpack.config.js:39
(Diff revision 1)
> +  resolve: {
> +    alias: {
> +      promise: path.join(__dirname, "./shared/shim/promise"),
> +      devtools: path.join(__dirname, "../"),
> +      Services: path.join(__dirname, "./shared/shim/Services"),
> +

"promise" in theory can be a dummy module that just resolves to Promise.  At least that was the plan with the Promise.defer bug.

There are a few globals that must be provided, basically everything from builtin-modules. The main odd ones, I think, are |isWorker| and |reportError|.

Bug 1248830 describes all the loader requirements that we know of.

::: devtools/shared/event-emitter.js:5
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -"use strict";
> +/**

If this file needs changes, then it should also have a bug; since aside from the components.stack thing (bug 1287910), I think this should all be ok.
(In reply to Tom Tromey :tromey from comment #4)
> https://reviewboard.mozilla.org/r/69378/#review66714
> 
> A few notes from reading through the patch.  Thanks for doing this.
> 
> ::: devtools/client/shared/shim/promise.js:11
> (Diff revision 1)
> > + * Promise.jsm is mostly the Promise web API with a `defer` method. Just drop this in here,
> > + * and use the native web API (although building with webpack/babel, it may replace this
> > + * with it's own version if we want to target environments that do not have `Promise`.
> > + */
> > +Promise.defer = function defer() {
> > +  var resolve, reject;
> 
> If this is needed, could you file a track 3 bug?  We tried to remove all
> Promise.defer uses in bug 1273941.  If we missed some, we want to clean them
> up.

Ah, alright.  I removed the and just made promise an external that returns Promise 
> 
> ::: devtools/client/webpack.config.js:4
> (Diff revision 1)
> > +
> > +"use strict";
> > +
> > +/*
> 
> Something in here (I don't know what) should make sure we pull in the raw
> loader.  See https://github.com/webpack/raw-loader and bug 1287915

I'll add it as a dependency.  I'm not sure if anything else needs to be done.

> ::: devtools/client/webpack.config.js:39
> (Diff revision 1)
> > +  resolve: {
> > +    alias: {
> > +      promise: path.join(__dirname, "./shared/shim/promise"),
> > +      devtools: path.join(__dirname, "../"),
> > +      Services: path.join(__dirname, "./shared/shim/Services"),
> > +
> 
> "promise" in theory can be a dummy module that just resolves to Promise.  At
> least that was the plan with the Promise.defer bug.
> 
> There are a few globals that must be provided, basically everything from
> builtin-modules. The main odd ones, I think, are |isWorker| and
> |reportError|.
> 
> Bug 1248830 describes all the loader requirements that we know of.
> 

OK, defined those using a webpack.DefinePlugin

> ::: devtools/shared/event-emitter.js:5
> (Diff revision 1)
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > -"use strict";
> > +/**
> 
> If this file needs changes, then it should also have a bug; since aside from
> the components.stack thing (bug 1287910), I think this should all be ok.

OK, for some reason when that was unchanged webpack started trying to bundle in all sorts of files (including server and test files).  Needs some more investigation.
Comment on attachment 8777994 [details]
Bug 1291049 - Attempt to bundle the inspector with webpack

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69378/diff/1-2/
The error you posted on irc showed Services.appinfo not being defined.
The loader has to map require("Services") to the services shim file.
There may be other stuff mentioned in the loader bug, I forget now.
Flags: qe-verify? → qe-verify-
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
(In reply to Tom Tromey :tromey from comment #7)
> The error you posted on irc showed Services.appinfo not being defined.
> The loader has to map require("Services") to the services shim file.
> There may be other stuff mentioned in the loader bug, I forget now.

Thanks, I guess I was missing a .js at the end of the file in the config -- published an update.  But now it's attempting to compile the world (including files in test directories and server) and I'm running into issues bundling in codemirror paths which it shouldn't have to worry about since that is never require()d in the first place.  Will take a fresh look at it another time.
I made a branch and cherry-picked all my pending patches into it.
Then I applied this patch.  A few things I learned:

* The instructions in webpack.config.js didn't work as-is.  I had to npm install
  the various dependencies by hand first; then it worked.
* The inspector-panel.js change really is needed, since otherwise acorn barfs on
  that setter.
* The event-emitter.js change really is needed to avoid a webpack error
* There are still more things from the loader bug that need to be addressed; and
  also the approach taken in bug 1287910 means we'll need one more loader hack.
* We should exclude the tests from the bundle - we discussed on irc and this
  seems to mean something is pretty wrong somewhere
This is probably going to be needed:

diff --git a/devtools/client/webpack.config.js b/devtools/client/webpack.config.js
index b2a2f40..d173509 100644
--- a/devtools/client/webpack.config.js
+++ b/devtools/client/webpack.config.js
@@ -67,6 +68,7 @@ module.exports = {
       "isWorker": JSON.stringify(false),
       "reportError": "console.error",
       "loader": "{ lazyRequireGetter: () => {} }",
+      "dump": "console.log",
     })
   ],
Depends on: 1293756
Depends on: 1293760
My patch is a mild pain to rebase (I did it on top of all my pending patches so
it requires re-doing the event-emitter.js change).  But just the webpack config
patch is useful, so I'm appending it here.

I can attach the codemirror change if you want.  It's a hack but maybe reasonable-ish.

I think it would be great to land your inspector-panel.js patch.
Actually I think it might make sense to just land the webpack config and work
on it in-tree.
Not sure what to do about event-emitter yet.


diff --git a/devtools/client/webpack.config.js b/devtools/client/webpack.config.js
index b2a2f40..4af7505 100644
--- a/devtools/client/webpack.config.js
+++ b/devtools/client/webpack.config.js
@@ -39,6 +39,7 @@ module.exports = {
   },
   resolve: {
     alias: {
+      "devtools/platform": path.join(__dirname, "../shared/platform/content"),
       devtools: path.join(__dirname, "../"),
       Services: path.join(__dirname, "./shared/shim/Services.js"),
 
@@ -67,7 +68,10 @@ module.exports = {
       "isWorker": JSON.stringify(false),
       "reportError": "console.error",
       "loader": "{ lazyRequireGetter: () => {} }",
-    })
+      "dump": "console.log",
+    }),
+
+    new webpack.ContextReplacementPlugin(/.*$/, /NEVER_MATCH^/),
   ],
 
   externals: [
> Not sure what to do about event-emitter yet.

The ContextReplacementPlugin bit means that this change is no longer needed.
Updated based on Tom's changes and general cleanup.  Note: I had to keep the changes to event-emitter.js because otherwise (a) it would try to use the components.utils condition (which I fixed with `|| typeof window != 'undefined'` in the initial condition) and then (b) once I fixed that I would get some 'module "." not found' error at runtime.

I think this needs a bit more work before requesting review / landing.  It shouldn't have a lot of bitrot in the meantime (I just ignoring all of codemirror for now instead of fixing the paths)
(In reply to (Unavailable until Aug 15) Brian Grinstead [:bgrins] from comment #15)
> Updated based on Tom's changes and general cleanup.  Note: I had to keep the
> changes to event-emitter.js because otherwise (a) it would try to use the
> components.utils condition 

Yeah, I see this at runtime.
This module is turning out to be a real pain!
(In reply to (Unavailable until Aug 15) Brian Grinstead [:bgrins] from comment #15)
> (b) once I fixed that I
> would get some 'module "." not found' error at runtime.

webpack changes the event-emitter.js call:

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/event-emitter.js#29

to

	    factory.call(this, !(function webpackMissingModule() { var e = new Error("Cannot find module \".\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()), exports, module, console);

I suppose basically trying to prevent a dynamic require or something along those lines.
Depends on: 1294187
Depends on: 1294190
I tried even more hackery to get things further along:

* I gave up and made a copy of event-emitter in the shim directory.
  However I still think we can make this work either by modifying the
  prologue or by work on the webpack config.

* I made the Services shim load the default prefs like this:

+/* eslint-disable camelcase */
+let sticky_pref = pref;
+
+let defaults = require("raw!devtools/client/shared/shim/devtools.js");
+/* eslint-disable no-eval */
+eval(defaults);

... and I copied devtools.js into the right spot.  And hacked it because it
turns out that some devtools prefs are defined in all.js.

* I made the sdk "work" by removing the dummy sdk module definitions and then:

+      sdk: path.join(__dirname, "../../addon-sdk/source/lib/sdk"),
+      method: path.join(__dirname, "../../addon-sdk/source/lib/method"),

But then I needed:

+      // From sdk.
+      "resource://gre/modules/Preferences.jsm": "{}",
+      "@loader/options": "{}",
+      "@loader/unload": "{}",

and, worse, this line makes the parser that webpack uses give a syntax error:

https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/deprecated/api-utils.js#125


After all this it still is having trouble in the sdk.
For instance it says that this redeclares setTimeout:

https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/lang/functional/concurrent.js#16

So maybe we need to rid ourselves of the sdk for real; or make our own copy of it and remove
the troublesome bits.

I'm going to leave this alone again for a bit and go back to some of the open bugs.
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29
I did some more experiments after applying my new patches.

I had to comment out the QueryInterface in devtools/client/framework/target.js.
And, I had to comment out the babel loader in the webpack config, to avoid having
babel mangle the function* at the end of view-helpers.js into something that didn't work.

The latest failure is in developer-toolbar.js, where loader.lazyImporter isn't defined.
Basically parts of the toolbar and framework need some changes to work.
No idea why that didn't overwrite the previous patch; and also maybe I needed to unhack
event-emitter.js.
In this version I added a loader to hack up event-emitter.js for us; and another to
deal with the prefs issue (see bug 1272098 for the Services shim use of this new loader).
As discussed in Bug 1294220, we need a special webpack configuration in order to load properties files used for localization.

The configuration should ensure :
- "devtools/locale/*.properties" maps to "devtools/client/locales/en-US/*.properties"
- "devtools-shared/locale/*.properties" maps to "devtools/shared/locales/en-US/*.properties"

I know very little about webpack, let me know if you need more details about this!
I found a bug in the prefs loader; I'll attach a new version shortly.
Also I found that the Services.prefs lazy loading isn't quite right, I'll 
file a new bug for that.
Depends on: 1299254
Iteration: 51.2 - Aug 29 → 51.3 - Sep 12
This next version supports the locale files.
I haven't tested it in chrome yet, since my branch for this has many hacks that prevent
it from actually working.
(In reply to Tom Tromey :tromey from comment #28)

> I haven't tested it in chrome yet, since my branch for this has many hacks
> that prevent
> it from actually working.

Naturally, it didn't work.
The only way I could get it working is to put the definition of require.context
into addon-sdk/source/lib/toolkit/loader.js.
Attached patch inspector-flip.patch (obsolete) — — Splinter Review 
WIP for switching to the bundled panel instead of the default one
Rebased and updated for some recent l10n.js changes.
Tom, just an update on trying to get this bundle to run.  I modified a version of debugger.html to attempt to load the bundle.  Instructions:

git clone https://github.com/bgrins/debugger.html
copy the bundled filed created from m-c into public/ in the clone
npm install
npm run firefox (you can also just start a local build with --start-debugger-server 6080)
npm start

Load localhost:8000 and select the correct tab from the web page.  Note that it's not working yet (I see lazy loader errors from the bundle).
With my hack patch in the bundle and those instructions I get this in the console:

CREATE INSPECTOR PANEL! undefined

I'm unsure whether this represents progress.
(In reply to Tom Tromey :tromey from comment #38)
> With my hack patch in the bundle and those instructions I get this in the
> console:
> 
> CREATE INSPECTOR PANEL! undefined
> 
> I'm unsure whether this represents progress.

Ha, I think it does. But I think we need to somehow export the InspectorPanel object from the webpack, possibly with: https://webpack.github.io/docs/configuration.html#output-library
Attachment #8777994 - Attachment is obsolete: true
Attachment #8787307 - Attachment is obsolete: true
Attached patch bundle-include-target.patch (obsolete) — — Splinter Review 
Some more progress towards getting the panel to open in my forked debugger.html repo (make sure to also update that repo before attempting to copy bundle over).

So, apply Tom's two patches plus this one, then:

cd devtools/client
webpack && cp inspector/inspector.bundle.js /pathto/other/repo/public/

And now after loading the page it's onto an error at runtime when requiring InspectorFront (fronts/inspector.js).

'TypeError: Cu is undefined'.  I have a feeling this file might require a lot of work to compile.  It's including devtools/shared/protocol.js and a number of SDK things which tend to create a ton of nested dependencies.
A few notes --

The setImmediate thing can be handled by having our loader export it by default.
Then if need be we can polyfill it in the webpack.

It's handy to use "webpack -d" (especially if you're going to check in the bundle)
since then the original file names are recorded in the resulting bundle.

I think we will have to break the shared->server dependencies and not include server
in the bundle.
Attached patch bundle-include-target.patch (obsolete) — — Splinter Review 
Removes now unnecessary setImmediate hack, set up devtools/server/main as an external.  Getting further along initialization now, hitting an error that looks like the Walker isn't getting event emitter functionality attached:

inspector.bundle.js:61754 Uncaught (in promise) TypeError: this._walker.on is not a function
    at Selection.setWalker (inspector.bundle.js:61754)
    at Selection (inspector.bundle.js:61698)
    at InspectorPanel.<anonymous> (inspector.bundle.js:168)
    at Generator.next (<anonymous>)
    at TaskImpl._run (inspector.bundle.js:5289)
Attachment #8787775 - Attachment is obsolete: true
(In reply to Brian Grinstead [:bgrins] from comment #42)

>     at Selection.setWalker (inspector.bundle.js:61754)
>     at Selection (inspector.bundle.js:61698)

BTW I have also noticed that even if I generate a source map, it isn't working,
either in the console or the debugger.
Attachment #8777994 - Attachment is obsolete: true
Attachment #8782647 - Attachment is obsolete: true
Attachment #8787674 - Attachment is obsolete: true
Attachment #8788928 - Attachment is obsolete: true
Attachment #8777994 - Attachment is obsolete: false
OK, progress!  If you apply the two patches and pull down the latest code from https://github.com/bgrins/debugger.html, then we get a sort-of working markup view.  Got past the issue in Comment 42 - was an issue on the 'fix' I had in place in the SDK object.js. There's instructions for running this in Comment 35 and Comment 40.  Hopefully we can streamline this once we have more features working.

I had to comment out a number of things to get past initialization.  Some of the known issues:

1) Errors when hovering things in markup view (need to pass in highlighterUtils somehow from the 'toolbox' or move it so that the inspector can create its own):
    inspector.bundle.js:35929 Uncaught TypeError: Cannot read property 'highlightNodeFront' of undefined
    at MarkupView._showBoxModel (inspector.bundle.js:35929)
    at MarkupView._onMouseMove (inspector.bundle.js:35728)_showBoxModel @ inspector.bundle.js:35929_onMouseMove @ inspector.bundle.js:35728
    inspector.bundle.js:35944 Uncaught TypeError: Cannot read property 'unhighlight' of undefined
    at MarkupView._hideBoxModel (inspector.bundle.js:35944)
    at MarkupView._onMouseOut (inspector.bundle.js:35909)
2) Not loading any of the style sheets in inspector or markup frames
3) A react error that I think is screwing up the sidebar: Uncaught (in promise) TypeError: Cannot read property 'firstChild' of undefined at Object.findComponentRoot (inspector.bundle.js:72141).  This is probably due to the likely wrong change I made from switching browserRequire to require.  Issue is that toolbox.browserRequire is undefined here.
4) No localization and only showing entity names.  This will be resolved by Bug 1294186
Depends on: 1294186
See Also: → 1297758
Iteration: 51.3 - Sep 19 → 52.1 - Oct 3
Depends on: 1304513
Iteration: 52.1 - Oct 3 → 52.2 - Oct 17
Attachment #8789054 - Attachment is obsolete: true
Attached patch bug1291049.inspector.html.proto (obsolete) — — Splinter Review 
Be warned this is rough and very hacky, but it proves the inspector can work when loaded in a content tab.

The patch here contains 2 changesets (one from Tom modified by Brian, already attached here) and one from me. You can just apply this patch on top of fx-team and it should be enough.

Now to get this to work:
#1 go to devtools/client/
  - npm install
  - webpack (creates the inspector bundle)
#2 go to devtools/client/inspector/inspector.html/
  - npm install 
  - npm start (will start the development server modified from debugger.html)
#3 on the Firefox instance you want to debug (let's call that Firefox A)
  - open gcli
  - "listen 6080"
#4 on another Firefox (Aurora or Nightly) (let's call that Firefox B)
  - open "localhost:8000"

From there, on Firefox B you should see a link for each tabs from Firefox A. Click on it and it should open the inspector running in a content tab. 

Still a lot of things to fix, but the core is working pretty decently.
Mandatory celebration screenshot.
Another detail, if after reloading the inspector, everything seems broken and you see "Error: could not find pref branch devtools.dump.emit" in the console, you can fix it by deleting the localstorage entry for localhost:8000 and then reload.

(This happens whenever you select another tab than the rule-view and then reload, an issue with the Services.prefs shim apparently)
Attachment #8798645 - Attachment is patch: true
Attachment #8799949 - Attachment is obsolete: true
Attachment #8799412 - Attachment is obsolete: true
Attachment #8799406 - Attachment is obsolete: true
Attachment #8799950 - Attachment is obsolete: true
Attachment #8799951 - Attachment is obsolete: true
Comment on attachment 8799952 [details]
Bug 1291049 - Workaround to avoid failure when loading preferences

Hi Tom! 

I am facing a small problem with the Services.prefs shim and I can't figure out if it's an implementation issue or not.

My STRs are:
- load the inspector in a content tab
- change the selected sidebar panel
- reload the inspector
-> load fails.

When I load the inspector for the first time, my local storage is empty, so in shim/Services.js::_initializeRoot, the default preferences are loaded.

After that when switching the sidebar panel, the preference for this panel gets written to local storage. 

When reloading the inspector, since I now have one preference in localStorage the default preferences are no longer loaded. But then I only have one preference available and a call to get another preference throws an error.

Either (1) some mechanism is supposed to write all preferences to local storage at some point (on unload maybe?) and doesn't get triggered here. Or (2) we should always merge the default preferences and the user preferences.

What do you think?
Flags: needinfo?(ttromey)
(In reply to Julian Descottes [:jdescottes] from comment #63)

> I am facing a small problem with the Services.prefs shim and I can't figure
> out if it's an implementation issue or not.

> Either (1) some mechanism is supposed to write all preferences to local
> storage at some point (on unload maybe?) and doesn't get triggered here. Or
> (2) we should always merge the default preferences and the user preferences.
> 
> What do you think?

I think probably #2.
I'll file a new bug and do the work there.
Flags: needinfo?(ttromey)
Depends on: 1309384
Attachment #8800186 - Attachment is obsolete: true
Attachment #8800187 - Attachment is obsolete: true
Attachment #8799952 - Attachment is obsolete: true
Attachment #8800188 - Attachment is obsolete: true
With the last patches uploaded here the inspector should work both in the firefox devtools as well as in a content tab. It even starts in Chrome (with many features not working at the moment).

The patches have been split as follows:
- first patch including the (huge) bundle from debugger html
- second patch with the development server (can still be cleaned up a bit)
- third patch with all the inspector modifications
Depends on: 1309650
Depends on: 1309733
Depends on: 1310211
Depends on: 1310279
Depends on: 1310310
Depends on: 1310337
Depends on: 1310416
Depends on: 1310417
Tom, I tried creating separate for all the blocking issues that could be addressed independently from this main bug.

The last item that might be worth tackling separately is what you had to change in l10n.js. Do you think that's something that could land on its own? If yes could you create a blocking bug for it? Thanks!
Flags: needinfo?(ttromey)
Depends on: 1310702
(In reply to Julian Descottes [:jdescottes] from comment #80)
> Tom, I tried creating separate for all the blocking issues that could be
> addressed independently from this main bug.
> 
> The last item that might be worth tackling separately is what you had to
> change in l10n.js. Do you think that's something that could land on its own?
> If yes could you create a blocking bug for it? Thanks!

I filed one.
Flags: needinfo?(ttromey)
Attachment #8798645 - Attachment is obsolete: true
Attachment #8777994 - Attachment is obsolete: true
Iteration: 52.2 - Oct 17 → 52.3 - Nov 7
Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c049e0e6cbad5672c83432a7a7b7db6b8fda37fd 

I tried to isolate all the hacks/workarounds so they have no impact when running in a Firefox panel, let's see what pops up on try.
Surprisingly, just a few test failures. This next try push should be green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dc39a47cb705cf3885b577429b50d143464b97c
The webpack config is going to need some minor tweaks once bug 1312041 lands.
Depends on: 1315679
Depends on: 1311541
Attachment #8801779 - Attachment is obsolete: true
Depends on: 1316577
Iteration: 52.3 - Nov 14 → 53.1 - Nov 28
Attachment #8800185 - Attachment is obsolete: true
Attachment #8801778 - Attachment is obsolete: true
Attachment #8811888 - Attachment is obsolete: true
PR opened on github for debugger.html changes : https://github.com/devtools-html/debugger.html/pull/1274
Assignee: bgrinstead → jdescottes
Attachment #8813251 - Attachment is obsolete: true
Attachment #8813378 - Attachment is obsolete: true
I submitted the last set of patches on try and it doesn't look like there are any new failures / intermittents here, which is encouraging!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1217e8271a9aab1af0a3e2f4e8b607bfabce094
As discussed in standup, the current patches should be compatible with the latest mc, and self contained. After pulling the patches:
- cd devtools/client/inspector
- npm install
- npm start

A server should normally start on port 8000. Then to use it
- listen to 6080 on a firefox instance (FF-1)
- open localhost:8000 on another firefox instance (FF-2)

In FF-2 you should be able to see the list of tabs in FF-1, and the inspector should open if you click on a tab from the list.
Doing the test locally, I am hitting this in localhost:8000:

ReferenceError: regeneratorRuntime is not defined[Learn More]bundle.js:152562:8  paused< http://localhost:8000/assets/build/bundle.js:152562:8
  <anonymous> http://localhost:8000/assets/build/bundle.js:152561:15
  __webpack_require__ http://localhost:8000/assets/build/bundle.js:20:12
  ...
Thanks for testing Brian! As discussed yesterday it looks like the parent package.json (in devtools/client) was forcing its babel preset es2015 on the inspector package.

I'm not sure why I didn't get the issue earlier but for now we moved devtools' package.json and webpack.config to client/sourceeditor. Hopefully the new set of patches should work without any other hack. Feedback appreciated!
:pbro offered to spend some time testing this! Let me know if the patches work for you.
Flags: needinfo?(pbrosset)
(In reply to Julian Descottes [:jdescottes] from comment #157)
> :pbro offered to spend some time testing this! Let me know if the patches
> work for you.
After working out through some Windows path separator regexp issues with Julian, I was able to test this locally. npm start, npm install all worked fine (it did require me to use node 7.2.0 though), and the inspector was displayed correctly in Firefox (I haven't tested in other browsers).

Changing a source file and then refreshing the page did work too!

Great work!
Flags: needinfo?(pbrosset)
Comment on attachment 8814045 [details]
Bug 1291049 - move client/package.json to sourceeditor;

https://reviewboard.mozilla.org/r/95364/#review96088
Attachment #8814045 - Flags: review?(bgrinstead) → review+
Let's get started with some review requests. I folded the devtools-local-toolbox integration and the inspector changes into a single patch because they don't really make sense separately. I could still extract the small one line workarounds if we want to discuss those separately but let's get started with this.
Iteration: 53.1 - Nov 28 → 53.2 - Dec 12
Attachment #8800183 - Attachment is obsolete: true
Attachment #8800183 - Flags: review?(pbrosset)
Comment on attachment 8800184 [details]
Bug 1291049 - use devtools-local-toolbox to load the inspector in content;

Including Matteo for the addon-sdk changes (pretty trivial but I believe they were needed to make webpack play nice)
Attachment #8800184 - Flags: review?(zer0)
Comment on attachment 8800184 [details]
Bug 1291049 - use devtools-local-toolbox to load the inspector in content;

https://reviewboard.mozilla.org/r/85172/#review96094

Re-tested this locally, and it works great

::: devtools/client/inspector/package.json:9
(Diff revision 20)
> +  "description": "The Firefox Inspector",
> +  "scripts": {
> +    "start": "node bin/dev-server"
> +  },
> +  "author": "",
> +  "license": "ISC",

This should be MPL, yes?  Or just omit it

::: devtools/client/framework/devtools.js:46
(Diff revision 22)
>  
>    AboutDevTools.register();
>  
>    EventEmitter.decorate(this);
>  
> +  if (Services.obs) {

Should we shim / sham this function instead either in the Services shim or somehow in webpack?  At the least this needs a comment.

::: devtools/client/inspector/toolbox.js:1
(Diff revision 22)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Don't really like the toolbox.js name since that's already used for the actual toolbox.  What about local-toolbox.js?

::: devtools/client/inspector/toolbox.js:33
(Diff revision 22)
> +    let inspector = new Inspector(fakeToolbox);
> +    inspector.init();
> +  });
> +}
> +
> +window.addEventListener("DOMContentLoaded", function onInspectorDOMLoaded() {

Can all of the logic from inspector-html-bootstrap.js be added here instead of in a separate script file that gets loaded on the page?  That would get rid of some build complexity and put most of this particular plumbing in one place.  But maybe I'm not understanding the relationship between this code and inspector.js -- is buildFakeToolbox called twice in the case when loading in the local toolbox, or only here?

I know inspector-html-bootstrap.js is also loaded in markup.xhtml but maybe we could querySelector for iframes and run the same logic here.  It seems like this file would translate fairly easy to future tools and it'd be great if most of the boilerplate logic here lived in one place.
Attachment #8800184 - Flags: review?(bgrinstead)
Comment on attachment 8809865 [details]
Bug 1291049 - allow non devtools prefs to be loaded in webpack prefs loader;

https://reviewboard.mozilla.org/r/92368/#review96874

Thank you.  This looks good.
Attachment #8809865 - Flags: review?(ttromey) → review+
Comment on attachment 8800184 [details]
Bug 1291049 - use devtools-local-toolbox to load the inspector in content;

re-flagging Matteo, since mozreview cleared it
Attachment #8800184 - Flags: review?(zer0)
Comment on attachment 8800184 [details]
Bug 1291049 - use devtools-local-toolbox to load the inspector in content;

https://reviewboard.mozilla.org/r/85172/#review96892

Thank you for doing this.

I was a bit nit-picky, on the theory that I'd like us to be sure that the things we land for this bug are in our long-term interests.

I didn't really understand the inspector.js changes at all.

FWIW I agree with Brian's comments.

::: devtools/client/framework/devtools.js:31
(Diff revision 22)
>  
>  /**
>   * DevTools is a class that represents a set of developer tools, it holds a
>   * set of tools and keeps track of open toolboxes in the browser.
>   */
> -this.DevTools = function DevTools() {
> +const DevTools = function DevTools() {

Seems like this could just be "function DevTools()..."

::: devtools/client/inspector/inspector-html-bootstrap.js:1
(Diff revision 22)
> +"use strict";

This should have the usual file header comment.

::: devtools/client/inspector/inspector-html-bootstrap.js:23
(Diff revision 22)
> +document.documentElement.setAttribute("class", "theme-light");
> +
> +var isMac = /Mac/.test(navigator.userAgent);
> +var isLinux = /(Linux)|(X11)/.test(navigator.userAgent);
> +if (isMac) {
> +  document.documentElement.setAttribute("platform", "mac");

I don't understand when this is needed; but perhaps this should be done some other way?  Services has a different method for platform detection that is used everywhere else; and the shim implements it too.

::: devtools/client/inspector/webpack.config.js:107
(Diff revision 22)
> +    {
> +      "promise": "var Promise",
> +      "devtools/server/main": "{}",
> +
> +      // Just trying to get build to work.  These should be removed eventually:
> +      "chrome": "{}",

It's fine to leave this here but I wonder whether all of these are still necessary.

::: devtools/client/inspector/webpack/editor-sham.js:9
(Diff revision 22)
> +Editor.modes = {};
> +function Editor() {}

Ordering of linse seems funny here.

::: devtools/client/shared/widgets/tooltip/TooltipToggle.js:80
(Diff revision 22)
> +    if (typeof toggleDelay === "undefined") {
> +      toggleDelay = DEFAULT_TOGGLE_DELAY;
> +    }

I'd like to understand the need for this change.

::: devtools/shared/DevToolsUtils.js:29
(Diff revision 22)
>  
>  /**
>   * Waits for the next tick in the event loop to execute a callback.
>   */
>  exports.executeSoon = function executeSoon(aFn) {
> -  if (isWorker) {
> +  // XXX: Move setImmmediate chrome implementation to loader

I think we probably just shouldn't be using DevToolsUtils.  I would suggest backing out this hunk and instead filing a bug for the setImmediate thing.
Attachment #8800184 - Flags: review?(ttromey) → review+
Removing blocker on the devtools CSS bug.

Thanks for the reviews so far! I will move the changesets to another bug. There are too many revisions in this one and it would be nice to be able to use the interdiff when addressing review comments.
No longer depends on: 1311541
Depends on: 1321509
Comment on attachment 8800184 [details]
Bug 1291049 - use devtools-local-toolbox to load the inspector in content;

https://reviewboard.mozilla.org/r/85172/#review96892

> This should have the usual file header comment.

This file is actually gone now. It's been merged with local-toolbox.js (which has the header).

> I don't understand when this is needed; but perhaps this should be done some other way?  Services has a different method for platform detection that is used everywhere else; and the shim implements it too.

This is needed to get the platform specific styles applied. Not sure how much it impacts the inspector, but this code is here to replace the missing theme-switching logic, which does the same thing (see http://searchfox.org/mozilla-central/source/devtools/client/shared/theme-switching.js#15-23 )
I updated it to reuse Services.appinfo though.

> Ordering of linse seems funny here.

Thanks! Fixed.

> I'd like to understand the need for this change.

This doesn't seem needed anymore but when I started working on this, this syntax was failing when loading the inspector on Firefox release (49 at the time?). Default values could not reference variables apparently. This seems to be ok now, so I removed it.

> I think we probably just shouldn't be using DevToolsUtils.  I would suggest backing out this hunk and instead filing a bug for the setImmediate thing.

In the interesting of landing this bug, I just added a shim for DevtoolsUtils as well that maps executeSoon to setImmediate.
Comment on attachment 8800184 [details]
Bug 1291049 - use devtools-local-toolbox to load the inspector in content;

https://reviewboard.mozilla.org/r/85172/#review96094

> This should be MPL, yes?  Or just omit it

I removed it, I don' think it matters unless we publish the package. Thanks!

> Should we shim / sham this function instead either in the Services shim or somehow in webpack?  At the least this needs a comment.

Done with a sham.

> Don't really like the toolbox.js name since that's already used for the actual toolbox.  What about local-toolbox.js?

Updated to local-toolbox. Maybe we should go further and have the common parts (waiting for DOMContentLoaded, fixing the stylesheets etc) in local-toolbox.js and use it in an inspector specific loader file (inspector-html-loader.js ?) which would just provide the inspector specific logic. But this is still a bit hacky at the moment so let's wait a bit.

> Can all of the logic from inspector-html-bootstrap.js be added here instead of in a separate script file that gets loaded on the page?  That would get rid of some build complexity and put most of this particular plumbing in one place.  But maybe I'm not understanding the relationship between this code and inspector.js -- is buildFakeToolbox called twice in the case when loading in the local toolbox, or only here?
> 
> I know inspector-html-bootstrap.js is also loaded in markup.xhtml but maybe we could querySelector for iframes and run the same logic here.  It seems like this file would translate fairly easy to future tools and it'd be great if most of the boilerplate logic here lived in one place.

As proposed I moved all the logic to the local-toolbox file. I am listening to mutations to pick new iframes being loaded. It seems to work fine at least for the markup view iframe. We could maybe add a whitelist of iframe URLs we want tot update to avoid side effects.
Comment on attachment 8800184 [details]
Bug 1291049 - use devtools-local-toolbox to load the inspector in content;

https://reviewboard.mozilla.org/r/85172/#review96892

Regarding the inspector.js changes, I am happy to go over them on vidyo if you want. The gist of it is that I isolated a "buildFakeToolbox" function that was already used when loading the inspector in a standalone chrome tab, and I export it in order to reuse that in our local-toolbox.js module (which initializes the inspector panel when we are in content).

Discussing with Brian yesterday, we should probably move this fake local toolbox to a separate file so that it's less messy.

Thanks for being nit-picky :) It is very important to minimize the impact of this change on the main/existing workflow.
Let's move the conversation to Bug 1321509 now.
Comment on attachment 8800184 [details]
Bug 1291049 - use devtools-local-toolbox to load the inspector in content;

Clearing reviews and marking attachments as obsolete, since this has moved to Bug 1321509
Attachment #8800184 - Attachment is obsolete: true
Attachment #8800184 - Flags: review?(zer0)
Attachment #8800184 - Flags: review?(jlaster)
Attachment #8800184 - Flags: review?(bgrinstead)
Attachment #8809865 - Attachment is obsolete: true
Attachment #8814045 - Attachment is obsolete: true
Iteration: 53.2 - Dec 12 → 53.3 - Dec 26
Bug 1321509 has landed, so we can close this one.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Assignee: jdescottes → nobody
No longer blocks: devtools-html-phase2
Iteration: 53.3 - Dec 26 → ---
Priority: P1 → --
Whiteboard: [devtools-html]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: