Closed Bug 1310150 Opened 8 years ago Closed 7 years ago

Document UITour-exposed data and methods in-tree

Categories

(Firefox :: Tours, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: Gijs, Assigned: MattN)

References

Details

Attachments

(1 file)

In bug 1305878 comment 28 Benjamin noted that there isn't in-tree documentation for UITour . While the bedrock JS lib is documented ( http://bedrock.readthedocs.io/en/latest/uitour.html ) the chrome code isn't.
I think the key point is being in-tree, it doesn't necessarily need to be rst. I propose using JSDoc for this since it also means we're documenting the API in-tree too rather than bedrock (which I've always found weird). For the purposes of bug 1305878 and bug 1343510, documenting the objects returned by getConfiguration are what's important for data review. Once showHeartbeat is removed from UITour.jsm there won't be any custom pings in the module.

I will push my patches to add JSDoc for the whole API including getConfiguration return values and a TC job to auto-generate the jsdoc output (which we could link to from gecko.readthedoc.io since readthedocs doesn't support HTML files for some reason).
Assignee: nobody → MattN+bmo
Blocks: 1343510
Status: NEW → ASSIGNED
Summary: Document UITour-exposed data and methods in an in-tree rst doc → Document UITour-exposed data and methods in-tree
Auto-generated jsdoc output using a TC job: https://public-artifacts.taskcluster.net/Xa83wskbRia2m0ZnW9uQTA/0/public/jsdoc/Mozilla.UITour.html

I'll file a separate bug for the TC jsdoc job which I don't think needs to block this from landing.
Blocks: 1352316
Once this gets reviewed I will make a pull request to remove the duplicated info from https://bedrock.readthedocs.io/en/latest/uitour.html and link to these auto-generated JSDocs.
(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #4)
> Once this gets reviewed I will make a pull request to remove the duplicated
> info from https://bedrock.readthedocs.io/en/latest/uitour.html and link to
> these auto-generated JSDocs.

This sounds great - thanks Matt

One request I would like to try and make - could we please include some info in the official docs about which version of Firefox each API method was introduced in? This has been really helpful for the mozorg team in the past to know when & where we can use things safely.
> One request I would like to try and make - could we please include some info in the official docs about which version of Firefox each API method was introduced in? This has been really helpful for the mozorg team in the past to know when & where we can use things safely.

Oh never mind - I see you've included this already (I missed it on first spot). Great work!
Comment on attachment 8853250 [details]
Bug 1310150 - Document UITour-exposed data and methods using jsdoc.

https://reviewboard.mozilla.org/r/125312/#review128064

I realize you're documenting a status quo, but of course reviewing the docs in some cases turns up surprises (at least for me!) about the implementation here. I've not marked up issues for those as they're not technically issues with the documentation, but it would be nice to take a moment to ensure we won't be caught off-guard by them at a later date.

::: browser/components/uitour/UITour-lib.js:143
(Diff revision 1)
>      }
>      _sendEvent("ping", data);
>    };
>  
> +  /**
> +   * Register a listener to observe all UITour notifications.

Can you document how to remove the listener? It looks like this is done with `observe(null)` or something, but it's not obvious.

Looking at this, I also find the method confusing in that it assigns to `notificationListener` and then uses `_notificationListener` (with added `_`). What's up with that? :-\

::: browser/components/uitour/UITour-lib.js:287
(Diff revision 1)
> +   *     style: 'primary',
> +   *     callback: confirmBtnCallback
> +   *   }
> +   * ];
> +   *
> +   * var icon = '//mozorg.cdn.mozilla.net/media/img/firefox/australis/logo.png';

This isn't an absolute URL. Doesn't it need a protocol? If not, please clarify this in the documentation above.

::: browser/components/uitour/UITour-lib.js:342
(Diff revision 1)
> +   *   "category":     "Firefox",
> +   *   "iconURL":      "https://addons.mozilla.org/_files/18066/preview_small.jpg?1241572934",
> +   *   "headerURL":    "https://addons.mozilla.org/_files/18066/1232849758499.jpg?1241572934",
> +   *   "name":         "Dark Fox",
> +   *   "author":       "randomaster",
> +   *   "footer":       "https://addons.mozilla.org/_files/18066/1232849758500.jpg?1241572934",
> +   *   "previewURL":   "https://addons.mozilla.org/_files/18066/preview.jpg?1241572934",
> +   *   "updateURL":    "https://versioncheck.addons.mozilla.org/en-US/themes/update-check/18066",
> +   *   "accentcolor":  "#000000",
> +   *   "header":       "https://addons.mozilla.org/_files/18066/1232849758499.jpg?1241572934",
> +   *   "version":      "1.0",
> +   *   "footerURL":    "https://addons.mozilla.org/_files/18066/1232849758500.jpg?1241572934",
> +   *   "detailURL":    "https://addons.mozilla.org/en-US/firefox/addon/dark-fox-18066/",
> +   *   "textcolor":    "#ffffff",
> +   *   "id":           "18066",
> +   *   "description":  "My dark version of the Firefox logo."

I'm worried this will get outdated, and about IP issues here. Can we just omit the explicit example?

::: browser/components/uitour/UITour-lib.js:532
(Diff revision 1)
> +   * @typedef {Object} Mozilla.UITour.Configuration.Sync
> +   * @property {Boolean} setup - Whether sync is setup
> +   * @property {Number} desktopDevices - Number of desktop devices
> +   * @property {Number} mobileDevices - Number of mobile devices

Does this still work? There were some shenanigans with the total number of devices in https://github.com/mozilla/normandy/pull/634#pullrequestreview-28973803 .

::: browser/components/uitour/UITour-lib.js:738
(Diff revision 1)
>      _sendEvent("toggleReaderMode");
>    };
>  
> +  /**
> +   * @param {String} pane - Pane to open/switch the preferences to.
> +   * Valid values match fragments on about:preferences:<ul>

Do we have tests for this that go orange if the panes change? I think Jared & mconley are coordinating a student project to change the in-content prefs...
Attachment #8853250 - Flags: review?(gijskruitbosch+bugs) → review+
I'm confused about the jsdoc/rst thing. Do the docs still end up at https://gecko.readthedocs.io/en/latest/ with everything else?
Flags: needinfo?(MattN+bmo)
Comment on attachment 8853250 [details]
Bug 1310150 - Document UITour-exposed data and methods using jsdoc.

https://reviewboard.mozilla.org/r/125312/#review128064

> Can you document how to remove the listener? It looks like this is done with `observe(null)` or something, but it's not obvious.
> 
> Looking at this, I also find the method confusing in that it assigns to `notificationListener` and then uses `_notificationListener` (with added `_`). What's up with that? :-\

> Can you document how to remove the listener? It looks like this is done with observe(null) or something, but it's not obvious.

Great idea. This method wasn't documented on bedrock so I couldn't just copy their docs.

> Looking at this, I also find the method confusing in that it assigns to notificationListener and then uses _notificationListener (with added _). What's up with that? :-\

`_notificationListener` is defined around line 75

> This isn't an absolute URL. Doesn't it need a protocol? If not, please clarify this in the documentation above.

Sorry, I copied the "absolute" mention from Bedrock, the URL actually gets resolved relative to the browser's URI.

> I'm worried this will get outdated, and about IP issues here. Can we just omit the explicit example?

Yeah, I had similar thoughts… this was the example from Bedrock. I think it can be useful to give a hint at what to expect but I'll condense it and make it more clear that there may be other properties.

> Does this still work? There were some shenanigans with the total number of devices in https://github.com/mozilla/normandy/pull/634#pullrequestreview-28973803 .

No idea, I was surprised to even see it in the code.

> Do we have tests for this that go orange if the panes change? I think Jared & mconley are coordinating a student project to change the in-content prefs...

Yeah, I thought about that. We don't need UITour code tests since we just pass through to `ChromeWindow.openPreferences()` and I don't think it's a big deal if this comment goes stale at that point. It's the reason why I mention to use the fragment from the URLs. I will change the comment to say that the list is an example.
(In reply to Benjamin Smedberg [:bsmedberg] from comment #8)
> I'm confused about the jsdoc/rst thing. Do the docs still end up at
> https://gecko.readthedocs.io/en/latest/ with everything else?

Yes, but at what level depends on how bug 1352316 goes. At a minimum it will be linked from there. Unfortunately RTD seems very inflexible so hosting it there may be a sub-par experience compared to the real JSDoc output.
Flags: needinfo?(MattN+bmo)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/77f5615436bb
Document UITour-exposed data and methods using jsdoc. r=Gijs
ok, I don't know a lot about the details of how `mach build-docs` works, but I'd like to keep this in the same flow if possible because we have automation and sheriffing to make sure doc updates don't break. gps is the mentor who helps me with doc generation stuff.
https://hg.mozilla.org/mozilla-central/rev/77f5615436bb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(In reply to Benjamin Smedberg [:bsmedberg] from comment #14)
> ok, I don't know a lot about the details of how `mach build-docs` works, but
> I'd like to keep this in the same flow if possible because we have
> automation and sheriffing to make sure doc updates don't break. gps is the
> mentor who helps me with doc generation stuff.

I will be a tier one TC job adjacent to the other docs and gps is involved in that bug so I think we're good.
No longer depends on: 1352664
Depends on: 1595953
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: