Changeset View
Standalone View
source/scriptinterface/ScriptInterface.cpp
Show First 20 Lines • Show All 596 Lines • ▼ Show 20 Lines | |||||
} | } | ||||
JS::Value ScriptInterface::GetGlobalObject() const | JS::Value ScriptInterface::GetGlobalObject() const | ||||
{ | { | ||||
JSAutoRequest rq(m->m_cx); | JSAutoRequest rq(m->m_cx); | ||||
return JS::ObjectValue(*JS::CurrentGlobalOrNull(m->m_cx)); | return JS::ObjectValue(*JS::CurrentGlobalOrNull(m->m_cx)); | ||||
} | } | ||||
bool ScriptInterface::SetGlobal_(const char* name, JS::HandleValue value, bool replace, bool constant, bool enumerate) | bool ScriptInterface::SetGlobal_(const char* name, JS::HandleValue value, bool constant, bool enumerate, bool hotloadable, bool currentlyHotloading) | ||||
{ | { | ||||
JSAutoRequest rq(m->m_cx); | JSAutoRequest rq(m->m_cx); | ||||
JS::RootedObject global(m->m_cx, m->m_glob); | JS::RootedObject global(m->m_cx, m->m_glob); | ||||
if (!replace) | |||||
{ | |||||
bool found; | bool found; | ||||
if (!JS_HasProperty(m->m_cx, global, name, &found)) | if (!JS_HasProperty(m->m_cx, global, name, &found)) | ||||
return false; | return false; | ||||
if (found) | if (found) | ||||
{ | { | ||||
JS::Rooted<JSPropertyDescriptor> desc(m->m_cx); | |||||
if (!JS_GetOwnPropertyDescriptor(m->m_cx, global, name, &desc)) | |||||
return false; | |||||
wraitii: What does it mean to return false here? We don't replace a property we inherited from a parent… | |||||
ItmsAuthorUnsubmitted Done Inline ActionsHere returning false is: there is a bug. I can't prove this is never supposed to happen, so I chose to pass the result over, instead of using ENSURE. Itms: Here returning false is: there is a bug. I can't prove this is never supposed to happen, so I… | |||||
wraitiiUnsubmitted Not Done Inline ActionsMaybe at least a logError or a comment // shouldn't happen then? This reads like a "normal" exit. wraitii: Maybe at least a logError or a comment `// shouldn't happen` then? This reads like a "normal"… | |||||
elexisUnsubmitted Not Done Inline Actions
ENSURE are only to help developers, the function should work correctly for all provided input values if we can make it so.
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_HasProperty says
So it could happen if the specs change, during an SM upgrade for instance. elexis: > I can't prove this is never supposed to happen
`ENSURE` are only to help developers, the… | |||||
if (desc.isReadonly()) | |||||
{ | |||||
if (!currentlyHotloading) | |||||
{ | |||||
JS_ReportError(m->m_cx, "SetGlobal \"%s\" called multiple times", name); | JS_ReportError(m->m_cx, "SetGlobal \"%s\" called multiple times", name); | ||||
elexisUnsubmitted Not Done Inline ActionsThe error message is omissive / misleading, it's possible to call SetGlobal on a variable multiple times, it should only not be possible for PERSISTENT ones. It should actually be possible for READONLY ones, no? (Which would mean currentlyHotloading not being supposed to be this function?) elexis: The error message is omissive / misleading, it's possible to call SetGlobal on a variable… | |||||
ItmsAuthorUnsubmitted Done Inline ActionsNo, it's not possible either for ReadOnly ones. SpiderMonkey is consistent with JavaScript, so JS_DefineProperty will do the same as JS_SetProperty when the property is already existing (and additionally it will set attributes, getters and setters) and will fail if the existing property is ReadOnly. Itms: No, it's not possible either for ReadOnly ones. SpiderMonkey is consistent with JavaScript, so… | |||||
return false; | return false; | ||||
} | } | ||||
if (desc.isPermanent()) | |||||
{ | |||||
JS_ReportError(m->m_cx, "The global \"%s\" cannot be modified during hotload. Please restart the engine.", name); | |||||
wraitiiUnsubmitted Not Done Inline ActionsNice error message ? wraitii: Nice error message ? | |||||
ItmsAuthorUnsubmitted Done Inline ActionsI'm a bit unsure about "please restart" , because I don't want to assume that the user wants to do that. But I don't think that could happen in a different situation. Itms: I'm a bit unsure about "please restart" , because I don't want to assume that the user wants to… | |||||
StanUnsubmitted Not Done Inline ActionsIsn't it can't be modified at runtime ? During hotload would imply the variable changes states during hotloading, while it's usually either after or before. Stan: Isn't it can't be modified at runtime ? During hotload would imply the variable changes states… | |||||
wraitiiUnsubmitted Not Done Inline ActionsYou can always add "please restart the engine to change it". wraitii: You can always add "please restart the engine to change it". | |||||
elexisUnsubmitted Not Done Inline ActionsWhy Please restart the engine.? We don't have this recommendation for any other error message. If someone choses to call SetGlobal with hotloadable = false, but then the developer hotloads that page, then should it warn, should it error out, or should it just not hotload the non-hotloadable global? elexis: Why `Please restart the engine.`? We don't have this recommendation for any other error message. | |||||
ItmsAuthorUnsubmitted Done Inline ActionsHum I made a mistake here, I wanted to warn the user that the non-hotloadable global won't change until the next engine restart, but return true. I shouldn't have copy-pasted the JS_ReportError; return false; from the rest of the function. If I remove the possibility to create non-hotloadable globals, this should not happen, so this would be an error, and I could remove the out-of-place "Please restart the engine". Itms: Hum I made a mistake here, I wanted to warn the user that the non-hotloadable global won't… | |||||
elexisUnsubmitted Not Done Inline ActionsI mean Please restart the engine isn't shown either for other gamebreaking errors, it's just implied that it's pronounced dead and open to the user to spawn a new instance or continue in the broken state. elexis: I mean `Please restart the engine` isn't shown either for other gamebreaking errors, it's just… | |||||
return false; | |||||
} | |||||
LOGMESSAGE("Hotloading new value for global \"%s\".", name); | |||||
ENSURE(JS_DeleteProperty(m->m_cx, global, name)); | |||||
} | |||||
} | } | ||||
uint attrs = 0; | uint attrs = 0; | ||||
if (constant) | if (constant) | ||||
{ | |||||
if (hotloadable) | |||||
attrs |= JSPROP_READONLY; | |||||
else | |||||
attrs |= JSPROP_READONLY | JSPROP_PERMANENT; | attrs |= JSPROP_READONLY | JSPROP_PERMANENT; | ||||
} | |||||
ItmsAuthorUnsubmitted Done Inline ActionsThis is the core of the patch. Itms: This is the core of the patch. | |||||
elexisUnsubmitted Not Done Inline ActionsBut does SetGlobal have to be educated about hotloading? Can't the hotloading code just account for permanent properties by not replacing these (and warning the developers that permanent properties won't be hotloaded)? elexis: But does SetGlobal have to be educated about hotloading? Can't the hotloading code just account… | |||||
ItmsAuthorUnsubmitted Done Inline ActionsYes, that was the idea, but currently every global that is permanent should be hotloadable (like for instance scripted component protoypes, or Commands or Cheats, ...). Either SetGlobal is educated about hotloading, or
I'm going to make hotloadable = true by default, so SetGlobal will still be educated about whether we are currently hotloading, but some of the complexity will be removed ? Itms: Yes, that was the idea, but currently every global that is permanent should be hotloadable… | |||||
elexisUnsubmitted Not Done Inline ActionsIf readonly, attrs |= JSPROP_READONLY elexis: If readonly, attrs |= JSPROP_READONLY
If permanent, attrs |= JSPROP_PERMANENT?
(It's daring but… | |||||
if (enumerate) | if (enumerate) | ||||
attrs |= JSPROP_ENUMERATE; | attrs |= JSPROP_ENUMERATE; | ||||
return JS_DefineProperty(m->m_cx, global, name, value, attrs); | return JS_DefineProperty(m->m_cx, global, name, value, attrs); | ||||
elexisUnsubmitted Not Done Inline ActionsIf a property with that name is already defined, if that property is PERSISTENT but not READONLY, then this call will return an error. (Probably not bad, but why are the other cases captured if not this one as well.) elexis: If a property with that name is already defined, if that property is PERSISTENT but not… | |||||
} | } | ||||
bool ScriptInterface::SetProperty_(JS::HandleValue obj, const char* name, JS::HandleValue value, bool constant, bool enumerate) const | bool ScriptInterface::SetProperty_(JS::HandleValue obj, const char* name, JS::HandleValue value, bool constant, bool enumerate) const | ||||
{ | { | ||||
JSAutoRequest rq(m->m_cx); | JSAutoRequest rq(m->m_cx); | ||||
uint attrs = 0; | uint attrs = 0; | ||||
if (constant) | if (constant) | ||||
attrs |= JSPROP_READONLY | JSPROP_PERMANENT; | attrs |= JSPROP_READONLY | JSPROP_PERMANENT; | ||||
▲ Show 20 Lines • Show All 517 Lines • Show Last 20 Lines |
What does it mean to return false here? We don't replace a property we inherited from a parent prototype?