Closed Bug 1432016 Opened 6 years ago Closed 6 years ago

Move code operating on a single view to a new PanelView class

Categories

(Firefox :: Toolbars and Customization, enhancement)

Product:
Firefox
Component:
Toolbars and Customization
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(4 files)

Bug 1432016 - Part 1 - Add a PanelView class using a base class shared with PanelMultiView.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1432016 - Part 2 - Move descriptionHeightWorkaround and some other methods to the PanelView class.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1432016 - Part 3 - Use the PanelView class for knownViews and openViews.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1432016 - Part 4 - Move keyboard navigation to the PanelView class.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
It turns out that a lot of the methods in the PanelMultiView class are actually  designed to operate on a single view. By moving these to a PanelView class, we reduce code complexity and argument passing and we can remove constructs such as the keyboard navigation map.

This also helps with moving to a model where we keep state on the PanelView class, allowing it to be attached to different PanelMultiView instances at different times, rationalizing the clean up code.

This was originally part of bug 1428839.
Flags: qe-verify-
This is still an intermediate state. Most of the PanelView.forNode internal calls will disappear later.
Comment on attachment 8944253 [details]
Bug 1432016 - Part 1 - Add a PanelView class using a base class shared with PanelMultiView.

https://reviewboard.mozilla.org/r/214524/#review220164

::: browser/components/customizableui/PanelMultiView.jsm:115
(Diff revision 1)
> +/**
> + * This is the implementation of the panelUI.xml XBL binding, moved to this
> + * module, to make it easier to fork the logic for the newer photon structure.
> + * Goals are:
> + * 1. to make it easier to programmatically extend the list of panels,
> + * 2. allow for navigation between panels multiple levels deep and
> + * 3. maintain the pre-photon structure with as little effort possible.
> + *

I don't think most of this is particularly relevant anymore. I know you're just moving it, but might as well update this.
Attachment #8944253 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8944254 [details]
Bug 1432016 - Part 2 - Move descriptionHeightWorkaround and some other methods to the PanelView class.

https://reviewboard.mozilla.org/r/214526/#review220166

::: browser/base/content/browser.js:7150
(Diff revision 1)
> +  },
>    get _identityPopupContentHosts() {
>      delete this._identityPopupContentHosts;
>      let selector = ".identity-popup-host";
>      return this._identityPopupContentHosts = [
> -      ...this._identityPopupMultiView._mainView.querySelectorAll(selector),
> +      ...this._identityPopupMainView.querySelectorAll(selector),

This is wrong, the "whole point" of having these is that they ought to select both the items in the main view and the security view.

::: browser/base/content/browser.js:7158
(Diff revision 1)
>    },
>    get _identityPopupContentHostless() {
>      delete this._identityPopupContentHostless;
>      let selector = ".identity-popup-hostless";
>      return this._identityPopupContentHostless = [
> -      ...this._identityPopupMultiView._mainView.querySelectorAll(selector),
> +      ...this._identityPopupMainView.querySelectorAll(selector),

Same here.

::: browser/components/customizableui/PanelMultiView.jsm:384
(Diff revision 1)
>          }
>        } else if (viewNode.parentNode == this._panelViewCache) {
>          this._viewStack.appendChild(viewNode);
>        }
>  
> +      let newPanelView = PanelView.forNode(viewNode);

I don't really understand why you called this `newPanelView` - it's not necessarily new, right (ie the instance might already have existed)? I guess you mean "the panelview that's about to be the current one". Perhaps we should just call it `nextPanelView` to match the terminology used for `previousViewNode` ? Or just `panelView` given `viewNode` is the one we're about to select.

::: browser/components/customizableui/PanelMultiView.jsm:494
(Diff revision 1)
>        return;
>      }
>  
>      const {window, document} = this;
>  
> +    let newPanelView = PanelView.forNode(viewNode);

Same here.

::: browser/components/customizableui/PanelMultiView.jsm:774
(Diff revision 1)
>          break;
>        }
>        case "popupshown":
>          // Now that the main view is visible, we can check the height of the
>          // description elements it contains.
> -        this.descriptionHeightWorkaround();
> +        if (this._mainView) {

When is `_mainView` null at the point where popupshown is fired? This is smelly, and either we shouldn't need the nullcheck, or there should be a comment explaining how/when this can happen.

::: browser/components/customizableui/PanelMultiView.jsm:1004
(Diff revision 1)
> +  get title() {
> +    return this.node.getAttribute("title");
> +  }

I think this getter is just confusing. It won't always return the actual header contents (ie the view can visually have a title yet this would return `""`), and it's only used once. Might as well just use the getAttribute call in the original location where this now calls `.title`, because it's an argument to `setHeader` anyway, and from an OOP perspective it wouldn't make sense to have an object that knows that its `title` is X but then needs some external code to call `setHeader(X)`.

::: browser/components/customizableui/PanelMultiView.jsm:1009
(Diff revision 1)
> +   * The "mainview" attribute is set before the panel is opened when this view
> +   * is displayed as the main view, and is reset before the <panelview> is
> +   * displayed as a subview.

What does the "reset" thing mean? Why would we need to set this more than once? :-\

::: browser/components/customizableui/PanelMultiView.jsm:1045
(Diff revision 1)
> +  }
> +
> +  /**
> +   * Adds a header with the given title, or removes it if the title is empty.
> +   */
> +  setHeader(titleText) {

There's no getter for this and the other properties, but the other properties are `set foo`, but this is `setFoo()`. Pick one of the two styles, don't mix them.
Attachment #8944254 - Flags: review?(gijskruitbosch+bugs) → review-
Comment on attachment 8944255 [details]
Bug 1432016 - Part 3 - Use the PanelView class for knownViews and openViews.

https://reviewboard.mozilla.org/r/214528/#review220174

I mean, tentative r+, but what does this get us?

::: browser/components/customizableui/PanelMultiView.jsm:365
(Diff revision 1)
>      this._viewShowing = null;
>  
>      if (!this.node || !theOne)
>        return;
>  
> -    if (!this.openViews.includes(theOne))
> +    let newPanelView = PanelView.forNode(theOne);

Again, I don't think `new` makes sense here. Given the method context, maybe `visiblePanelView` ?
Attachment #8944255 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8944256 [details]
Bug 1432016 - Part 4 - Move keyboard navigation to the PanelView class.

https://reviewboard.mozilla.org/r/214530/#review220176

r=me minus the openViews resetting that I'm 99% sure belongs elsewhere.

::: browser/components/customizableui/PanelMultiView.jsm:795
(Diff revision 1)
>            }
>          }
>          this.window.removeEventListener("keydown", this);
>          this._panel.removeEventListener("mousemove", this);
> -        this._resetKeyNavigation();
> +        this.openViews.forEach(panelView => panelView.clearNavigation());
> +        this.openViews = [];

This seems a change that's unrelated to the key navigation, right? Feels like this should have been done in one of the csets in the other bug, tbh...
Attachment #8944256 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #8)
> Comment on attachment 8944254 [details]
> Bug 1432016 - Part 2 - Move descriptionHeightWorkaround and some other
> methods to the PanelView class.
> 
> https://reviewboard.mozilla.org/r/214526/#review220166
> 
> ::: browser/base/content/browser.js:7150
> (Diff revision 1)
> > +  },
> >    get _identityPopupContentHosts() {
> >      delete this._identityPopupContentHosts;
> >      let selector = ".identity-popup-host";
> >      return this._identityPopupContentHosts = [
> > -      ...this._identityPopupMultiView._mainView.querySelectorAll(selector),
> > +      ...this._identityPopupMainView.querySelectorAll(selector),
> 
> This is wrong, the "whole point" of having these is that they ought to
> select both the items in the main view and the security view.
> 
> ::: browser/base/content/browser.js:7158
> (Diff revision 1)
> >    },
> >    get _identityPopupContentHostless() {
> >      delete this._identityPopupContentHostless;
> >      let selector = ".identity-popup-hostless";
> >      return this._identityPopupContentHostless = [
> > -      ...this._identityPopupMultiView._mainView.querySelectorAll(selector),
> > +      ...this._identityPopupMainView.querySelectorAll(selector),
> 
> Same here.

Huh, I just realized *I'm* wrong. This is using the second qSA call (not shown in quote) as backup already.

Basically, this code is terrible as it is (blame can probably find you the bug where we realized why/how this was broken, having to do with XBL anon content and how qSA (doesn't) work(s) with it), and if we can find a way of making it more sane without breaking either of the 2 views, that would be great.
(In reply to :Gijs from comment #11)
> Basically, this code is terrible as it is (blame can probably find you the
> bug where we realized why/how this was broken, having to do with XBL anon
> content and how qSA (doesn't) work(s) with it), and if we can find a way of
> making it more sane without breaking either of the 2 views, that would be
> great.

That's the reason why I assumed this code was necessary, but it turns out that the second qSA is enough to select both elements even though they are in different subviews. I'll just simplify this and see if any regression test has to be adjusted.
(In reply to :Gijs from comment #9)
> I mean, tentative r+, but what does this get us?

In the end it will reduce the calls to PanelView.forNode(). Most arguments of functions will eventually be direct references to PanelView objects, but since I'm still moving methods and properties to the PanelView class, I've not started doing it yet for arguments that will eventually disappear.

> ::: browser/components/customizableui/PanelMultiView.jsm:365
> (Diff revision 1)
> >      this._viewShowing = null;
> >  
> >      if (!this.node || !theOne)
> >        return;
> >  
> > -    if (!this.openViews.includes(theOne))
> > +    let newPanelView = PanelView.forNode(theOne);
> 
> Again, I don't think `new` makes sense here. Given the method context, maybe
> `visiblePanelView` ?

It's the same as the nextPanelView. I've updated the argument of this function to be the PanelView object, which I planned to do later but may already make sense to start doing here for this function.
Comment on attachment 8944254 [details]
Bug 1432016 - Part 2 - Move descriptionHeightWorkaround and some other methods to the PanelView class.

https://reviewboard.mozilla.org/r/214526/#review220220

r=me assuming try is happy with the browser.js changes, and the following manual test works:

(using https for all domains)
0. open browser
1. open example.com
2. check main pane of identity popup
3. check security pane of identity popup
4. close popup
5. open mozilla.org
6. check main pane of identity popup
7. go back to example.com
8. check main pane + security pane
9. go forward to mozilla.org
10. check main pane + security pane
11. enter about:support in the url bar and navigate
12. check main pane (no security pane)
13. navigate to example.com and check both panes
Attachment #8944254 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1424264
No longer blocks: 1428839
Depends on: 1432015
Manual tests passed, and regressions tests too after I fixed a missing "return" in the subclass:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44533a362156dcc2e83c113ffc57919ae1da176a
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2044f8c205a9
Part 1 - Add a PanelView class using a base class shared with PanelMultiView. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/e89cb0fdb1fd
Part 2 - Move descriptionHeightWorkaround and some other methods to the PanelView class. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/933f482ecbc7
Part 3 - Use the PanelView class for knownViews and openViews. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/663987f8bdd9
Part 4 - Move keyboard navigation to the PanelView class. r=Gijs
Depends on: 1438815
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: