951633 - Drop the <xul:menulist> support for WidgetMethods

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Victor Porof [:vporof][:vp]]

Attachment: widget-no-label.patch

Keeping this around is a pain in the ass. There's literally no benefit from doing so, and we're not using it for anything except a potential ChromeGlobals view which isn't even implemented yet and is only for the browser debugger.

Removing this support (and, inherently not expecting strings anymore when pushing an item into a widget) will significantly reduce code complexity. It will also allow us to grow the API without worrying about breaking <xul:menulist>s.

Here's a patch. It does the following:
* makes Items always expect an nsIDOMNode
* makes WidgetMethods's push() always expect an nsIDOMNode
* removes the idea of "labels" and "descriptions"; Items are only identified by a "value" and a single nsIDOMNode; the use of "labels" and "descriptions" was removed from everywhere
* removes the idea of "relaxed" pushes; you can't push an item anymore that doesn't have a corresponding node; values are still optional, and it doesn't matter anymore whether they're supplied or not
* adds ensureElementIsVisible() as an optional widget method, called when appropriate
* consolidates the "headerText" and "emptyText" widget setters, sharing their implementation
* moves the debugger's ListWidget in /shared, renaming it to SimpleListWidget
* removes the ChromeGlobals view
* removes unused code
* updates documentation

Comment 1

[Victor Porof [:vporof][:vp]]

Comment on attachment 8349360 [details] [diff] [review]
widget-no-label.patch

This might seems scary at first, but it's largely mechanical and does everything mentioned in comment 0, and nothing more. Should be easy to review.

Comment 2

[Victor Porof [:vporof][:vp]]

Here's a diff of the changes in the tutorial gist:
https://gist.github.com/victorporof/5749386/revisions

Read the gist here:
https://gist.github.com/victorporof/5749386

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: widget-no-label-test.patch

Fix tests.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8349360 [details] [diff] [review]
widget-no-label.patch

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

I don't feel like I really undrestand the zen of WidgetMethods well enough to give a good review here rather than just looking for typos in code or whatever. Also, with a patch of this size, try pushes increase confidence :)

Perhaps someone else can review these patches.

Comment 5

[Victor Porof [:vporof][:vp]]

Comment on attachment 8349360 [details] [diff] [review]
widget-no-label.patch

:(

Deferring this to Panos. Please read comment 0 for justification and comment 2 for what actually changes as seen from the outside world. This bug is about removing unnecessary complexity which might have made sense at some point, but today it's just cruft. Changes are mostly mechanical.

I don't know who else I can ask for review.

Panos, there is no rush in reviewing this.

Comment 6

[Victor Porof [:vporof][:vp]]

Comment on attachment 8349382 [details] [diff] [review]
widget-no-label-test.patch

I'll add a try run shortly.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8349360 [details] [diff] [review]
widget-no-label.patch

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

Nice cleanups!

::: browser/devtools/shared/widgets/BreadcrumbsWidget.jsm
@@ -140,5 @@
> -      if (this._selectedItem &&
> -        // Sometimes the this._list doesn't have some methods, because the node
> -        // is accessed while it's not visible or has been removed from the DOM.
> -        // Avoid outputing an exception to the console in those cases.
> -        this._list.ensureElementIsVisible) {

I guess this is no longer the case?

::: browser/devtools/shared/widgets/ViewHelpers.jsm
@@ +567,5 @@
>   * Language:
>   *   - An "item" is an instance of an Item.
>   *   - An "element" or "node" is a nsIDOMNode.
>   *
> + * The supplied widget can be any object interfacing the following methods:

interfacing -> implementing

@@ +580,5 @@
>   *   - function removeAttribute(aName:string)
>   *   - function addEventListener(aName:string, aCallback:function, aBubbleFlag:boolean)
>   *   - function removeEventListener(aName:string, aCallback:function, aBubbleFlag:boolean)
>   *
> + * Optional methods that can be interfaced by the widget:

interfaced -> implemented

@@ +601,5 @@
>    set widget(aWidget) {
>      this._widget = aWidget;
>  
> +    // Can't use a WeakMap for itemsByValue because keys are strings, and
> +    // itemsByElement needs to be iterable.

I think a slight rewording would make this comment more clear:

"Can't use a WeakMap for _itemsByValue because keys are strings, and can't use one for _itemsByElement either, since it needs to be iterable."

Better also mention _stagedItems, too.

@@ +660,1 @@
>        delete aOptions.index;

I don't think we modify the options object after creation, so you might as well assign a null value to avoid falling into dictionary mode.

@@ +1396,5 @@
>     *        The item for which to verify eligibility.
>     * @return boolean
>     *         True if the item is eligible, false otherwise.
>     */
>    isEligible: function(aItem) {

If this is still supported, then maybe it shouldn't be removed from the docs? Aren't widgets supposed to override this in some cases?

::: browser/themes/linux/devtools/debugger.css
@@ +106,5 @@
>  /* Classic stack frames view */
>  
>  .dbg-classic-stackframe {
>    display: block;
> +  padding: 0px 4px;

Don't we omit the unit from zeros?

::: browser/themes/linux/devtools/netmonitor.css
@@ +312,5 @@
>  
>  /* SideMenuWidget */
>  
> +.side-menu-widget-item-contents {
> +  padding: 0px;

-px

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8349382 [details] [diff] [review]
widget-no-label-test.patch

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

::: browser/devtools/debugger/test/browser_dbg_stack-02.js
@@ +40,1 @@
>      TAB_URL, "Oldest frame name is mirrored correctly.");

This invocation of is() has 4 arguments.

@@ +47,1 @@
>      TAB_URL, "Newest frame name is mirrored correctly.");

This one too.

Comment 9

[Rob Campbell [:rc] (:robcee)]

lookit you guys writing and reviewing patches. Marking P3.

Comment 10

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/eaa6d472d0d4
https://hg.mozilla.org/integration/fx-team/rev/11ad00252b95

Comment 11

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/01753b4a5056
https://hg.mozilla.org/integration/fx-team/rev/8fee8dd350fd

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01753b4a5056
https://hg.mozilla.org/mozilla-central/rev/8fee8dd350fd
https://hg.mozilla.org/mozilla-central/rev/eaa6d472d0d4
https://hg.mozilla.org/mozilla-central/rev/11ad00252b95