Closed Bug 942393 Opened 6 years ago Closed 6 years ago

Document CustomizableUI

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: mconley, Assigned: Gijs)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed, Whiteboard: [Australis:P2])

Attachments

(1 obsolete file)

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.
Yes.
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...)
I think the most important documentation is JavaDoc-like comments in the code itself. They can be duplicated on MDN if necessary.
(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.
(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.
I can try start on this on the weekend but I'll definitely have questions for people who know CustomizableUI better.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Hardware: x86 → All
Taking per discussion on mail + IRC
Assignee: MattN+bmo → gijskruitbosch+bugs
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?
Attachment #8343953 - Flags: review?(jaws)
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.
Attachment #8343953 - Flags: review?(jaws) → review+
(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.
Depends on: 947867
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.
Attachment #8343953 - Attachment is obsolete: true
First set of in-JSM docs:

remote:   https://hg.mozilla.org/integration/fx-team/rev/f0a47eacfb9d
https://hg.mozilla.org/mozilla-central/rev/f0a47eacfb9d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [Australis:P2] → [Australis:P2][leave open]
Target Milestone: Firefox 29 → ---
>   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.
(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. :-)
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.
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.
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.
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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][leave open] → [Australis:P2]
(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)
(In reply to :Gijs Kruitbosch from comment #21)

Please hold off on resolving bugs unless you're landing on m-c directly.
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.