942393 - Document CustomizableUI

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

I just helped a fellow in IRC who is working on a toolbar library for the SDK, and it just rung home how non-existent our documentation for CustomizableUI is.

We really really really need to write that stuff.

Comment 1

[:Gijs (he/him)]

Yes.

Comment 2

[:Gijs (he/him)]

Cc'ing Mike so he can keep track of our progress here, as he showed interest in getting these docs ready.

I started with an extremely basic outline, but it's the weekend and I need to run out now. Link-dropping:

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm

(also, we should probably have some tutorials of sorts, but one thing at a time...)

Comment 3

[Matthew N. [:MattN]]

I think the most important documentation is JavaDoc-like comments in the code itself. They can be duplicated on MDN if necessary.

Comment 4

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (catching up on reviews) from comment #3)
> I think the most important documentation is JavaDoc-like comments in the
> code itself. They can be duplicated on MDN if necessary.

I strongly disagree. I don't think anyone but ourselves will use our own comments. They are not helpful for add-on authors and other API-consumers outside of the people who actually write/wrote CustomizableUI. Duplicating them would only lead to inconsistencies between the two copies, as there are no extant tools to sync stuff like this, and blocking the writing of docs on the creation of such a tool (which has never been made despite having been requested over and over again in the past forever years) will mean the docs simply won't get written in time.

I think we should go ahead and document stuff on MDN as soon as reasonably possible.

Comment 5

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Matthew N. [:MattN] (catching up on reviews) from comment #3)
> > I think the most important documentation is JavaDoc-like comments in the
> > code itself. They can be duplicated on MDN if necessary.
> 
> I strongly disagree. I don't think anyone but ourselves will use our own
> comments. They are not helpful for add-on authors and other API-consumers
> outside of the people who actually write/wrote CustomizableUI.

This isn't true as this is how it works for many APIs already and this is how it has been at Mozilla for a long time. Often documentating APIs on MDN is a matter of copying the relevant information from the source files.   Examples and long-form text don't need to be in the source files but information about arguments and return values for common functions should be since they are more likely to be updated when the function signature changes. This is one of the main reasons why MDN almost always links to MXR at the top of API pages.

Note, I wasn't suggesting anything not be documented on MDN, just that function information be in the tree.

Comment 6

[Matthew N. [:MattN]]

I can try start on this on the weekend but I'll definitely have questions for people who know CustomizableUI better.

Comment 7

[:Gijs (he/him)]

Taking per discussion on mail + IRC

Comment 8

[:Gijs (he/him)]

Attachment: fix issues with events in Australis' CustomizableUI discovered when documenting,

So, I noticed a few things while documenting. One, onWidgetReset and onAreaReset will fire for each buildArea, so they should be passing around DOM nodes, not generic widget/area IDs, which won't tell consumers what's going on. Second, onWidgetBefore/AfterDOMChange events fired through CustomizableUI's onWidgetRemoved should pass the node's actual container, and use that when removing the node. For the overflowable toolbar, that will *not* be the area's customizationTarget, so I think the current code is buggy, and I fixed it. Jared, do these make sense?

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8343953 [details] [diff] [review]
fix issues with events in Australis' CustomizableUI discovered when documenting,

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

These should probably be pulled out to a separate bug which will be easier to track in the future.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +813,5 @@
>      }
>    });
>  }
>  #endif
> +#endif

Thanks for adding a newline here.

Comment 10

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #9)
> Comment on attachment 8343953 [details] [diff] [review]
> fix issues with events in Australis' CustomizableUI discovered when
> documenting,
> 
> Review of attachment 8343953 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These should probably be pulled out to a separate bug which will be easier
> to track in the future.

Good point. Filed bug 947867.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8343953 [details] [diff] [review]
fix issues with events in Australis' CustomizableUI discovered when documenting,

Landed this as part of bug 947867, so obsoleting here.

Comment 12

[:Gijs (he/him)]

First set of in-JSM docs:

remote:   https://hg.mozilla.org/integration/fx-team/rev/f0a47eacfb9d

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Luís Miguel [:quicksaver] (not reading bugmail)]

>   1.247 +   * Callers should ensure that NO MATTER WHAT they call endBatchUpdate once
>   1.248 +   * for each call to endBatchUpdate, even if there are exceptions in the
>   1.249 +   * code in the batch update.

It should read "they call endBatchUpdate for each call to beginBatchUpdate" I think.

Comment 15

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #14)
> >   1.247 +   * Callers should ensure that NO MATTER WHAT they call endBatchUpdate once
> >   1.248 +   * for each call to endBatchUpdate, even if there are exceptions in the
> >   1.249 +   * code in the batch update.
> 
> It should read "they call endBatchUpdate for each call to beginBatchUpdate"
> I think.

Yes! Brain...fried...so many docs...

Good catch, will fix that in an upcoming commit. :-)

Comment 16

[:Gijs (he/him)]

More docs:

remote:   https://hg.mozilla.org/integration/fx-team/rev/6c439db7f5c9


This has every public method / getter of CustomizableUI documented (bar the constants at the top), and the mistake listed in comment #14.

I'll fix the constants at the top and then sync the docs from the code to MDN later today.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c439db7f5c9

Comment 18

[:Gijs (he/him)]

Apologies for the delay here. Constants:

remote:   https://hg.mozilla.org/integration/fx-team/rev/70c67626470c

Now trying to stick the rest of this on MDN.

Comment 19

[:Gijs (he/him)]

Removed TYPE_BUTTON per rs from jaws on IRC, nitted something else:

remote:   https://hg.mozilla.org/integration/fx-team/rev/ff1038cbc195


I've started moving this to MDN but it's more painful than I thought it was going to be, so progress has been slow. I anticipate I'll gradually complete this over this week.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/70c67626470c
https://hg.mozilla.org/mozilla-central/rev/ff1038cbc195

Comment 21

[:Gijs (he/him)]

Tiny nits:

remote:   https://hg.mozilla.org/integration/fx-team/rev/838f8a3aecec

I've completed adding the docs from the code to MDN. I've requested editorial review on the main page. I think we're done here.

Comment 22

[:Gijs (he/him)]

(FWIW, I'm sure we could write more tutorial-wise on how to migrate code or do X with CustomizableUI, but I don't think that needs to happen in this bug, and certainly not with this high a priority)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/838f8a3aecec

Comment 24

[Ryan VanderMeulen [:RyanVM]]

(In reply to :Gijs Kruitbosch from comment #21)

Please hold off on resolving bugs unless you're landing on m-c directly.