232327 - window._content mapped javascript function URL should be valid

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

In bug 215985, I slightly goofed.  |return| is not technically valid outside of
a JS function.  I'd like to change the JS in the URL we give for mapping
window._content to be valid.  Patch forthcoming.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch

Comment 2

[Mike Shaver (:shaver emeritus)]

At the risk of exposing my ignorance of this subsystem, why not just
javascript:this.content; // See ...
?

(Also, do you want to update the NS_LITERAL_STRING as well?)

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

The NS_LITERAL_STRING is the body of the function we're compiling and is thus
still correct.  Because we're compiling this as a function, I'd rather go with
the patch, which keeps it a function, but I could change it if so desired.

Comment 4

[Brendan Eich [:brendan]]

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

Wait, what's the deal here?  javascript: is a label (unused, therefore
useless).  What follows should be a statement.	An expression statement that
calls an unnamed function to return this.content will evaluate to the returned
value, but that value is then discarded -- there's no return from the body of
the function being created by my_context->CompileFunction.

What was wrong with "return this.content"? Just that venkman or something saw
that as invalid source?

/be

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Brendan, javascript: in this case is a URL scheme which runs the script
following the colon.  So in effect, we are pointing to an anonymous script file
with text "return this.content;"  As you can see from the URL I just added to
the bug, it is invalid in this context since there is no function to contain it.

Comment 6

[Mike Shaver (:shaver emeritus)]

The anonymous-function baggage would seem to work against readability, I think.
 If nothing else, the javascript: function should have the name _content, as the
compiled function does, but I think "this.content" is all that the reader really
wants or needs to see.

Comment 7

[Brendan Eich [:brendan]]

Sorry, I misread the patch (lack of sleep, new baby, blah blah).

It's still odd to have that kind of "filename".  What about shaver's point?  Why
isn't javascript:this.content enough?

/be

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

I must admit part of me wants to go with shaver's other suggestion of naming the
function _content (it even looks cooler than what I have currently), but I'll
certainly buy into readability.  It does work, and it is a valid point that the
user may not understand/care to see more than that.  New patch coming up.

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: As per shaver

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 140003 [details] [diff] [review]
As per shaver

Ok, lets give this a whirl.

Comment 11

[Mike Shaver (:shaver emeritus)]

Comment on attachment 140003 [details] [diff] [review]
As per shaver

I obviously like it. =)

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 140003 [details] [diff] [review]
As per shaver

What does the

// See " __FILE__,

mean now?  Is it just a holdover?  It looks odd, what with the "" and the
trailing comma.

Lose that and sr=me.

/be

Comment 13

[Mike Shaver (:shaver emeritus)]

The __FILE__ is string-concatenated by the preprocessor such that the URL looks like

javascript:this.content // See nsDOMClassInfo.cpp

when hovered-over, etc.

I might make that __FILE__ ":" __LINE__, in fact, but I'm not religious about it.

Go get some sleep. =)

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

What shaver said.  :-)

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in.