Closed Bug 1441284 Opened 6 years ago Closed 6 years ago

Remove various properties of the PanelMultiview object and start simplifying the cleanup process

Categories

(Firefox :: Toolbars and Customization, enhancement, P1)

Product:
Firefox
Component:
Toolbars and Customization
Version:
59 Branch
Type:
enhancement

Tracking

()

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

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(8 files)

Bug 1441284 - Part 1 - Remove the "current" property.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1441284 - Part 2 - Remove the "_currentSubView" property.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1441284 - Part 3 - Remove the "showingSubView" property.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1441284 - Part 4 - Remove redundant calls before _moveOutKids and simplify the function.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1441284 - Part 5 - Remove the "_panelViewCache" property.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1441284 - Part 6 - Do not move out subviews when the window is closing.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1441284 - Part 7 - Remove the "_ephemeral" property.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1441284 - Part 8 - Remove the "_mainView" and "_mainViewId" properties.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
There are now some easy wins and simplifications that can be done in the PanelMultiView object. This separate bug gets them out of the way, so they don't need to be handled as drive-by cleanup during or after bug 1428839.
I'm not sure how to test the only consumer of the showSubView property in part 3, as I found closemenu="single" only on context menus.

https://dxr.mozilla.org/mozilla-central/search?q=closemenu%3D%22single%22
https://dxr.mozilla.org/mozilla-central/search?q=(%22closemenu%22

I'm wondering if this is effectively dead code that we can remove together with the property:

https://dxr.mozilla.org/mozilla-central/rev/7208b6a7b11c3ed8c87a7f17c9c30a8f9583e791/browser/components/customizableui/CustomizableUI.jsm#1826-1833
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8954153 [details]
Bug 1441284 - Part 8 - Remove the "_mainView" and "_mainViewId" properties.

https://reviewboard.mozilla.org/r/223310/#review229358


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/customizableui/PanelMultiView.jsm:690
(Diff revision 1)
> +    }
> +
> +    let mainViewNode = this.document.getElementById(mainViewId);
> +    if (!mainViewNode) {
> +      Cu.reportError(new Error(`Main view ${mainViewId} doesn't exist.`));
> +      return;

Error: Async method '_showmainview' expected a return value. [eslint: consistent-return]
Comment on attachment 8954153 [details]
Bug 1441284 - Part 8 - Remove the "_mainView" and "_mainViewId" properties.

https://reviewboard.mozilla.org/r/223310/#review229364


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/customizableui/PanelMultiView.jsm:690
(Diff revision 2)
> +    }
> +
> +    let mainViewNode = this.document.getElementById(mainViewId);
> +    if (!mainViewNode) {
> +      Cu.reportError(new Error(`Main view ${mainViewId} doesn't exist.`));
> +      return;

Error: Async method '_showmainview' expected a return value. [eslint: consistent-return]
Comment on attachment 8954146 [details]
Bug 1441284 - Part 1 - Remove the "current" property.

https://reviewboard.mozilla.org/r/223296/#review229506
Attachment #8954146 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8954147 [details]
Bug 1441284 - Part 2 - Remove the "_currentSubView" property.

https://reviewboard.mozilla.org/r/223298/#review229508
Attachment #8954147 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8954149 [details]
Bug 1441284 - Part 4 - Remove redundant calls before _moveOutKids and simplify the function.

https://reviewboard.mozilla.org/r/223302/#review229512
Attachment #8954149 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8954148 [details]
Bug 1441284 - Part 3 - Remove the "showingSubView" property.

https://reviewboard.mozilla.org/r/223300/#review229510

Yeah, I think this can just go away entirely, together with the closemenu=single stuff.
Attachment #8954148 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8954150 [details]
Bug 1441284 - Part 5 - Remove the "_panelViewCache" property.

https://reviewboard.mozilla.org/r/223304/#review229514
Attachment #8954150 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8954151 [details]
Bug 1441284 - Part 6 - Do not move out subviews when the window is closing.

https://reviewboard.mozilla.org/r/223306/#review229516
Attachment #8954151 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8954152 [details]
Bug 1441284 - Part 7 - Remove the "_ephemeral" property.

https://reviewboard.mozilla.org/r/223308/#review229518
Attachment #8954152 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8954153 [details]
Bug 1441284 - Part 8 - Remove the "_mainView" and "_mainViewId" properties.

https://reviewboard.mozilla.org/r/223310/#review229520

The diff stat here is +18/-12 . Why is this an improvement?
Attachment #8954153 - Flags: review?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #26)
> The diff stat here is +18/-12 . Why is this an improvement?

Do you want me to remove the error reporting?
Flags: needinfo?(gijskruitbosch+bugs)
(We can also just assume that the mainViewId attribute is always set, and not check for null.)
(In reply to :Paolo Amadini from comment #27)
> (In reply to :Gijs from comment #26)
> > The diff stat here is +18/-12 . Why is this an improvement?
> 
> Do you want me to remove the error reporting?

I guess. I don't expect anyone implementing anything with this module will run into this case and not notice (opening the panel just won't work). And there are other things for which you need knowledge of the API to get proper behaviour (ie using the static methods to open panels etc.). So I don't think the error reporting gets us much here.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8954153 [details]
Bug 1441284 - Part 8 - Remove the "_mainView" and "_mainViewId" properties.

https://reviewboard.mozilla.org/r/223310/#review229722
Attachment #8954153 - Flags: review?(gijskruitbosch+bugs) → review+
Sorry, I need to think about the part 3 bit when it's not midnight, so that review will have to wait. There's still closemenu=single instances in the context menu which is still inside the <panel>. So the loop here used to catch those and now won't. They're meant to close the context menu but not the enclosing panel, and there are so many early returns/breaks/conditions here that I can't work out whether your patch changes behaviour or not.
Comment on attachment 8954148 [details]
Bug 1441284 - Part 3 - Remove the "showingSubView" property.

https://reviewboard.mozilla.org/r/223300/#review229714

r=me

Seems closemenu=single for the context menu case is handled in the `isInteractive` check anyway.

::: browser/components/customizableui/CustomizableUI.jsm:1805
(Diff revision 3)
>      // We can't use event.target because we might have passed a panelview
>      // anonymous content boundary as well, and so target points to the
>      // panelmultiview in that case. Unfortunately, this means we get
>      // anonymous child nodes instead of the real ones, so looking for the
>      // 'stoooop, don't close me' attributes is more involved.

Is this still true? I expect not...
Attachment #8954148 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #41)
> ::: browser/components/customizableui/CustomizableUI.jsm:1805
> (Diff revision 3)
> >      // We can't use event.target because we might have passed a panelview
> >      // anonymous content boundary as well, and so target points to the
> >      // panelmultiview in that case. Unfortunately, this means we get
> >      // anonymous child nodes instead of the real ones, so looking for the
> >      // 'stoooop, don't close me' attributes is more involved.
> 
> Is this still true? I expect not...

I believe this may be still true if the button is located inside any other XBL binding. Even though we may not have any other remaining cases, I've updated the comment accordingly.
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1687a0f1db4c
Part 1 - Remove the "current" property. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/a81851454a36
Part 2 - Remove the "_currentSubView" property. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/00d855c4d71d
Part 3 - Remove the "showingSubView" property. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/63cfc3426678
Part 4 - Remove redundant calls before _moveOutKids and simplify the function. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4b46b5edc28
Part 5 - Remove the "_panelViewCache" property. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e819acb990a
Part 6 - Do not move out subviews when the window is closing. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd1250f64280
Part 7 - Remove the "_ephemeral" property. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0ecce89ac1a
Part 8 - Remove the "_mainView" and "_mainViewId" properties. r=Gijs
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: