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)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
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)

Bug 1369945 - Part 1: Display a split rule view panel in the inspector.
59 bytes, text/x-review-board-request
bgrins
: review+
Details
portrait-landscape.gif
1.22 MB, image/gif
Details 
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.
Blocks: dt-grid
No longer blocks: 1347964
Priority: -- → P3
Assignee: nobody → tigleym
Status: NEW → ASSIGNED
Blocks: 1376157
Attachment #8887268 - Flags: review?(pbrosset)
Attachment #8887268 - Flags: review?(bgrinstead)
Attachment #8887268 - Flags: review?(pbrosset)
Attachment #8887268 - Flags: review?(bgrinstead)
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+
I'm planning to look at this tomorrow
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).
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.
Attached image portrait-landscape.gif — 
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 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-
Keywords: DevAdvocacy
Whiteboard: [devRel:P1]
No longer blocks: dt-grid
Attachment #8887268 - Attachment is obsolete: true
Attachment #8887268 - Flags: review?(bgrinstead)
Attachment #8887268 - Attachment is obsolete: true
Depends on: 1405063
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: tigleym → gl
Attachment #8887268 - Flags: review?(bgrinstead)
Depends on: 1407347
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?
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.
(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.
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.
(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 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)
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)
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.
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
(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)
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.
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.
The DAMP results with it pref'd on is loading the rules and computed view.
Whiteboard: [devRel:P1] → [DevRel:P1][designer-tools]
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)
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 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+
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
No longer blocks: 1376157
Flags: needinfo?(gl)
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
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)
Flags: needinfo?(gl)
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
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)
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?
(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)
(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.
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
https://hg.mozilla.org/mozilla-central/rev/0313f74cb8ff
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
This is not intended to be exposed publicly.
Keywords: dev-doc-needed
Depends on: 1424658
Depends on: 1425653
Depends on: 1427051
Depends on: 1433611
No longer depends on: 1407347, 1433611, 1405063, 1424658, 1425653, 1427051, 1431236
Blocks: 1433716
Product: Firefox → DevTools
Depends on: 1471935
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: