Closed Bug 1325401 Opened 3 years ago Closed 3 years ago
Experiment using a bundle for Reps
107.76 KB, patch
|Details | Diff | Splinter Review|
59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
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.
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+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/35986e7aa356 Start using Reps bundle behind a flag. r=ttromey
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
Assignee: chevobbe.nicolas → jlaster
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/8091269b852c Start using Reps bundle behind a flag. r=bgrins
Backed out for breaking ESLint in the same manner as shown in the last Try push. https://hg.mozilla.org/integration/autoland/rev/f03d25dd8af07454613c00d7aca1e11a04cb81b9
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.
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.
Some strange build failures on Linux. Retriggered builds : https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2d96793dff8a00de652506cfac7a2e8068a9d81
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 firstname.lastname@example.org: 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
You need to log in before you can comment on or make changes to this bug.