Bug 45142 – [Qt] V8 port for Qt platform: Use DateMath from WTF

• Component: New Bugs
• Status: RESOLVED INVALID

[Vlad]

Add getDSTOffset, cachedUTCOffset to global object as hidden properties

[Vlad]

Created attachment 66435 [details]
Use DateMath from WTF

[Andreas Kling]

Comment on attachment 66435 [details]
Use DateMath from WTF

> +        [Qt] V8 port for QT platform: Use DateMath from WTF 

I'm sure there's a reason we need this from WTF, but could you expand on why? :-)

> -            if (*dateString != ':') {
> +            if (*dateString != ':')
>                  offset = ((o / 100) * 60 + (o % 100)) * sgn;
> -            } else { // GMT+05:00
> +            else { // GMT+05:00

While correct, please avoid making unrelated style changes as it makes "svn blame" harder to use.

[Vlad]

DateMath is used by /bridge/qt/qt_runtime... 

(In reply to comment #2)
> (From update of attachment 66435 [details])
> > +        [Qt] V8 port for QT platform: Use DateMath from WTF 
> 
> I'm sure there's a reason we need this from WTF, but could you expand on why? :-)

I got style failure, so I went and fixed that... 

> 
> > -            if (*dateString != ':') {
> > +            if (*dateString != ':')
> >                  offset = ((o / 100) * 60 + (o % 100)) * sgn;
> > -            } else { // GMT+05:00
> > +            else { // GMT+05:00
> 
> While correct, please avoid making unrelated style changes as it makes "svn blame" harder to use.

[Vlad]

These API is used by /bridge/qt/qt_runtime
void msToGregorianDateTime(v8::Handle<v8::Context> context, double, bool outputIsUTC, GregorianDateTime&);
double gregorianDateTimeToMS(v8::Handle<v8::Context> context, const GregorianDateTime&, double, bool inputIsUTC);
double getUTCOffset(v8::Handle<v8::Context> context);
double parseDateFromNullTerminatedCharacters(v8::Handle<v8::Context> context, const char* dateString);

[Vlad]

Created attachment 66536 [details]
Updated according to comments

[Adam Barth]

Comment on attachment 66536 [details]
Updated according to comments

View in context: https://bugs.webkit.org/attachment.cgi?id=66536&action=prettypatch

I was tempted to R- this patch because of the dependency issue, but I don't really understand the contraints here in enough detail.

> JavaScriptCore/wtf/DateMath.cpp:866
>  #if USE(JSC)
>  namespace JSC {
> +#elif USE(V8)
> +namespace V8 {
> +#endif
Woah, crazy.

> JavaScriptCore/wtf/DateMath.h:54
> +#if USE(V8) && PLATFORM(QT)
> +#include <v8.h>
> +#endif
This seems like a bad dependency.  I don't think code outside of WebCore/bindings/v8 is supposed to depend on v8.h.

[Andreas Kling]

Comment on attachment 66536 [details]
Updated according to comments

Yeah, this isn't gonna work, we don't want to put V8 stuff in JavaScriptCore/

I'd rather we implement these methods where they're needed in the Qt bridge.
Or better yet, remove the dependency altogether (if possible.)

[Vlad]

Sorry guys, but if datemath has direct references to JSC, why it cannot depends on V8? Datemath API is used in QT-v8 based implementation. I cannot use v8 api without including v8.h 

#ifdef USE(JSC)
#elif USE(V8)
#endif
based on other comments for the same metabug.
Crazy would be copying the same implementation to another location, IMHO.

[Andreas Kling]

It reads to me like the bridge code could be written using Qt date/time types only -- do you really need something that JSC's GregorianDateTime provides?

[Kent Hansen]

(In reply to comment #8)
> Sorry guys, but if datemath has direct references to JSC, why it cannot depends on V8? Datemath API is used in QT-v8 based implementation. I cannot use v8 api without including v8.h 
> 
> #ifdef USE(JSC)
> #elif USE(V8)
> #endif
> based on other comments for the same metabug.
> Crazy would be copying the same implementation to another location, IMHO.

Granted, the DateMath dependency on JSC isn't the best ever (an ExecState just to get to a DST/UTCOffset cache?!). But wtf/DateMath does live under JavaScriptCore/, after all, so it's a rather pragmatic solution.

Seems like the two options are either to generalize DateMath (so it doesn't have the JSC dependency), or, like Andreas suggested, use QDateTime directly. I'm in favor of the latter, we should at least figure out why QDateTime isn't sufficient (and extend it if appropriate).  For example, instead of msToGregorianDateTime() it should be possible to base the conversion on QDateTime::fromTime_t() (http://doc.trolltech.com/4.6/qdatetime.html#fromTime_t).

[Simon Hausmann]

Closing as invalid. I agree with Kent and as discussed with Andreas we're closing this as invalid and instead will use QDateTime for landing.

[anton muhin]

Vlad, may I ask you to post patches as plain patches, not zipped sources?  It's more convenient to review them, esp. with the tools Adam is working on these days.

First round of comments:

qt_instancev8.h:

(stylistic issue, feel free to ignore):

    typedef enum {
        MetaProperty,
        DynamicProperty,
        ChildObject
    } QtFieldType;

Why not enum QtFieldType etc.?  It looks more C++-ish to me.

static v8::Handle<v8::Value> GetProperty(v8::Local<v8::String> propname, const v8::AccessorInfo &info);

I think WebKit code style requires method names to start with lower case.  Ditto for SetProperty, SetAccessors.  BTW, there are scripts under WebKitTools\Scripts which should have warn you about style violations.

qt_instancev8.cpp:

: m_context(v8::Persistent<v8::Context>::New(context))

Do you clear the handle somewhere?  Please note that destructor won't clean it up.  You should add explicit invocation of Dispose, probably followed by Clear to make it easier to catch errors.

v8::Handle<v8::FunctionTemplate> v8ClassTempl = v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New());

Persistent handles are heavyweight and should be avoided unless really needed.  They should be used when you want to have a reference to the object out of HandleScope, for example maps from native objects to v8 wrappers.

Apparently in your case you should just use HandleScope (unless it's already been created) and use local handles which are cheaper and has better (imho) semantics.


 m_v8Obj->SetPointerInInternalField(QtInstanceIndexId, (void*)QtInstanceGeneric);

That is somewhat tricky.  I'd rather not trick and cast numbers to pointers, v8's ints might be a better choice here (unless it's really necessary from performance point of view).

And even for performance, in your case you're lucky to have even number (0) and that would be stored rather efficiently, however if it'd been odd, performance would suffer and memory usage would be bigger.

        methodlPropTempl->SetAccessor(
            v8::String::New("connect"), 
                fieldGetter, 
                fieldSetter, 
                v8::String::New("connect"),
                v8::DEFAULT, 
                v8::DontDelete);
        methodlPropTempl->SetAccessor(
            v8::String::New("disconnect"), 
                fieldGetter, 
                fieldSetter, 
                v8::String::New("disconnect"),
                v8::DEFAULT, 
                v8::DontDelete);


Indentation is apparently off (compare first argument to others).

methodFunc->SetHiddenValue(v8::String::New("__qt_hidden_marker__"), v8::External::Wrap(this));

Just in case, hidden values are somewhat slow.

QtInstance::CreateV8Object:

you may want to refactor wrapping logic of creation connect/disconnect methods.

void QtInstance::GcCallback(v8::Persistent<v8::Value> obj, void*)
{
    v8::HandleScope scope;
    obj.Dispose();
}


As you don't create local handles here, you apparently don't need HandleScope. 

               // The garbage collector removes instances, but it may happen that the wrapped
                // QObject dies before the gc kicks in. To handle that case we have to do an additional
                // check if to see if the instance's wrapped object is still alive. If it isn't, then
                // we have to create a new wrapper.

Minor: indentation is off.

More principal: you would probably loose all the properties on the object (so named expando properties).  That might be fine, but typically those should be preserved.


// Sorry v8 cannot do fallback - no way to add accessors on the fly, when object exists...
// v8 patch is coming for that, but ...not going to use it.


That is not true anymore :)

qt_pixmapruntimev8.cpp

 HTMLImageElement* imageElement = V8HTMLImageElement::toNative(v8::Handle<v8::Object>::Cast(object));

Apparently cast is not needed.


   v8::Handle<v8::Object> global = instance->context()->Global();

    toV8(imageElement->document());
    return v8::Undefined();

Probably just in work, but you never use global, and doing toV8 conversion which is not used later as well.

Overall, might be prone to persistent handles leaks: nor m_context, nor m_v8Obj is cleaned.

qt_runtimev8.cpp

object = value->ToObject();

Probably the simple cast will do.

I am a paranoid about performance, so feel free to ignore, but

object->GetIdentityHash()

might be pretty expensive.  If you have pointers to Qt objects and those are never moved, it might be faster to use those pointers to check if object has been processed before.

 v8::Handle<v8::Value> v = thisObject->Get(args[1]->ToString());

That may throw, so you might consider using TryCatches around.

v8::Handle<v8::Value> QtRuntimeConnectionMethod::lengthGetter(v8::Local<v8::String> propname, const v8::AccessorInfo& info) 

If it always returns 1, maybe it should be a plain property?

[Vlad]

Thank you very much for the review, you helped a lot! Qt guys decided to discontinue direct injection of QObjects into JS, so this code became obsolete :(
BTW, your comments are for https://bugs.webkit.org/show_bug.cgi?id=45150

(In reply to comment #12)
> Vlad, may I ask you to post patches as plain patches, not zipped sources?  It's more convenient to review them, esp. with the tools Adam is working on these days.
> 
> First round of comments:
> 
> qt_instancev8.h:
> 
> (stylistic issue, feel free to ignore):
> 
>     typedef enum {
>         MetaProperty,
>         DynamicProperty,
>         ChildObject
>     } QtFieldType;
> 
> Why not enum QtFieldType etc.?  It looks more C++-ish to me.
> 
> static v8::Handle<v8::Value> GetProperty(v8::Local<v8::String> propname, const v8::AccessorInfo &info);
> 
> I think WebKit code style requires method names to start with lower case.  Ditto for SetProperty, SetAccessors.  BTW, there are scripts under WebKitTools\Scripts which should have warn you about style violations.
> 
> qt_instancev8.cpp:
> 
> : m_context(v8::Persistent<v8::Context>::New(context))
> 
> Do you clear the handle somewhere?  Please note that destructor won't clean it up.  You should add explicit invocation of Dispose, probably followed by Clear to make it easier to catch errors.
> 
> v8::Handle<v8::FunctionTemplate> v8ClassTempl = v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New());
> 
> Persistent handles are heavyweight and should be avoided unless really needed.  They should be used when you want to have a reference to the object out of HandleScope, for example maps from native objects to v8 wrappers.
> 
> Apparently in your case you should just use HandleScope (unless it's already been created) and use local handles which are cheaper and has better (imho) semantics.
> 
> 
>  m_v8Obj->SetPointerInInternalField(QtInstanceIndexId, (void*)QtInstanceGeneric);
> 
> That is somewhat tricky.  I'd rather not trick and cast numbers to pointers, v8's ints might be a better choice here (unless it's really necessary from performance point of view).
> 
> And even for performance, in your case you're lucky to have even number (0) and that would be stored rather efficiently, however if it'd been odd, performance would suffer and memory usage would be bigger.
> 
>         methodlPropTempl->SetAccessor(
>             v8::String::New("connect"), 
>                 fieldGetter, 
>                 fieldSetter, 
>                 v8::String::New("connect"),
>                 v8::DEFAULT, 
>                 v8::DontDelete);
>         methodlPropTempl->SetAccessor(
>             v8::String::New("disconnect"), 
>                 fieldGetter, 
>                 fieldSetter, 
>                 v8::String::New("disconnect"),
>                 v8::DEFAULT, 
>                 v8::DontDelete);
> 
> 
> Indentation is apparently off (compare first argument to others).
> 
> methodFunc->SetHiddenValue(v8::String::New("__qt_hidden_marker__"), v8::External::Wrap(this));
> 
> Just in case, hidden values are somewhat slow.
> 
> QtInstance::CreateV8Object:
> 
> you may want to refactor wrapping logic of creation connect/disconnect methods.
> 
> void QtInstance::GcCallback(v8::Persistent<v8::Value> obj, void*)
> {
>     v8::HandleScope scope;
>     obj.Dispose();
> }
> 
> 
> As you don't create local handles here, you apparently don't need HandleScope. 
> 
>                // The garbage collector removes instances, but it may happen that the wrapped
>                 // QObject dies before the gc kicks in. To handle that case we have to do an additional
>                 // check if to see if the instance's wrapped object is still alive. If it isn't, then
>                 // we have to create a new wrapper.
> 
> Minor: indentation is off.
> 
> More principal: you would probably loose all the properties on the object (so named expando properties).  That might be fine, but typically those should be preserved.
> 
> 
> // Sorry v8 cannot do fallback - no way to add accessors on the fly, when object exists...
> // v8 patch is coming for that, but ...not going to use it.
> 
> 
> That is not true anymore :)
> 
> qt_pixmapruntimev8.cpp
> 
>  HTMLImageElement* imageElement = V8HTMLImageElement::toNative(v8::Handle<v8::Object>::Cast(object));
> 
> Apparently cast is not needed.
> 
> 
>    v8::Handle<v8::Object> global = instance->context()->Global();
> 
>     toV8(imageElement->document());
>     return v8::Undefined();
> 
> Probably just in work, but you never use global, and doing toV8 conversion which is not used later as well.
> 
> Overall, might be prone to persistent handles leaks: nor m_context, nor m_v8Obj is cleaned.
> 
> qt_runtimev8.cpp
> 
> object = value->ToObject();
> 
> Probably the simple cast will do.
> 
> I am a paranoid about performance, so feel free to ignore, but
> 
> object->GetIdentityHash()
> 
> might be pretty expensive.  If you have pointers to Qt objects and those are never moved, it might be faster to use those pointers to check if object has been processed before.
> 
>  v8::Handle<v8::Value> v = thisObject->Get(args[1]->ToString());
> 
> That may throw, so you might consider using TryCatches around.
> 
> v8::Handle<v8::Value> QtRuntimeConnectionMethod::lengthGetter(v8::Local<v8::String> propname, const v8::AccessorInfo& info) 
> 
> If it always returns 1, maybe it should be a plain property?

[anton muhin]

(In reply to comment #13)
> Thank you very much for the review, you helped a lot! Qt guys decided to discontinue direct injection of QObjects into JS, so this code became obsolete :(
> BTW, your comments are for https://bugs.webkit.org/show_bug.cgi?id=45150

I am sorry.

> 
> (In reply to comment #12)
> > Vlad, may I ask you to post patches as plain patches, not zipped sources?  It's more convenient to review them, esp. with the tools Adam is working on these days.
> > 
> > First round of comments:
> > 
> > qt_instancev8.h:
> > 
> > (stylistic issue, feel free to ignore):
> > 
> >     typedef enum {
> >         MetaProperty,
> >         DynamicProperty,
> >         ChildObject
> >     } QtFieldType;
> > 
> > Why not enum QtFieldType etc.?  It looks more C++-ish to me.
> > 
> > static v8::Handle<v8::Value> GetProperty(v8::Local<v8::String> propname, const v8::AccessorInfo &info);
> > 
> > I think WebKit code style requires method names to start with lower case.  Ditto for SetProperty, SetAccessors.  BTW, there are scripts under WebKitTools\Scripts which should have warn you about style violations.
> > 
> > qt_instancev8.cpp:
> > 
> > : m_context(v8::Persistent<v8::Context>::New(context))
> > 
> > Do you clear the handle somewhere?  Please note that destructor won't clean it up.  You should add explicit invocation of Dispose, probably followed by Clear to make it easier to catch errors.
> > 
> > v8::Handle<v8::FunctionTemplate> v8ClassTempl = v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New());
> > 
> > Persistent handles are heavyweight and should be avoided unless really needed.  They should be used when you want to have a reference to the object out of HandleScope, for example maps from native objects to v8 wrappers.
> > 
> > Apparently in your case you should just use HandleScope (unless it's already been created) and use local handles which are cheaper and has better (imho) semantics.
> > 
> > 
> >  m_v8Obj->SetPointerInInternalField(QtInstanceIndexId, (void*)QtInstanceGeneric);
> > 
> > That is somewhat tricky.  I'd rather not trick and cast numbers to pointers, v8's ints might be a better choice here (unless it's really necessary from performance point of view).
> > 
> > And even for performance, in your case you're lucky to have even number (0) and that would be stored rather efficiently, however if it'd been odd, performance would suffer and memory usage would be bigger.
> > 
> >         methodlPropTempl->SetAccessor(
> >             v8::String::New("connect"), 
> >                 fieldGetter, 
> >                 fieldSetter, 
> >                 v8::String::New("connect"),
> >                 v8::DEFAULT, 
> >                 v8::DontDelete);
> >         methodlPropTempl->SetAccessor(
> >             v8::String::New("disconnect"), 
> >                 fieldGetter, 
> >                 fieldSetter, 
> >                 v8::String::New("disconnect"),
> >                 v8::DEFAULT, 
> >                 v8::DontDelete);
> > 
> > 
> > Indentation is apparently off (compare first argument to others).
> > 
> > methodFunc->SetHiddenValue(v8::String::New("__qt_hidden_marker__"), v8::External::Wrap(this));
> > 
> > Just in case, hidden values are somewhat slow.
> > 
> > QtInstance::CreateV8Object:
> > 
> > you may want to refactor wrapping logic of creation connect/disconnect methods.
> > 
> > void QtInstance::GcCallback(v8::Persistent<v8::Value> obj, void*)
> > {
> >     v8::HandleScope scope;
> >     obj.Dispose();
> > }
> > 
> > 
> > As you don't create local handles here, you apparently don't need HandleScope. 
> > 
> >                // The garbage collector removes instances, but it may happen that the wrapped
> >                 // QObject dies before the gc kicks in. To handle that case we have to do an additional
> >                 // check if to see if the instance's wrapped object is still alive. If it isn't, then
> >                 // we have to create a new wrapper.
> > 
> > Minor: indentation is off.
> > 
> > More principal: you would probably loose all the properties on the object (so named expando properties).  That might be fine, but typically those should be preserved.
> > 
> > 
> > // Sorry v8 cannot do fallback - no way to add accessors on the fly, when object exists...
> > // v8 patch is coming for that, but ...not going to use it.
> > 
> > 
> > That is not true anymore :)
> > 
> > qt_pixmapruntimev8.cpp
> > 
> >  HTMLImageElement* imageElement = V8HTMLImageElement::toNative(v8::Handle<v8::Object>::Cast(object));
> > 
> > Apparently cast is not needed.
> > 
> > 
> >    v8::Handle<v8::Object> global = instance->context()->Global();
> > 
> >     toV8(imageElement->document());
> >     return v8::Undefined();
> > 
> > Probably just in work, but you never use global, and doing toV8 conversion which is not used later as well.
> > 
> > Overall, might be prone to persistent handles leaks: nor m_context, nor m_v8Obj is cleaned.
> > 
> > qt_runtimev8.cpp
> > 
> > object = value->ToObject();
> > 
> > Probably the simple cast will do.
> > 
> > I am a paranoid about performance, so feel free to ignore, but
> > 
> > object->GetIdentityHash()
> > 
> > might be pretty expensive.  If you have pointers to Qt objects and those are never moved, it might be faster to use those pointers to check if object has been processed before.
> > 
> >  v8::Handle<v8::Value> v = thisObject->Get(args[1]->ToString());
> > 
> > That may throw, so you might consider using TryCatches around.
> > 
> > v8::Handle<v8::Value> QtRuntimeConnectionMethod::lengthGetter(v8::Local<v8::String> propname, const v8::AccessorInfo& info) 
> > 
> > If it always returns 1, maybe it should be a plain property?

[Vlad]

Dont be:) What I meant was that the exposure is not happening via qt bridge code any longer, but through QtScript engine now. Thanks again.