1397295 - Browser Toolbox doesn't remember its size and location

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Mark Banner (:standard8)]

STR

1) Build an artifact build of nightly (I suspect this also works on all nightly builds, this is just what I'm using)
2) Have a "dev" profile and run

./mach run -P dev

3) Open the Browser Toolbox and Browser Console.
4) Resize the toolbox and console windows into something different that you'll remember.
5) Quit Nightly.
6) Start it again as per step 2.
7) Open the Browser Toolbox and Browser Console.

Expected Results

=> Both the toolbox and console are at the size and location set in step 4

Actual Results

=> Only the console remembers its size and location, the toolbox reverts to a "standard" position.

Hence, this makes debugging hard as I have to reset the size and location every time I open it up.

Comment 1

[Patrick Brosset <:pbro>]

Works fine in 55 and 56. So this is a new problem with 57. It would be very nice for DevTools, Firefox and WebExtensions developers to have this fixed. Not useful for web developers though. So probably not worth blocking 57, but still would be a nice to have.
I'll try to investigate a bit.

Comment 2

[Patrick Brosset <:pbro>]

So the initial size and persistence come from this code:

<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
        id="devtools-toolbox-window"
        macanimationtype="document"
        fullscreenbutton="true"
        windowtype="devtools:toolbox"
        width="900" height="600"
        persist="screenX screenY width height sizemode">

in devtools\client\framework\toolbox-process-window.xul

Which seems to say that XUL windows offer some kind of automatic persistence. I'm not seeing any devtools pref that would hold those values.

The weird thing is that it works for non-browser-toolbox toolboxes (normal toolboxes that is). And they use the same code:

<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
        id="devtools-toolbox-window"
        macanimationtype="document"
        fullscreenbutton="true"
        windowtype="devtools:toolbox"
        width="900" height="320"
        persist="screenX screenY width height sizemode">

in devtools\client\framework\toolbox-window.xul

Comment 3

[Patrick Brosset <:pbro>]

The persist attribute relies on a profile being available, that's where the persisted values end up.
In 57, we changed a few things related to this, so I backed out a few bugs and found the culprit: bug 1392744.

Brian, any idea how that bug could have caused the regression?
Also, do you agree with me about marking this as 57 fix-optional?

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset <:pbro> from comment #3)
> The persist attribute relies on a profile being available, that's where the
> persisted values end up.
> In 57, we changed a few things related to this, so I backed out a few bugs
> and found the culprit: bug 1392744.
> 
> Brian, any idea how that bug could have caused the regression?
> Also, do you agree with me about marking this as 57 fix-optional?

That change didn't affect which profile the Browser Toolbox loads with. I suspect the persistence is keyed on the URL, and since we are now passing a query string to specify the port the URL is changing every time it gets opened (https://hg.mozilla.org/mozilla-central/rev/30182d13fb9d#l1.49). So this would have been a problem before with addon toolboxes opened through ToolboxProcess, but all Browser Toolboxes had the same URL.

Mossop, are you aware of a way to make the `persist` attribute work for a page that can be loaded with different query string values? If there's not an easy way to change that, we could use an environment variable instead of a query string value.

Comment 5

[Dave Townsend [:mossop]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Patrick Brosset <:pbro> from comment #3)
> > The persist attribute relies on a profile being available, that's where the
> > persisted values end up.
> > In 57, we changed a few things related to this, so I backed out a few bugs
> > and found the culprit: bug 1392744.
> > 
> > Brian, any idea how that bug could have caused the regression?
> > Also, do you agree with me about marking this as 57 fix-optional?
> 
> That change didn't affect which profile the Browser Toolbox loads with. I
> suspect the persistence is keyed on the URL, and since we are now passing a
> query string to specify the port the URL is changing every time it gets
> opened (https://hg.mozilla.org/mozilla-central/rev/30182d13fb9d#l1.49). So
> this would have been a problem before with addon toolboxes opened through
> ToolboxProcess, but all Browser Toolboxes had the same URL.

This sounds right to me.

> Mossop, are you aware of a way to make the `persist` attribute work for a
> page that can be loaded with different query string values? If there's not
> an easy way to change that, we could use an environment variable instead of
> a query string value.

Short of changing the persistence system I don't think there is a way.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Dave Townsend [:mossop] from comment #5)
> > Mossop, are you aware of a way to make the `persist` attribute work for a
> > page that can be loaded with different query string values? If there's not
> > an easy way to change that, we could use an environment variable instead of
> > a query string value.
> 
> Short of changing the persistence system I don't think there is a way.

OK, in that case I'd suggest to fix this bug we migrate the port and addonId query string parameters to environment variables.

I do agree with it being fix-optional for 57, as it's Browser Toolbox only so getting it fixed for local builds is the main priority

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: Bug 1397295 - Use environment variables instead of query string for params to Browser Toolbox;

This allows data like screen position to be persisted in the xul store for the document

Review commit: https://reviewboard.mozilla.org/r/181002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181002/

Comment 8

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

Comment on attachment 8909536 [details]
Bug 1397295 - Use environment variables instead of query string for params to Browser Toolbox;

https://reviewboard.mozilla.org/r/181002/#review186548

Looks good to me, thanks! :)

Comment 9

[Brian Grinstead [:bgrins]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e49233ffda07282dfd5104634cd27340e64236a4

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad4a5b8426ed
Use environment variables instead of query string for params to Browser Toolbox;r=jryans

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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