662231 - Re-disallow embeddings reaching into our guts (jsnum.h)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Apparently (or so I understand it, at a skim, not investigating thoroughly right now) bug 548205 deliberately removed jsnum.h from INSTALLED_HEADERS.  In order to make a patch compile, I (re?)added it in bug 636079.  Undo that, probably by moving the offending method to an inlines file which doesn't need jsnum.h installed.

More important, however, we should add a comment by INSTALLED_HEADERS explaining the rules for when something should or shouldn't be there.  I didn't know it was bad to have jsnum.h here, but if it is, we should explain when files should and shouldn't be added to that list, to avoid future mistakes.

Comment 1

[Mike Hommey [:glandium]]

Your concern is somehow related to bug 554088. We should only install public headers, and there should be a clear distinction between public and private APIs, which (sadly) is far from the case at the moment.

More to the point, the problem is somehow amplified by the fact that we usually rely on being able to include private bits without extra changes in Makefiles, and that relies on headers being copied in dist/include. The fact that all headers from there basically end up in the SDK is a problem.

So I guess a dev-platform discussion should be raised on the subject of private headers vs. public headers and how we want them to be dealt with (considering the implication this would have on what developers would need to do when they add new headers)

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch

I have no idea what the right comment is to explain when things can and can't be added to INSTALLED_HEADERS, so I'm going to hope fear will keep the loca^H^H^H^developers in line.  Fear of this comment.

Comment 3

[Jim Blandy :jimb]

Comment on attachment 538065 [details] [diff] [review]
Patch

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

::: js/src/Makefile.in
@@ +190,5 @@
> +##########################################################################
> +# Changes to internal header files, used externally, massively slow down #
> +# browser builds.  Don't add new files here unless you know what you're  #
> +# doing!                                                                 #
> +##########################################################################

I think you should drop the scary border, but the content seems fine.

Comment 4

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/9374da0a2b1a

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/9374da0a2b1a