Closed Bug 1325401 Opened 3 years ago Closed 3 years ago

Experiment using a bundle for Reps

Categories

(DevTools :: Console, defect, P2)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: nchevobbe, Assigned: jlast)

References

Details

Attachments

(3 files, 6 obsolete files)

Since we're running an experiment having Reps in Github (https://github.com/devtools-html/devtools-reps), it would be nice to have the ability to require the webpack bundle generated there instead of the Rep components in m-c.

As a first step, we can have a user preference to tell the browser whether we want to load Reps from the bundle or from the regular path.
Along with that, we can have a reps-bundle.js stub file in m-c, which then will be replaced by the real module when launching the script to run mochitests in the Github project.

Once we're confident about the bundle, we could then remove the stub and use the real bundle in m-c, and always load from the bundle in the console.
Attached patch bundle-reps.patch (obsolete) — Splinter Review
Attachment #8825071 - Flags: review?(ttromey)
This patch adds a reps.js and reps.css bundle file to tree along with a new pref use-reps-bundle, which will let us try the bundle in tree.

It also sets us up well to update the reps module while we work on it in github.
Comment on attachment 8825071 [details] [diff] [review]
bundle-reps.patch

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

Thank you.  This looks good and is fine with one nit.
To be clear, I didn't actually read the reps.js file, but I assume it's fine.

::: devtools/client/preferences/devtools.js
@@ +323,5 @@
>  #else
>  pref("devtools.webconsole.new-frontend-enabled", false);
>  #endif
>  
> +pref("devtools.webconsole.use-reps-bundle", false);

I think this should have a comment explaining its purpose.  That seems to be the standard in this file.
Attachment #8825071 - Flags: review?(ttromey) → review+
Attached patch bundle-reps1.patch (obsolete) — Splinter Review
Attachment #8825071 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35986e7aa356
Start using Reps bundle behind a flag. r=ttromey
Keywords: checkin-needed
Backed out for unknown -webkit-scrollbar in reps.css:

https://hg.mozilla.org/integration/mozilla-inbound/rev/abb4c9666456dcbb7a05eb7093e73ba8098afb06

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=22fa3b948119d788d50a02394ab9e3624e8505ef
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=67424819&repo=mozilla-inbound

15:03:22     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_parsable_css.js | Got error message for jar:file:///builds/slave/test/build/application/firefox/browser/omni.ja!/chrome/devtools/modules/devtools/client/shared/components/reps.css: Unknown pseudo-class or pseudo-element ‘-webkit-scrollbar’.  Ruleset ignored due to bad selector. - 
15:03:22     INFO - Stack trace:
15:03:22     INFO -     chrome://mochitests/content/browser/browser/base/content/test/general/browser_parsable_css.js:messageIsCSSError:185
15:03:22     INFO -     chrome://mochitests/content/browser/browser/base/content/test/general/browser_parsable_css.js:checkAllTheCSS:343
15:03:22     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:737:9
15:03:22     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:657:7
15:03:22     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59
Flags: needinfo?(jlaster)
Assignee: chevobbe.nicolas → jlaster
Flags: needinfo?(jlaster)
Attached patch bundle-reps2.patch (obsolete) — Splinter Review
Attachment #8825174 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8091269b852c
Start using Reps bundle behind a flag. r=bgrins
Keywords: checkin-needed
Backed out for breaking ESLint in the same manner as shown in the last Try push.
https://hg.mozilla.org/integration/autoland/rev/f03d25dd8af07454613c00d7aca1e11a04cb81b9
Attachment #8825869 - Attachment is obsolete: true
New try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=8db7e3c54b5af25fc58318349741efb983eafa23

The reps bundle used here was built by using a modified version of devtools-launchpad, see issue on github: https://github.com/devtools-html/devtools-core/issues/91
Updated the patch to remove the reps.css from the whitelist of browser_parsable_css.js. The version we have right now is not triggering any warning and as such doesn't need to be ignored. Except for this, try was green, so I propose to move forward with this.

Jason: how did you build the reps.css you had in your previous patch? It looked quite different from the one in the devtools-reps source.
Flags: needinfo?(jlaster)
The second changeset is splitting the devtools/client/shared/components/test/mochitest tests into reps/ and (existing) mochitest/ folders. 

All the tests for reps now live in their dedicated folder which will make synchronisation to/from github easier.
Comment on attachment 8827469 [details]
Bug 1325401 - extract reps chrome mochitests to a dedicated folder;

https://reviewboard.mozilla.org/r/105142/#review105974

Looks good!
Attachment #8827469 - Flags: review?(jlaster) → review+
Comment on attachment 8827113 [details]
Bug 1325401 - Add a reps bundle, disabled by default;

https://reviewboard.mozilla.org/r/104938/#review105976

Looks good. Impressive we get all of the call sites covered.
Attachment #8827113 - Flags: review?(jlaster) → review+
Thanks for the reviews, let's just wait for the last try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0d383ca01c2f5e67a555a88cb4ee3cfe2818832

Clearing ni? as we clarified this on IRC.
Flags: needinfo?(jlaster)
Comment on attachment 8827627 [details]
Bug 1325401 - Export all reps from rep.js without createFactories wrappers;

https://reviewboard.mozilla.org/r/105242/#review106194
Attachment #8827627 - Flags: review?(jlaster) → review+
Comment on attachment 8827638 [details]
Bug 1325401 - update reps bundle;

https://reviewboard.mozilla.org/r/105250/#review106196
Attachment #8827638 - Flags: review?(jlaster) → review+
Attachment #8827627 - Attachment is obsolete: true
Attachment #8827638 - Attachment is obsolete: true
Thanks for the reviews! This should hopefully be the last version for this patch.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63683d535cdf06aefd29c614d7905371fc36fd5c
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/50aca7a3d253
Add a reps bundle, disabled by default;r=jlast
https://hg.mozilla.org/integration/autoland/rev/03792b9efb64
extract reps chrome mochitests to a dedicated folder;r=jlast
Comment on attachment 8827960 [details]
Bug 1325401 - use load-reps in netmonitor;

https://reviewboard.mozilla.org/r/105528/#review106338
Attachment #8827960 - Flags: review?(jlaster) → review+
Attachment #8827960 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/50aca7a3d253
https://hg.mozilla.org/mozilla-central/rev/03792b9efb64
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.