Closed
Bug 1369945
Opened 7 years ago
Closed 7 years ago
Make it possible to see both the Rules panel and the Layout panel at the same time
Categories
(DevTools :: Inspector, enhancement, P3)
DevTools
Inspector
Tracking
(firefox59 fixed)
RESOLVED
FIXED
Firefox 59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: jensimmons, Assigned: gl)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: DevAdvocacy, Whiteboard: [DevRel:P1][designer-tools])
Attachments
(2 files)
I love the new Layout panel. I use it a _lot_.
I also use the Rules panel a lot, rewriting CSS.
Having to click the tab to go back and forth between the Rules panel and the Layout panel adds friction to the workflow.
I would like to be able to do this:
https://monosnap.com/file/WJ9U0MJrai9lVrutgo0hrHiRCTxaG9.png
And put the Layout panel next to the Rules panel.
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Priority: -- → P3
Updated•7 years ago
|
Assignee: nobody → tigleym
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8887268 -
Flags: review?(pbrosset)
Attachment #8887268 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•7 years ago
|
Attachment #8887268 -
Flags: review?(pbrosset)
Attachment #8887268 -
Flags: review?(bgrinstead)
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8887268 [details]
Bug 1369945 - Part 1: Display a split rule view panel in the inspector.
https://reviewboard.mozilla.org/r/158064/#review164060
The code changes look pretty straight forward to me. But I have not played with the patch yet. Hard to find time for this now.
So R+ is based on me scanning the code. But before landing we need:
- a green try build
- a review from Brian
- and some manual testing from someone else than you, to check how this feels. This is only in nightly, so we don't need to be too strict, the idea is to let people play with it, but I'd like to make sure we avoid obvious bugs first if we can.
::: devtools/client/inspector/inspector.js:706
(Diff revision 1)
> this.fontinspector.init();
>
> this.sidebar.toggleTab(true, "fontinspector");
> }
>
> + if (Services.prefs.getBoolPref("devtools.inspector.split-rule-enabled")) {
You're accessing the value of this pref in 3 different places, I suggest doing this only once in the constructor and storing it on `this`.
::: devtools/client/inspector/rules/rules.js:1586
(Diff revision 1)
> isSidebarActive: function () {
> if (!this.view) {
> return false;
> }
> - return this.inspector.sidebar.getCurrentTabID() == "ruleview";
> +
> + return Services.prefs.getBoolPref("devtools.inspector.split-rule-enabled") ?
Same in this file in 2 places, you could then use `this.inspector.<something>` to retrieve the value of the pref.
Attachment #8887268 -
Flags: review?(pbrosset) → review+
Comment 3•7 years ago
|
||
I'm planning to look at this tomorrow
Comment 4•7 years ago
|
||
Usage note: when changing a rule in the rule view, the computed value doesn't get updated immediately (you have to switch to another panel like Layout and then back to Computed again). This was OK when only one could be visible at once, but I think it needs to be updated immediately if we are going to show them next to each other. Interestingly this doesn't appear to be a problem with the Rule View or Layout panel (changes from one are immediately seen in the other).
Comment 5•7 years ago
|
||
Usage note: the widths of the rule view and the other panels aren't remembered when closing and reopening the toolbox. The width of the markup view is remembered, but if you shrink the computed view way down then close/reopen then the rule and computed become split with each other 50/50.
Comment 6•7 years ago
|
||
Usage note: panels become stuck in vertical mode:
1) make the window small enough for the panels to become vertical
2) close toolbox
3) reopen toolbox
4) make the window big enough that they should become horizontal
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8887268 [details]
Bug 1369945 - Part 1: Display a split rule view panel in the inspector.
https://reviewboard.mozilla.org/r/158064/#review165464
I could definitely see using this view. Clearing review mostly based on the usage notes in comments 4-6.
For test changes it'd be best to run both tests if possible so that any breakage will be detected in CI. For example first do the non split test then close and reopen toolbox and do the split test, or make a new test file that does just the split test.
::: devtools/client/inspector/inspector.js:490
(Diff revision 1)
> +
> + let splitter = isSplitRuleViewEnabled ?
> + SplitBox({
> + className: "inspector-sidebar-splitter",
> + initialSize: INITIAL_SIDEBAR_SIZE,
> + minSize: "50%",
IMO this should be much smaller, like 10 or 20% so that the markup view can be expanded to ~90% as before
Attachment #8887268 -
Flags: review?(bgrinstead) → review-
Reporter | ||
Updated•7 years ago
|
Keywords: DevAdvocacy
Whiteboard: [devRel:P1]
Assignee | ||
Updated•7 years ago
|
Attachment #8887268 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8887268 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•7 years ago
|
Attachment #8887268 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8887268 [details]
Bug 1369945 - Part 1: Display a split rule view panel in the inspector.
https://reviewboard.mozilla.org/r/158064/#review191570
This is much better, thanks. A few comments for now
::: devtools/client/inspector/inspector.js:620
(Diff revision 4)
> +
> + onSidebarToggle: function () {
> + Services.prefs.setBoolPref(SPLIT_RULE_VIEW_PREF, !this.isSplitRuleViewEnabled);
> + },
> +
> + async onSplitRuleViewPrefChanged() {
There's a fair amount of duplicated logic or dependency spread between `setupSidebar`, `onSplitRuleViewChanged`, and `setupRuleViewSidebar`. As an example, `setupRuleViewSidebar` is called both on initialization and pref change, but code like `hideRuleViewSidebar` / `this.sidebar.addExistingTab` is duplicated in setupSidebar and here.
I'm worried that will lead to bugs in the future if a change is made in one place but not the other. I'd prefer if we had a single function like `setupSidebars` that could be called at any time, and would be used both during initialization and on pref change. I'm sure it would require either some guarding against calling addExistingTab when it's already added (same for removeTab when it's not there) - or making the sidebar APIs more resiliant to being called multiple times. What do you think?
::: devtools/client/inspector/inspector.js:647
(Diff revision 4)
> +
> + await this.sidebar.removeTab("ruleview");
> +
> + // If the prior sidebar tab was the rule view before being split, select
> + // the computed view.
> + if (currentTab === "ruleview") {
Seems like this logic could be encoded into the sidebar `removeTab` function. i.e. if the tab being removed was selected then select the next one (or first one if the selected tab was last).
::: devtools/client/inspector/inspector.js:703
(Diff revision 4)
> + hideTabstripe: true
> + });
> + }
> +
> + if (this.isSplitRuleViewEnabled) {
> + this.getPanel("ruleview");
What is the purpose of this line?
::: devtools/client/inspector/inspector.xhtml:72
(Diff revision 4)
> role="group" data-localization="aria-label=inspector.breadcrumbs.label" tabindex="0"></div>
> </div>
> </div>
>
> <!-- Splitter -->
> - <div xmlns="http://www.w3.org/1999/xhtml" id="inspector-splitter-box">
> + <div id="inspector-splitter-box"></div>
Not for this bug, but can we make inspector a plain HTML file now? I don't see any other elements with xmlns, we don't use dtd files, and I don't see a reference to the xul namespace in scripts in inspector/.
::: devtools/client/inspector/rules/rules.js:1626
(Diff revision 4)
> this.inspector.selection.on("detached-front", this.onSelected);
> this.inspector.selection.on("new-node-front", this.onSelected);
> this.inspector.selection.on("pseudoclass", this.refresh);
> this.inspector.target.on("navigate", this.clearUserProperties);
> +
> + if (this.inspector.isSplitRuleViewEnabled) {
Initialization is still only called once - won't this not bind to the event on the correct sidebar if the splitRuleView isn't enabled on initialization but is after the fact (and vice versa)?
Is there any harm to binding to both sidebars? Or emitting / listening to an event on the inspector object instead of needing to know which to listen to here.
Attachment #8887268 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•7 years ago
|
Assignee: tigleym → gl
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8887268 -
Flags: review?(bgrinstead)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8887268 [details]
Bug 1369945 - Part 1: Display a split rule view panel in the inspector.
https://reviewboard.mozilla.org/r/158064/#review194586
First pass with some comments
::: devtools/client/inspector/inspector.js:528
(Diff revision 8)
> - endPanel: this.InspectorTabPanel({
> + endPanel: this.InspectorTabPanel({
> - id: "inspector-sidebar-container"
> + id: "inspector-sidebar-container"
> - }),
> + }),
> - vert: this.useLandscapeMode(),
> + ref: splitbox => this.sidebarSplitBox = splitbox,
> + }),
> + vert: this.useLandscapeMode()
Nit: missing trailing comma
::: devtools/client/inspector/inspector.js:625
(Diff revision 8)
> +
> + async onSplitRuleViewPrefChanged() {
> + // Update the stored value of the split rule view preference since it changed.
> + this.isSplitRuleViewEnabled = Services.prefs.getBoolPref(SPLIT_RULE_VIEW_PREF);
> +
> + if (!this.isSplitRuleViewEnabled) {
Nit: I'd prefer to have a function like `addRuleView` or similar that has this branching in it so that it can be used here and in `onSplitRuleViewPrefChanged`.
And then either merge all of the logic from `addRuleViewToSidebar` and `addRuleViewToSplitView` into that new one, or keep them separate and call from the new one, depending on how much can be shared between them.
::: devtools/client/preferences/devtools.js:64
(Diff revision 8)
> // Enable the new color widget
> pref("devtools.inspector.colorWidget.enabled", false);
> // Enable the CSS shapes highlighter
> pref("devtools.inspector.shapesHighlighter.enabled", true);
> +// Enable the split rule view in the inspector
> +pref("devtools.inspector.split-rule-enabled", true);
Should this be on by default on all channels? Or is this something we want to do in Nightly only? If Nightly only, then should we have a second pref that causes the toggle button to be hidden, or do we want the button to ride the train?
Reporter | ||
Comment 17•7 years ago
|
||
Has this landed in Nightly yet (behind a flag or default on)? Looks like not yet.
I'd love to see it, and to user test it a bit, before we decide to send it on a train.
Comment 18•7 years ago
|
||
(In reply to Jen Simmons [:jensimmons] from comment #17)
> Has this landed in Nightly yet (behind a flag or default on)? Looks like not
> yet.
>
> I'd love to see it, and to user test it a bit, before we decide to send it
> on a train.
No it hasn't landed yet - it's blocked on Bug 1407347 for now. And I agree Nightly-only is the best option at least for now.
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8887268 [details]
Bug 1369945 - Part 1: Display a split rule view panel in the inspector.
https://reviewboard.mozilla.org/r/158064/#review194586
> Should this be on by default on all channels? Or is this something we want to do in Nightly only? If Nightly only, then should we have a second pref that causes the toggle button to be hidden, or do we want the button to ride the train?
I am not gonna address this in part 1. If we want to change the defaults, let's discuss this further and add another patch in this bug to do what we discussed.
Comment hidden (mozreview-request) |
Comment 21•7 years ago
|
||
(In reply to Gabriel [:gl] (ΦωΦ) from comment #19)
> Comment on attachment 8887268 [details]
> Bug 1369945 - Part 1: Display a split rule view panel in the inspector.
>
> https://reviewboard.mozilla.org/r/158064/#review194586
>
> > Should this be on by default on all channels? Or is this something we want to do in Nightly only? If Nightly only, then should we have a second pref that causes the toggle button to be hidden, or do we want the button to ride the train?
>
> I am not gonna address this in part 1. If we want to change the defaults,
> let's discuss this further and add another patch in this bug to do what we
> discussed.
Agreed about doing this in a separate patch - what do you think we should do for defaults? I think it'd be OK to just have the button always visible and not worry about hiding it behind a pref, but would be best to get Victoria's opinion. Would also suggest we turn the feature on by default in Nightly and off by default elsewhere. We may also want to turn it off by default for the Browser Toolbox to give the markup view more space.
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8887268 [details]
Bug 1369945 - Part 1: Display a split rule view panel in the inspector.
https://reviewboard.mozilla.org/r/158064/#review195610
This looks pretty much ready to go, just some minor comments throughout. Clearing review because this could use a simple test to make sure that the toggle works as expected.
::: devtools/client/inspector/inspector.js:534
(Diff revision 9)
> });
>
> - this._splitter = this.ReactDOM.render(splitter,
> + this.splitBox = this.ReactDOM.render(splitter,
> this.panelDoc.getElementById("inspector-splitter-box"));
>
> + if (!this.isSplitRuleViewEnabled) {
Is this still needed? The display will be set in addRuleView which is called shortly after this function
::: devtools/client/inspector/inspector.js:572
(Diff revision 9)
>
> // Initialize splitter size from preferences.
> try {
> width = Services.prefs.getIntPref("devtools.toolsidebar-width.inspector");
> height = Services.prefs.getIntPref("devtools.toolsidebar-height.inspector");
> + sidebarWidth = Services.prefs.getIntPref(
I see below that `devtools.toolsidebar-width.inspector.sidebar` / `sidebarWidth` are referring to the split rule view, but it's not clear from the names alone.
Could this be `devtools.toolsidebar-width.inspector.splitsidebar` / `splitSidebarWidth`, or something else that could help avoid a comment?
::: devtools/client/inspector/inspector.js:641
(Diff revision 9)
> + * Thie id of the default tab for the sidebar.
> + */
> + async addRuleView(defaultTab = "ruleview") {
> + let ruleViewSidebar = this.sidebarSplitBox.startPanelContainer;
> +
> + if (!this.isSplitRuleViewEnabled) {
Nit: I'd flip the conditions here and make it `if (this.isSplitRuleViewEnabled)` to make it a bit easier to read
::: devtools/client/locales/en-US/inspector.properties:70
(Diff revision 9)
> #LOCALIZATION NOTE: Used in the tooltip for Capturing
> eventsTooltip.Capturing=Capturing
>
> +# LOCALIZATION NOTE (inspector.displaySplitRulesView): This is the tooltip for the button
> +# that toggles on the display of a split rule view sidebar in the inspector.
> +inspector.showSplitRulesView=Show a split Rules panel
Nit: one of these strings says "a split Rules panel" and the other is "the split Rules panel". Would suggest making these consistent with "the" or removing the article in both
::: devtools/client/preferences/devtools.js:64
(Diff revision 9)
> // Enable the new color widget
> pref("devtools.inspector.colorWidget.enabled", false);
> // Enable the CSS shapes highlighter
> pref("devtools.inspector.shapesHighlighter.enabled", true);
> +// Enable the split rule view in the inspector
> +pref("devtools.inspector.split-rule-enabled", true);
As mentioned, please have a follow up patch that sets this conditionally
Attachment #8887268 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 23•7 years ago
|
||
Assignee | ||
Comment 24•7 years ago
|
||
Assignee | ||
Comment 25•7 years ago
|
||
Alex, just wanted you to be aware of this new feature/mode in the works. The idea is to have the ability to display 3 panes in the inspector. This is toggle-able with the former sidebar toggle. However, if the user has 3 pane modes on, and the toolbox was restarted it will default to loading the 3 panes. So, having this pref'd off by default wouldn't be a perfect solution. What you see in talos test is with the 3 panes being on, and it is opening both rules and computed view.
I am gonna push a follow up talos try run with the pref off to measure how much of this is because of loading a new ToolSidebar for the middle panel versus how much of this is from opening both rules and computed view.
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 26•7 years ago
|
||
We will of course be trying to tune and improve the performance before shipping, but I wanted you to be aware early on and perhaps you have ideas to improve this.
Assignee | ||
Comment 27•7 years ago
|
||
This is with the feature pref'd off - so not loading 3 panes, but the new toolsidebar for the middle pane is still created. https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=c9216beb912e3e76dbda857b3606d3bdefb0e7e2
Comment 28•7 years ago
|
||
(In reply to Gabriel [:gl] (ΦωΦ) from comment #27)
> This is with the feature pref'd off - so not loading 3 panes, but the new
> toolsidebar for the middle pane is still created.
> https://treeherder.mozilla.org/perf.html#/
> comparechooser?newProject=try&newRevision=c9216beb912e3e76dbda857b3606d3bdefb
> 0e7e2
Thanks for the head's up!
Looks good especially if default behavior is full speed.
(In reply to Gabriel [:gl] (ΦωΦ) from comment #26)
> We will of course be trying to tune and improve the performance before
> shipping, but I wanted you to be aware early on and perhaps you have ideas
> to improve this.
I imagine there is no magic here. Only way to mitigate the perf impact of this is to optimize all the sidebar panels...
The only thing I could think would be to work on load order. May be the rule view and markup should have a higher priority and be loaded first.
Flags: needinfo?(poirot.alex)
Comment 29•7 years ago
|
||
May be some DAMP test specific to layout view (or to this new mode) could help working on its performance before enabling this to broader audience.
Assignee | ||
Comment 30•7 years ago
|
||
Just for clarification - the new mode is to essentially pop the rules view into its own panel (middle of the 3 panel) where the right sidebar will still contain all the usual sidebar panels minus the rules view. The goal is to allow users to continue working with the styles while interacting with other panels of interest.
Assignee | ||
Comment 31•7 years ago
|
||
The DAMP results with it pref'd on is loading the rules and computed view.
Whiteboard: [devRel:P1] → [DevRel:P1][designer-tools]
Comment hidden (mozreview-request) |
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8887268 [details]
Bug 1369945 - Part 1: Display a split rule view panel in the inspector.
https://reviewboard.mozilla.org/r/158064/#review206596
Landing like this is fine with me, but please see my notes in comment 22 - it looks like they haven't been addressed yet
Attachment #8887268 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8887268 [details]
Bug 1369945 - Part 1: Display a split rule view panel in the inspector.
https://reviewboard.mozilla.org/r/158064/#review208594
::: devtools/client/inspector/inspector.js:534
(Diff revision 9)
> });
>
> - this._splitter = this.ReactDOM.render(splitter,
> + this.splitBox = this.ReactDOM.render(splitter,
> this.panelDoc.getElementById("inspector-splitter-box"));
>
> + if (!this.isSplitRuleViewEnabled) {
Removed
::: devtools/client/inspector/inspector.js:641
(Diff revision 9)
> + * Thie id of the default tab for the sidebar.
> + */
> + async addRuleView(defaultTab = "ruleview") {
> + let ruleViewSidebar = this.sidebarSplitBox.startPanelContainer;
> +
> + if (!this.isSplitRuleViewEnabled) {
Fixed.
::: devtools/client/preferences/devtools.js:64
(Diff revision 9)
> // Enable the new color widget
> pref("devtools.inspector.colorWidget.enabled", false);
> // Enable the CSS shapes highlighter
> pref("devtools.inspector.shapesHighlighter.enabled", true);
> +// Enable the split rule view in the inspector
> +pref("devtools.inspector.split-rule-enabled", true);
Defaults to false
Comment hidden (mozreview-request) |
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8887268 [details]
Bug 1369945 - Part 1: Display a split rule view panel in the inspector.
https://reviewboard.mozilla.org/r/158064/#review208632
Remove "Part 1" from the commit message since we are landing this in one patch
Attachment #8887268 -
Flags: review?(bgrinstead) → review+
Comment 37•7 years ago
|
||
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fbd437cab11
Display a split rule view panel in the inspector. r=bgrins
Comment 39•7 years ago
|
||
Backed out for eslint failure in devtools/client/shared/components/splitter/SplitBox.js and devtools failures, e.g. in devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d19d96f960842ec93a66c36e3d9369f0c6e9fbb
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4fbd437cab111cfaa53ba70409d99165c8fcdbee&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry
Eslint failure: https://treeherder.mozilla.org/logviewer.html#?job_id=147983629&repo=mozilla-inbound
Flags: needinfo?(gl)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gl)
Comment 40•7 years ago
|
||
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdbb870f32dc
Display a split rule view panel in the inspector. r=bgrins
Comment 41•7 years ago
|
||
Backed out 1 changesets (bug 1369945) for failing devtool tests test/browser_inspector_breadcrumbs_visibility.js r=backout on a CLOSED TREE
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/0e4e79435223abf6c5edf5320c11f3432b579fd3
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bdbb870f32dc45882620972e22c6207835f72ac0&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=148240030&repo=mozilla-inbound&lineNumber=2046
18:58:23 INFO - 31 INFO Checking timeline tick item elements after enlarge sidebar width
18:58:23 INFO - Buffered messages finished
18:58:23 ERROR - 32 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/animation/test/browser_animation_animation_list_time_tick.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/inspector/animation/test/head.js:120 - TypeError: inspector._splitter is undefined
18:58:23 INFO - Stack trace:
18:58:23 INFO - setSidebarWidth@chrome://mochitests/content/browser/devtools/client/inspector/animation/test/head.js:120:3
18:58:23 INFO - async*@chrome://mochitests/content/browser/devtools/client/inspector/animation/test/browser_animation_animation_list_time_tick.js:35:9
18:58:23 INFO - Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1065:21
18:58:23 INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:1056:9
18:58:23 INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:956:9
18:58:23 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
Flags: needinfo?(gl)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gl)
Comment 42•7 years ago
|
||
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/044f5a964dc5
Display a split rule view panel in the inspector. r=bgrins
Comment 43•7 years ago
|
||
Backed out changeset 044f5a964dc5 (bug 1369945) for Eslint failures client/shared/components/splitter/SplitBox.js and devtools failures test/browser_inspector_breadcrumbs_visibility.js r=backout on a CLOSED TREE
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/473dfd4578485ba7ed8cee11ae4366ed3f8dbc7d
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=044f5a964dc59a6a99a1f0bedb5ea436dc4559b1&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Eslint failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=148259950&repo=mozilla-inbound&lineNumber=224
[task 2017-11-28T20:21:15.092Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2017-11-28T20:29:02.156Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/shared/components/splitter/SplitBox.js:231:18 | Arrow function should not return assignment. (no-return-assign)
[task 2017-11-28T20:29:02.156Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/shared/components/splitter/SplitBox.js:246:18 | Arrow function should not return assignment. (no-return-assign)
devtools failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=148268425&repo=mozilla-inbound&lineNumber=3087
[task 2017-11-28T20:52:41.752Z] 20:52:41 ERROR - TEST-UNEXPECTED-TIMEOUT | devtools/client/inspector/test/browser_inspector_breadcrumbs_visibility.js | application timed out after 370 seconds with no output
[task 2017-11-28T20:52:41.752Z] 20:52:41 ERROR - Force-terminating active process(es).
Flags: needinfo?(gl)
Comment 44•7 years ago
|
||
Gabriel, before repushing it to m-c, could you push it to try with DAMP tests?
./mach try -b o -p linux64 -u none -t g2-e10s --rebuild-talos 6
I'm seeing a lot of fuzz on cold.inspector.open these last days and I'm wondering if it is related to this patch:
12% increase of time it takes to open the inspector for the first time!
https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,1556628,1,1&selected=%5Bmozilla-central,f953f2b413a907da389451bc2329f2a7b01999c2,282548,376565567,1%5D
changelog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cff54d05bcad74654249e2a2d4735b491bc452fd&tochange=5b33b070378ae0806bed0b5e5e34de429a29e7db
Didn't you had performance improvement to land first before enabling dual panels?
Assignee | ||
Comment 45•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #44)
> Gabriel, before repushing it to m-c, could you push it to try with DAMP
> tests?
> ./mach try -b o -p linux64 -u none -t g2-e10s --rebuild-talos 6
>
> I'm seeing a lot of fuzz on cold.inspector.open these last days and I'm
> wondering if it is related to this patch:
> 12% increase of time it takes to open the inspector for the first time!
>
> https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,
> 1556628,1,1&selected=%5Bmozilla-central,
> f953f2b413a907da389451bc2329f2a7b01999c2,282548,376565567,1%5D
> changelog:
>
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=cff54d05bcad74654249e2a2d4735b491bc452fd&tochange=5b33
> b070378ae0806bed0b5e5e34de429a29e7db
>
> Didn't you had performance improvement to land first before enabling dual
> panels?
This was landing it pref'd off. The goal was to land this pref'd off so our lead users can test it out and provide early user feedback. Try builds were insufficient because our lead users would rather flip a pref instead of downloading a build.
In terms of performance, this should realistically only see an extra hidden sidebar added.
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=c9dff5b40596539f1c07191069eac839bfbe41ab
Flags: needinfo?(gl)
Comment 46•7 years ago
|
||
(In reply to Gabriel [:gl] (ΦωΦ) from comment #45)
> (In reply to Alexandre Poirot [:ochameau] from comment #44)
> > Gabriel, before repushing it to m-c, could you push it to try with DAMP
> > tests?
> > ./mach try -b o -p linux64 -u none -t g2-e10s --rebuild-talos 6
> >
> > I'm seeing a lot of fuzz on cold.inspector.open these last days and I'm
> > wondering if it is related to this patch:
> > 12% increase of time it takes to open the inspector for the first time!
> >
> > https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,
> > 1556628,1,1&selected=%5Bmozilla-central,
> > f953f2b413a907da389451bc2329f2a7b01999c2,282548,376565567,1%5D
> > changelog:
> >
> > https://hg.mozilla.org/mozilla-central/
> > pushloghtml?fromchange=cff54d05bcad74654249e2a2d4735b491bc452fd&tochange=5b33
> > b070378ae0806bed0b5e5e34de429a29e7db
> >
> > Didn't you had performance improvement to land first before enabling dual
> > panels?
>
> This was landing it pref'd off. The goal was to land this pref'd off so our
> lead users can test it out and provide early user feedback. Try builds were
> insufficient because our lead users would rather flip a pref instead of
> downloading a build.
>
> In terms of performance, this should realistically only see an extra hidden
> sidebar added.
>
> https://treeherder.mozilla.org/perf.html#/
> comparechooser?newProject=try&newRevision=c9dff5b40596539f1c07191069eac839bfb
> e41ab
DAMP says it might have an impact on cold open, but nothing of the scale of what I'm saw (3% versus 12%)
It may just be because of the additional SidebarToggle.css loaded in inspector.xhtml...
But DAMP is more confident on reporting a small regression on complicated.reload 2.78%,
likely related to the additional SplitBox component added in this patch.
complicated.reload means that the inspector is slower to update itself when reloading a webpage.
Comment 47•7 years ago
|
||
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0313f74cb8ff
Display a split rule view panel in the inspector. r=bgrins
Comment 48•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•7 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 49•7 years ago
|
||
This is not intended to be exposed publicly.
Keywords: dev-doc-needed
Depends on: 1431236
Assignee | ||
Updated•7 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•