661423 - api-utils: runtime module should be documented

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: Documentation, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

There is no MD file for this module and it can be very handy for developers to use this one to guess what is the current OS.

Comment 1

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

I'll give this a shot. Should hopefully have time tonight to get a patch up.

Comment 2

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

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/180

Something like this?

Comment 3

[Alexandre Poirot [:ochameau]]

Thanks Wes for taking this bug.
Seems good except that OS is not necessary in full capitals.
So It is: WINNT, Linux or Darwin.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 536816 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/180

Looks pretty good, just a couple of issues.  Let's get Will to take a look at it as well.

Comment 5

[Will Bamberg [:wbamberg]]

Comment on attachment 536816 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/180

Thanks for this Wes. I never even knew this module existed! I've commented on the pull request. I think my main comments, apart from a bit of formatting, are to explain a bit more about why add-on developers might care about these different properties.

Comment 6

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

I just committed some changes to address most of the comments/nits that were raised. (Not quite sure how the pull request stuff works on Github, do I have to do anything to add the new changes to the pull request or is that updated automatically?)

My only real question is how we/you want to go forward with the e10s process type section. Clean up the documentation to match the module or change the module to match the documentation?

Beyond that, I personally can't really explain why some of these things would be useful for add-on developers, since I don't personally understand what these things would be useful for. (Like the XPCOM ABI section: is that even relevant to SDK devs?)

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> Beyond that, I personally can't really explain why some of these things
> would be useful for add-on developers, since I don't personally understand
> what these things would be useful for. (Like the XPCOM ABI section: is that
> even relevant to SDK devs?)

Note: the low-level modules in api-utils were are not intended for use by addon developers; they are intended for use by API developers building the high-level modules in addon-kit.

That could change in the future as we start to prioritize transitioning traditional addon developers to the new platform.

And in some cases, especially with modules like runtime that were developed early in the product's lifetime, they may not have been designed for a particular use and with a particular target audience in mind, which is why it's hard to document their utility: we didn't and still don't know actually know how and to whom they're useful.

In such cases, it's probably still better to have some documentation for them that provides basic descriptions of their symbols and references sources of additional information.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(automatic reprioritization of 1.0 bugs)

Comment 9

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

I think landing this now would be worth it, although Eddy would know if the process types would still be relevant once we get e10s. (As it stands, it's not really important, since everything's in the chrome process right now.)

It doesn't mention the new addon process type that Eddy added.

I'd say land what we have now and file a followup to clean up the process types?

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Will is back; let's get his opinion.

Comment 11

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

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 12

[Will Bamberg [:wbamberg]]

(In reply to Wes Kocher (:KWierso) (Jetpack Bugmaster) from comment #9)
> I think landing this now would be worth it, although Eddy would know if the
> process types would still be relevant once we get e10s. (As it stands, it's
> not really important, since everything's in the chrome process right now.)
> 
> It doesn't mention the new addon process type that Eddy added.
> 
> I'd say land what we have now and file a followup to clean up the process
> types?

Yes, let's do that. I agree that this is much better than the nothing we currently have.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Will: can you do the honors?

Comment 14

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

I don't think this ever landed.

Comment 15

[Will Bamberg [:wbamberg]]

Sorry, no, I missed that :(. I"ll land it tomorrow.

Comment 16

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

Attachment: document runtime module

This is a patch version of that really mangled pull request.

It still doesn't mess with the e10s part of it, because I still don't know what's happening in there.

I'm asking for another review because so much time has passed, and I had to recreate the patch partially from memory because the pull request was broken badly.

Comment 17

[Will Bamberg [:wbamberg]]

Comment on attachment 567868 [details] [diff] [review]
document runtime module

The content looks good!

However, the patch doesn't apply cleanly:
- I think it expects there to be an empty runtime.md file there? At any rate, if I create one then it does apply
- it gives me a bunch of warnings about trailing whitespace:

  (jetpack-sdk)~/mozilla/jetpack-sdk(test) > git apply runtime1.patch
  runtime1.patch:16: trailing whitespace.
    This value is `true` if Firefox was started in safe mode, 
  runtime1.patch:23: trailing whitespace.
    `"WINNT"`, `"Darwin"`, or `"Linux"`. See [OS_TARGET][OS_TARGET] 
  runtime1.patch:25: trailing whitespace.
  
  runtime1.patch:72: trailing whitespace.
    A string identifying the [ABI][ABI] of the current processor and compiler   vtable. 
  runtime1.patch:73: trailing whitespace.
    This string takes the form \<`processor`\>-\<`compilerABI`\>, 
  warning: squelched 1 whitespace error
  warning: 6 lines add whitespace errors.

Comment 18

[Will Bamberg [:wbamberg]]

-> https://github.com/mozilla/addon-sdk/commit/b43779dd70b10b4bf778dffe1ab825b462ec431e