Created attachment 159682 [details] Patch

Comment on attachment 159682 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159682&action=review > Source/WebCore/bridge/qt/qt_runtime.cpp:838 > + // ### FIXME: Need private JSC C API. If possible, create a bug for this and reference it here. > Source/WebCore/bridge/qt/qt_runtime.cpp:884 > + if (exception && *exception) > + break; > + if (propertyValue) > + JSObjectSetProperty(context, ret, propertyName, propertyValue, kJSPropertyAttributeNone, exception); > + if (exception && *exception) > + break; Do we want to stop when there's an error converting something, or just skip it to be consistent with the other "container" types? > Source/WebCore/bridge/qt/qt_runtime.cpp:894 > + // ### Could use special array class that lazily converts. Use "TODO" :-)

Comment on attachment 159682 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159682&action=review > Source/WebCore/ChangeLog:19 > + * bridge/qt/qt_instance.cpp: > + (Bindings): > + (JSC::Bindings::QtField::valueFromInstance): > + * bridge/qt/qt_runtime.cpp: > + (JSC::Bindings::convertQVariantToValue): > + (JSC::Bindings::QtRuntimeMethod::call): > + (JSC::Bindings::QtConnectionObject::execute): > + (JSC::Bindings::::valueAt): > + * bridge/qt/qt_runtime.h: > + (Bindings): a bit more info could be added. Like now you actually throw errors >> Source/WebCore/bridge/qt/qt_runtime.cpp:894 >> + // ### Could use special array class that lazily converts. > > Use "TODO" :-) I think we are actually supposed to use FIXME: still a fixme or todo is better with a bug link :-) > Source/WebCore/bridge/qt/qt_runtime.cpp:928 > } else if (type == (QMetaType::Type)qMetaTypeId<QList<int> >()) { why no space before qMeta here you have that higher up in the code

Comment on attachment 159682 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159682&action=review >> Source/WebCore/ChangeLog:19 >> + (Bindings): > > a bit more info could be added. Like now you actually throw errors The error handling is actually just an artifact of the JSC C API, behaviourally nothing changed (because in the internal API the exceptions are not handled using a separate variable but they "remain" in ExecState). But I'll write a few more words :) >> Source/WebCore/bridge/qt/qt_runtime.cpp:838 >> + // ### FIXME: Need private JSC C API. > > If possible, create a bug for this and reference it here. On second thought, I think I should remove the comment. The DOM binding for this type doesn't belong into the (pure) JSC C API and the use of internal API is okay here. When moving this code out WebCore at some point I'll see if we can handle the bytearray conversion using the custom type code. Along with the pixmap runtime there'll always be a little bit of code left that uses the JSC internals, but we can keep that code minimal and separate from the rest. >> Source/WebCore/bridge/qt/qt_runtime.cpp:884 >> + break; > > Do we want to stop when there's an error converting something, or just skip it to be consistent with the other "container" types? Hm, right. Ok, I'll change it back for consistency and pass null in fact. (we don't want to "pollute" the exception parameter of the overall function then) >>> Source/WebCore/bridge/qt/qt_runtime.cpp:894 >>> + // ### Could use special array class that lazily converts. >> >> Use "TODO" :-) > > I think we are actually supposed to use FIXME: still a fixme or todo is better with a bug link :-) Alright, I'll file a bug and use TODO. >> Source/WebCore/bridge/qt/qt_runtime.cpp:928 >> } else if (type == (QMetaType::Type)qMetaTypeId<QList<int> >()) { > > why no space before qMeta here you have that higher up in the code Hm, good point, that's inconsistent, although I think higher up in the code there should not be a space. I'll make it more consistent :)

Created attachment 159902 [details] Patch

Comment on attachment 159902 [details] Patch I still think the changelog could use a few words

(In reply to comment #6) > (From update of attachment 159902 [details]) > I still think the changelog could use a few words Oops, I forgot that in the uploaded patch. Will elaborate when landing. Sorry :)

Committed r126290: <http://trac.webkit.org/changeset/126290>