1432015 - Separate the set of known views from the stack of open views in PanelMultiView

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Paolo Amadini]

This was originally part of bug 1428839.

Comment 1

[:Paolo Amadini]

Attachment: Bug 1432015 - Part 2 - Remove the setMainView methods.

The setMainView method of PanelMultiView controls the "mainview" attribute, which is already set or removed later in the showSubView method. When called at construction time, it changes the mainViewId if there is at least one child in the "panelmultiview" element and the original mainViewId does not already reference the first child. This would be incorrect, but in practice it never happens for either ephemeral or static panels.

The setMainView method of PanelUI is never called, and this makes the corresponding PanelMultiView method removable.

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

Comment 2

[:Paolo Amadini]

Attachment: Bug 1432015 - Part 3 - Ensure that views passed to showSubView are added to the list of known views without having to reset the internal _panelViews variable.

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

Comment 3

[:Paolo Amadini]

Attachment: Bug 1432015 - Part 4 - Separate the set of known views from the stack of open views.

This makes the code easier to follow and facilitates future refactoring, for example the set of known views can be removed entirely by making the clean up and navigation code use the stack of open views.

The SlidingPanelView class can thus be removed, saving various lines of code. The class implemented a small optimization for garbage collection, that was already less effective because various other objects are created during each view transition anyways.

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

Comment 4

[:Paolo Amadini]

Attachment: Bug 1432015 - Part 5 - Remove unused code.

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

Comment 5

[:Paolo Amadini]

Comment on attachment 8944247 [details]
Bug 1432015 - Part 2 - Remove the setMainView methods.

Carrying over r+ from bug 1428839.

Comment 6

[:Paolo Amadini]

Comment on attachment 8944250 [details]
Bug 1432015 - Part 5 - Remove unused code.

I spotted this piece of unused code while moving state to the PanelView class.

As this code can be removed at any point, I just added it to the end of this bug.

Comment 7

[:Paolo Amadini]

Let's check if there is any regression test to update following these changes:

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

Comment 8

[:Paolo Amadini]

Attachment: Bug 1432015 - Part 1 - Fix anchor handling for the action feedback panel.

Commands started from an activated action panel should not be anchored to the main button, and regression tests should simulate the case where both the main action panel and the individual action button are visible and the action is invoked from the main action panel.

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

Comment 9

[:Paolo Amadini]

Comment on attachment 8944247 [details]
Bug 1432015 - Part 2 - Remove the setMainView methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214514/diff/1-2/

Comment 10

[:Paolo Amadini]

Comment on attachment 8944248 [details]
Bug 1432015 - Part 3 - Ensure that views passed to showSubView are added to the list of known views without having to reset the internal _panelViews variable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214516/diff/1-2/

Comment 11

[:Paolo Amadini]

Comment on attachment 8944249 [details]
Bug 1432015 - Part 4 - Separate the set of known views from the stack of open views.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214518/diff/1-2/

Comment 12

[:Paolo Amadini]

Comment on attachment 8944250 [details]
Bug 1432015 - Part 5 - Remove unused code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214520/diff/1-2/

Comment 13

[:Gijs (he/him)]

Comment on attachment 8944250 [details]
Bug 1432015 - Part 5 - Remove unused code.

https://reviewboard.mozilla.org/r/214520/#review220126

Comment 14

[:Paolo Amadini]

I had to add a missing mainViewId attribute for activated action panels, which are a special case of throw-away non-ephemeral panels, aligning them to what other similar code does. I had to fix one test for the feedback of the "Send to Device" action that weirdly stated failing after that change, although the original code under test apparently should not have handled the case correctly in the first place.

I also fixed a null reference when using the "current" attribute on a PanelMultiView that was already destroyed.

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

Comment 15

[:Paolo Amadini]

Hm, the latest run is apparently hitting the same intermittent failures as the ones in bug 1402845 comment 45. These are probably entirely based on timing or on accessing properties of destroyed objects.

These failures will be much easier to understand after the rest of the refactoring has been done and all the code using public members for cleanup purposes has been removed. In other words, I plan to ignore the browser_ext_browserAction_disabled.js and browser_ext_browserAction_popup.js failures that we have on Mac OS X in the try runs for now. This means that even though I split the work in multiple bugs, we may still have to land everything together to avoid this chicken-and-egg situation.

Comment 16

[:Paolo Amadini]

Curiously, and seemingly confirming the workability of the strategy in the previous comment, the failures in browser_ext_browserAction_popup_resize.js have gone away spontaneously after applying the four changesets in bug 1432016 on top of the ones in this bug.

Comment 17

[:Gijs (he/him)]

I'm a little bit confused as to what happened to the flags here. Do I actually need to re-review parts 1, 2 and 4?

Comment 18

[:Paolo Amadini]

Part 1 is new so it needs a new review, while parts 2 and 4 have small changes for comment 14 that you may want to take a look at, although they're quite simple and likely don't need too much scrutiny.

Thanks for the great reviews on bug 1432016, I'll move the resetting of openViews to one of the changesets here.

Comment 19

[:Paolo Amadini]

Comment on attachment 8944249 [details]
Bug 1432015 - Part 4 - Separate the set of known views from the stack of open views.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214518/diff/2-3/

Comment 20

[:Paolo Amadini]

Comment on attachment 8944250 [details]
Bug 1432015 - Part 5 - Remove unused code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214520/diff/2-3/

Comment 21

[:Gijs (he/him)]

Comment on attachment 8944290 [details]
Bug 1432015 - Part 1 - Fix anchor handling for the action feedback panel.

https://reviewboard.mozilla.org/r/214536/#review220222

I mean, the theory of this makes sense to me, so rs=me for that. But it doesn't seem particularly related to the summary of this bug, right? Also, the net consequence is the horizontal shifting of a panel without an arrow that is now anchored on the main panel button even if it'd otherwise use the url bar button (I guess?), so the practical consequences are also limited... So why do this here/now? Am I misunderstanding what's being fixed?

The first subsentence of the commit message ("Commands started from an activated action panel should not be anchored to the main button") doesn't really make sense to me wrt the test change. I guess this would more precisely apply to e.g. commands triggered from a separate synced tabs panel (if you pinned that action) which would have an ancestor panel (the synced tabs one) and shouldn't use the 'main' button for anchoring. But you don't seem to create a test for this case, instead leaving it with the url copy entry which *is* in the main panel. So I'm confused. :-)

Comment 22

[:Gijs (he/him)]

Comment on attachment 8944247 [details]
Bug 1432015 - Part 2 - Remove the setMainView methods.

https://reviewboard.mozilla.org/r/214514/#review220228

::: browser/base/content/browser-pageActions.js:237
(Diff revision 2)
>        let multiViewNode = document.createElement("panelmultiview");
>        panelViewNode = this._makePanelViewNodeForAction(action, true);
> +      multiViewNode.setAttribute("mainViewId", panelViewNode.id);
>        multiViewNode.appendChild(panelViewNode);
>        panelNode.appendChild(multiViewNode);

It would be nice if we could make this + PanelUI use a static method on PanelMultiView instead, or something (in a separate bug).

Comment 23

[:Gijs (he/him)]

Comment on attachment 8944249 [details]
Bug 1432015 - Part 4 - Separate the set of known views from the stack of open views.

https://reviewboard.mozilla.org/r/214518/#review220230

::: browser/components/customizableui/PanelMultiView.jsm:135
(Diff revisions 1 - 3)
>     * @return {panelview} the currently visible subview OR the subview that is
>     *                     about to be shown whilst a 'ViewShowing' event is being
>     *                     dispatched.
>     */
>    get current() {
> -    return this._viewShowing || this._currentSubView;
> +    return this.node && (this._viewShowing || this._currentSubView);

So I'm guessing this fixes the null reference. When did this actually happen?

Comment 24

[:Paolo Amadini]

(In reply to :Gijs from comment #21)
> The first subsentence of the commit message ("Commands started from an
> activated action panel should not be anchored to the main button") doesn't
> really make sense to me wrt the test change.

There are two issues being fixed here. I used only one changeset as they are related, but let me know if I should separate them.

> I guess this would more
> precisely apply to e.g. commands triggered from a separate synced tabs panel
> (if you pinned that action) which would have an ancestor panel (the synced
> tabs one) and shouldn't use the 'main' button for anchoring. But you don't
> seem to create a test for this case, instead leaving it with the url copy
> entry which *is* in the main panel.

There is already a test for this case in the synced tabs panel. Given the mistake in the production code this test was supposed to fail already, but for some reason it only started failing after the other changes I made in this bug. I don't know why this happened, but it's not worth spending time finding out given that the test does the right thing.

To fix the test, I initially tried removing the "event" argument entirely, as I supposed this was just leftover code, and all the tests still passed. For good measure I looked up the bug where this argument was added, and I found out that the tests added in that bug actually didn't cover the case that was being fixed there. Hence the test changes, that are not strictly related to the fix above but are still good to have.

> So I'm confused. :-)

Comment 14 wasn't that clear... hope this one clarifies better! :-)

Comment 25

[:Paolo Amadini]

(In reply to :Gijs from comment #23)
> So I'm guessing this fixes the null reference. When did this actually happen?

https://dxr.mozilla.org/mozilla-central/rev/9fe69ff0762da191fbd2b9fc0718e96f1ab4137e/browser/components/customizableui/content/panelUI.js#427-432

I'll definitely get rid of this piece of code by the end of bug 1428839, as "closing" the view in the old panel should be possible synchronously, and should be entirely handled by the PanelMultiView methods.

This will probably facilitate removing the "current" property entirely, as it's ambiguous during asynchronous animation, and also the "showMainView" method.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac47c9282115
Part 1 - Fix anchor handling for the action feedback panel. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/62330176bbb8
Part 2 - Remove the setMainView methods. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/0022830a87cb
Part 3 - Ensure that views passed to showSubView are added to the list of known views without having to reset the internal _panelViews variable. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1df27563323
Part 4 - Separate the set of known views from the stack of open views. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/4db8bc4b474c
Part 5 - Remove unused code. r=Gijs

Comment 27

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/ac47c9282115
https://hg.mozilla.org/mozilla-central/rev/62330176bbb8
https://hg.mozilla.org/mozilla-central/rev/0022830a87cb
https://hg.mozilla.org/mozilla-central/rev/e1df27563323
https://hg.mozilla.org/mozilla-central/rev/4db8bc4b474c