1503175 - 3-pane inspector does not restore the previous column sizes

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Patrick Brosset <:pbro>]

I could swear we had this working in the past, so I'm a bit surprised. But better file it anyway:

Steps:
- open the inspector
- make sure you are in "3-pane mode" (if you see only 2 columns: DOM and CSS then click on the expand icon in the toolbar to see 3 columns: DOM, CSS and Layout)
- resize the 3 columns to whatever you like
- close the inspector
- re-open it again

==> The previous sizes aren't restored, which can be very frustrating.

Comment 1

[Vincent]

Can I take a look at this bug?

btw:
 - it can be reproduced with the 2 columns layout too,
 - it seems to be a recent regression: I can repro on a dev FF, it works fine on FF 63.0 (current prod).

Comment 2

[Patrick Brosset <:pbro>]

Of course, I'll assign it to you. I'm running a mozregression at the moment to find out which bug broke this, maybe this will help us find a solution.

Comment 3

[Patrick Brosset <:pbro>]

mozregression completed and pointed to this pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=409be898a58d93400ea8c6cad5d4385b64dc628a&tochange=3f01040106719f099339ebe65ff98690a65ee9f0

Out of the 2 bugs, the cause for this regression is probably bug 1494162. And in particular the patch part 29 in that bug broke this: https://hg.mozilla.org/integration/mozilla-inbound/rev/3f0104010671

So that confirms that this regressed in 64. This means as soon as we find a fix, we should not only land it in m-c as always, but also request an uplift so it also lands on the beta branch (in 64).

Vincent: don't worry about this for now, we have time to work on this, and I'll request the uplift whenever the time is right.

Comment 4

[Vincent]

Thanks for the assignment, taking a look now.

For reference, not all tabs seem to actually record their preferred size (verified on ff 63.0):
 - Splitbox storing/restoring width: inspector and style tabs
 - Splitbox not restoring: a11y and debugger tabs
 - Splitbox not draggable: canvas, memory
 - No Splitbox: console, network, webaudio, dom, shader, scratchpad

Comment 5

[Vincent]

I found the issue.
The sidebar's lifecycle was not properly tied to the inspector's lifecycle.
When the sidebar is destroyed or hidden, it stores the preferred widths for the various panels.
However, the inspector destroy was not calling the sidebar destroy properly.
Properties were never stored. At read time, the pref reading code was throwing and defaulting to the default width values.

The fix should be a one liner and uplift-friendly.
I still need to write a non-regression test.

After reading some of the lifecycle management JS code, I think there is some room for improvement here.
Not sure what's the best forum/mode to make an impactful proposal though.

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Vincent from comment #5)
> After reading some of the lifecycle management JS code, I think there is
> some room for improvement here.
> Not sure what's the best forum/mode to make an impactful proposal though.
Great work so far! 
I suggest joining the public devtools-html slack and the #inspector channel there. The relevant people to include in this conversation would be @gl, @rcaliman I think.

Comment 7

[Vincent]

Attachment: fix_inspector_layout_on_open.patch

Comment 8

[Vincent]

Attachment: fix_inspector_layout_on_open.patch

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Vincent, it looks like the review flag was cleared on the second version of the patch...  Is that one intended for review now?

Comment 10

[Vincent]

Attachment: fix_inspector_layout_on_open.patch

Adding back the patch with a review request. Thanks!

Comment 11

[Vincent]

Ready for review, thanks jryans!

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 9024051 [details] [diff] [review]
fix_inspector_layout_on_open.patch

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

Thanks a lot Vincent for submitting a fix for this so quickly!
Let me pass this on to Gabriel since he was the one who worked on this originally.

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 9024051 [details] [diff] [review]
fix_inspector_layout_on_open.patch

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

::: devtools/client/inspector/test/browser_inspector_sidebarstate.js
@@ +41,5 @@
>  ];
>  
> +// Verify that the inspector properly read/write from user preferences to
> +// layout the inspector split boxes.
> +add_task(async function() {

We should move this to a new test file. Perhaps name it browser_inspector_pane_state_restore.js.

@@ +43,5 @@
> +// Verify that the inspector properly read/write from user preferences to
> +// layout the inspector split boxes.
> +add_task(async function() {
> +  // Simulate that the user already has stored splitboxes preferred widths.
> +  const EXPECTED_INITIAL_WIDTH = 101;

We should move these constants outside the add_task(). See https://searchfox.org/mozilla-central/source/devtools/client/inspector/test/browser_inspector_pane-toggle-03.js.

@@ +46,5 @@
> +  // Simulate that the user already has stored splitboxes preferred widths.
> +  const EXPECTED_INITIAL_WIDTH = 101;
> +  const EXPECTED_INITIAL_HEIGHT = 102;
> +  const EXPECTED_INITIAL_SIDEBAR_WIDTH = 103;
> +  Services.prefs.setIntPref(

We should use "await pushPref" to set these values.

@@ +53,5 @@
> +    "devtools.toolsidebar-height.inspector", EXPECTED_INITIAL_HEIGHT);
> +  Services.prefs.setIntPref(
> +    "devtools.toolsidebar-width.inspector.splitsidebar", EXPECTED_INITIAL_SIDEBAR_WIDTH);
> +
> +  // Open inspector.

We can remove this comment since it's implied by the function name.

@@ +54,5 @@
> +  Services.prefs.setIntPref(
> +    "devtools.toolsidebar-width.inspector.splitsidebar", EXPECTED_INITIAL_SIDEBAR_WIDTH);
> +
> +  // Open inspector.
> +  const res = await openInspectorForURL("about:blank");

Simplify to const { inspector } = await openInspectorForURL("about:blank");

Comment 14

[Vincent]

Thanks Gabriel for the review!
However, I'm on a vacation break this week and don't have access to my dev machine right now.
I will continue on this next week, most likely Tuesday/Wednesday.

Comment 15

[Vincent]

Attachment: fix_inspector_layout_on_open.patch

I applied your comments and ran eslint/mochitests.
I did a new round of local testing too since the original patch was quite old.

Please take another look to the updated patch.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/decb54eeaa7d
Restore the previous inspector layout when reopening. r=gl

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

Thank you for the patch, Vincent. I made minor comment changes to your unit test and landed it.

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/decb54eeaa7d

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Maybe request uplift?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Note that Fx64 is being released tomorrow, so the approval request will need to be for mozilla-release for possible dot release inclusion.

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 9028754 [details] [diff] [review]
fix_inspector_layout_on_open.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1503175

User impact if declined: 3-pane inspector does not restore the previous column sizes

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: 1. Open the inspector in 3 pane mode.
2. Change the width of the any of the sidebars.
3. Close the devtools
4. Reopen the inspector
5. Check that the widths of the newly opened inspector is the same as before it was closed

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Covered by automated tests, and the code itself is a simply destroy method call to ensure the widths of the inspector is stored in the prefs when the devtools are closed.

String changes made/needed:

Comment 22

[Julien Cristau [:jcristau]]

Comment on attachment 9028754 [details] [diff] [review]
fix_inspector_layout_on_open.patch

looks safe enough to ride along in 64.0.2.

Comment 23

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/a2b5b227742b

Comment 24

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced this issue using Fx 64.0b buildID 20181206201918 on Windows 10 x64.

Confirming the fix across platforms using:

• 64.0.2, buildID 20190108160530
• 65.0b9, buildID 20190107180200