sugarlabs / sugar-web

Components for Sugar web activities
Apache License 2.0
13 stars 32 forks source link

env.getEnvironment return {} in standalone mode. #101

Closed rogemita closed 10 years ago

rogemita commented 10 years ago

NOTE: env.getEnvironment single-thread only

rogemita commented 10 years ago

We will going to prepare try2 with this revision points, thanks for your review =)!!!

code-sur commented 10 years ago

@dnarvaez, @rogemita This code remains single-threaded, though the likelihood of failing is low, there is not guarantee of correctness in other contexts (i.e. future webkit versions or other browsers). Also, I can't figure out an easy way to make it thread-safe.

Should we take some action due to this known issue?

dnarvaez commented 10 years ago

I tend to think adding a comment would be fine for now. I can't think of a way to avoid the race either, we might have to write a webkit extension and avoid the hack if it turns out we need to be multi thread safe.

dnarvaez commented 10 years ago

One possible approach to get rid of the race without having to write C code might be to fetch an activity://..../environment.json (dynamically generated on the python side). I sort of like that approach actually, it feels better of what we have now at least.

dnarvaez commented 10 years ago

(Breaking the contract with toolkit would be annoying for backward compatibility reason, but I'm not sure we should start worrying too much about that yet).

rogemita commented 10 years ago

I tend to think adding a comment would be fine for now.

so is ok at the moment, a message on getEnvironment function, like: ...

???

dnarvaez commented 10 years ago

Maybe "// FIXME we assume this code runs on the same thread as the javascript executed from python" or something like that.

code-sur commented 10 years ago

summary for try2:

@dnarvaez, I'm going to fix those points and push directly (if you agree). I'll rebase with master in order to merge fast-forward (without empty merge commit)

dnarvaez commented 10 years ago

Yup!

code-sur commented 10 years ago

pushed

Thanks for reviewing!