Closed Bug 232327 Opened 20 years ago Closed 20 years ago

window._content mapped javascript function URL should be valid

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: caillon, Assigned: caillon)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch (obsolete) — Splinter Review
Attachment #139994 - Flags: superreview?(brendan)
Attachment #139994 - Flags: review?(brendan)
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?)
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 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
Attachment #139994 - Flags: superreview?(brendan)
Attachment #139994 - Flags: superreview-
Attachment #139994 - Flags: review?(brendan)
Attachment #139994 - Flags: review-
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.
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.
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
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.
Attached patch As per shaverSplinter Review
Attachment #139994 - Attachment is obsolete: true
Comment on attachment 140003 [details] [diff] [review]
As per shaver

Ok, lets give this a whirl.
Attachment #140003 - Flags: superreview?(brendan)
Attachment #140003 - Flags: review?(shaver)
Comment on attachment 140003 [details] [diff] [review]
As per shaver

I obviously like it. =)
Attachment #140003 - Flags: review?(shaver) → review+
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
Attachment #140003 - Flags: superreview?(brendan) → superreview+
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. =)
What shaver said.  :-)
Checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.