Bug 87965 – Need customized toDOMStringList for IndexedDB JSC binding.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Charles Wei]

IndexedDB API:  

Interface IDBDabase:

        [CallWith=ScriptExecutionContext] IDBTransaction transaction(in DOMStringList storeNames, in [Optional=DefaultIsNullString] DOMString mode)
            raises (IDBDatabaseException);
        [CallWith=ScriptExecutionContext] IDBTransaction transaction(in DOMString[] storeNames, in [Optional=DefaultIsNullString] DOMString mode)
            raises (IDBDatabaseException);
        [CallWith=ScriptExecutionContext] IDBTransaction transaction(in DOMString storeName, in [Optional=DefaultIsNullString] DOMString mode)


The auto-generated  DOMStringList* toDOMStringList( JSValue value)  can't convert an JavaScript String Array into a native DOMStringList object.  Need a customized toDOMStringList(JSValue).

[Charles Wei]

Created attachment 146836 [details]
Patch

Comment on attachment 146836 [details]
Patch

Is it possible to write a test for this?

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

> Source/WebCore/bindings/js/JSDOMStringListCustom.cpp:34
> +    for (int i = 0; i < array->length(); i++)

nit. ++i

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1023
> +        } elsif($interfaceName eq "DOMStringList") {

space between elseif and (

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1025
> +        }else {

space between } and else

[Charles Wei]

Created attachment 146992 [details]
Patch

[Kentaro Hara]

Comment on attachment 146992 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        The generated toDOMStringList() can't convert an JSArry of Strings to 
> +        native DOMStringList. We need a custom function to do this.

We do not want to add custom bindings. Instead you can add toDOMStringList() to JSDOMBinding.{h,cpp}. Please refer to v8ValueToWebCoreDOMStringList() in V8Binding.{h,cpp}

Also valueToDOMStringList() would be a better name, for consistency with other names in JSDOMBinding.h.

> Source/WebCore/bindings/js/JSDOMStringListCustom.cpp:34
> +    for (int i = 0; i < array->length(); ++i)

Nit: int => unsigned

[Charles Wei]

Comment on attachment 146992 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        native DOMStringList. We need a custom function to do this.
> 
> We do not want to add custom bindings. Instead you can add toDOMStringList() to JSDOMBinding.{h,cpp}. Please refer to v8ValueToWebCoreDOMStringList() in V8Binding.{h,cpp}
> 
> Also valueToDOMStringList() would be a better name, for consistency with other names in JSDOMBinding.h.

Ok. As discussed on IRC, I will move the toDOMStringList() to JSDOMBinding.{h,cpp}.  About renaming toDOMStringList() to valueTODOMStringList(),  because the generated code uses toDOMStringList() all over the places, we won't do the renaming and will keep the name as is.

>> Source/WebCore/bindings/js/JSDOMStringListCustom.cpp:34
>> +    for (int i = 0; i < array->length(); ++i)
> 
> Nit: int => unsigned

Ok.

[Charles Wei]

Created attachment 147016 [details]
Patch

[Kentaro Hara]

Comment on attachment 147016 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        The generted toDOMStringList() can't convert an JSArray of Strings to

typo: generted => generated

> Source/WebCore/bindings/js/JSDOMBinding.cpp:290
> +    if (!value.inherits(&JSArray::s_info))
> +        return 0;

I think that isJSArray(value) would be the correct check here.

Also I think you need to add the code that corresponds to v8ValueToWebCoreDOMStringList():

  if (V8DOMStringList::HasInstance(v8Value)) {
    ...;
  }

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2831
> +    "DOMString[]" => "RefPtr<DOMStringList>",
> +    "DOMStringList" => "RefPtr<DOMStringList>",

By the way, what's the difference between DOMString[] and DOMStringList after this patch?

[Kentaro Hara]

cc-ing JSC folks

[Charles Wei]

Comment on attachment 147016 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        The generted toDOMStringList() can't convert an JSArray of Strings to
> 
> typo: generted => generated

Thanks. Will fix.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:290
>> +        return 0;
> 
> I think that isJSArray(value) would be the correct check here.
> 
> Also I think you need to add the code that corresponds to v8ValueToWebCoreDOMStringList():
> 
>   if (V8DOMStringList::HasInstance(v8Value)) {
>     ...;
>   }

We can use isJSArray(value).  will fix it with the new patch.

V8NativeObject::HasInstance(V8Value)  seems V8-specific logic. I don't see corresponding JSC code in any of generated JSC binding code or custom JSC binding code. Please point to me if there's any that I should follow in JSC.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2831
>> +    "DOMStringList" => "RefPtr<DOMStringList>",
> 
> By the way, what's the difference between DOMString[] and DOMStringList after this patch?

They are the same.  Please have a look at the V8 code generator, they have the same logic -- DOMStringList is equivalent to DOMString[] in IDL files.

[Charles Wei]

Created attachment 147030 [details]
Patch

[Kentaro Hara]

Comment on attachment 147030 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        * bindings/js/JSDOMBinding.h:
> +        (WebCore):

This file is missing.

> Source/WebCore/bindings/js/JSDOMBinding.cpp:291
> +    if (!isJSArray(value))
> +        return 0;

> V8NativeObject::HasInstance(V8Value)  seems V8-specific logic. I don't see corresponding JSC code in any of generated JSC binding code or custom JSC binding code. Please point 
> to me if there's any that I should follow in JSC.

V8DOMStringList::HasInstance(value) corresponds to value.inherits(&JSDOMStringList::s_info)

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:204
> +    return 1 if $type eq "CSSStyleDeclaration" or $type eq "MediaList" or $type eq "DOMStringList" or $type eq "DOMStringList[]"  or $type eq "DOMTokenList" or $type eq "DOMSettableTokenList";

Shouldn't this be $type eq "DOMString[]"?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1024
> +        } elsif ($interfaceName eq "DOMStringList") {
> +            push(@headerContent, "PassRefPtr<DOMStringList> toDOMStringList(JSC::ExecState*, JSC::JSValue);\n");

This would be no longer needed. The declaration should be in JSDOMBinding.h.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2830
> -    "DOMString[]" => "DOMStringList*",
> +    "DOMString[]" => "RefPtr<DOMStringList>",

Why do you need to change DOMStringList* to RefPtr<DOMStringList>?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2995
>      if ($type eq "DOMString[]") {
>          AddToImplIncludes("JSDOMStringList.h", $conditional);
> -        return "toDOMStringList($value)";
> +        return "toDOMStringList(exec, $value)";
> +    }
> +
> +    if ($type eq "DOMStringList") {
> +        AddToImplIncludes("JSDOMStringList.h", $conditional);
> +        return "toDOMStringList(exec, $value)";
>      }

You can wrap up two if statements into one.

[Charles Wei]

Created attachment 147039 [details]
Patch

[WebKit Review Bot]

Attachment 147039 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Charles Wei]

Comment on attachment 147030 [details]
Patch

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

>> Source/WebCore/ChangeLog:18
>> +        (WebCore):
> 
> This file is missing.

We don't need to change this file anymore.   The new patch removes it from the change log.

The declaration of toDOMStringList() is in the generated JSDOMStringList.h,  not in JSDOMBinding.h

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:291
>> +        return 0;
> 
> 

So isJSArray(value) is the right check,  even though inherits(JSArray::s_info) is another option.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:204
>> +    return 1 if $type eq "CSSStyleDeclaration" or $type eq "MediaList" or $type eq "DOMStringList" or $type eq "DOMStringList[]"  or $type eq "DOMTokenList" or $type eq "DOMSettableTokenList";
> 
> Shouldn't this be $type eq "DOMString[]"?

My fault.  Fixed in the new patch.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1024
>> +            push(@headerContent, "PassRefPtr<DOMStringList> toDOMStringList(JSC::ExecState*, JSC::JSValue);\n");
> 
> This would be no longer needed. The declaration should be in JSDOMBinding.h.

No,  we still need this, because the code generator will otherwise generate 

DOMStringList* toDOMStringList(JSC::JSValue);

That is not what we want.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2830
>> +    "DOMString[]" => "RefPtr<DOMStringList>",
> 
> Why do you need to change DOMStringList* to RefPtr<DOMStringList>?

This is for memory management.  The DOMStringList* will be generated in the native code inside toDOMStringList(),   and pass that to the JS code.  The JS code won't delete it when done.  So we need to use RefPtr<DOMStringList> which will be automatically de-structed when JS code is done.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2995
>>      }
> 
> You can wrap up two if statements into one.

Yes, fixed in the new patch.

[Charles Wei]

Created attachment 147041 [details]
Patch

[Kentaro Hara]

Comment on attachment 147030 [details]
Patch

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

>>> Source/WebCore/bindings/js/JSDOMBinding.cpp:291
>>> +        return 0;
>> 
>> 
> 
> So isJSArray(value) is the right check,  even though inherits(JSArray::s_info) is another option.

Both are needed. The code would be something like:

if (value.inherits(&JSDOMStringList::s_info))
  return jsCast<JSDOMStringList*>(asObject(value))->impl();

if (!isJSArray(value))
  return 0;

>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1024
>>> +            push(@headerContent, "PassRefPtr<DOMStringList> toDOMStringList(JSC::ExecState*, JSC::JSValue);\n");
>> 
>> This would be no longer needed. The declaration should be in JSDOMBinding.h.
> 
> No,  we still need this, because the code generator will otherwise generate 
> 
> DOMStringList* toDOMStringList(JSC::JSValue);
> 
> That is not what we want.

I am really sorry for this. After reading how JSC bindings and V8 bindings work, as you pointed out, it would make sense to add toDOMStringList() in a custom file. Would you revert back the custom file?

>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2830
>>> +    "DOMString[]" => "RefPtr<DOMStringList>",
>> 
>> Why do you need to change DOMStringList* to RefPtr<DOMStringList>?
> 
> This is for memory management.  The DOMStringList* will be generated in the native code inside toDOMStringList(),   and pass that to the JS code.  The JS code won't delete it when done.  So we need to use RefPtr<DOMStringList> which will be automatically de-structed when JS code is done.

My original question was... why is it OK to change the native type of DOMString[] despite the fact that this patch is just changing the behavior of DOMStringList?

It seems the answer is that currently no one uses DOMString[] in JSC (DOMString[] is used by IDB*.idl only, which is not yet enabled in JSC). So changing DOMString[] to DOMStringList is harmless.

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2439
> -    if ((argsCount == 1 && (arg0.isNull() || (arg0.isObject() && asObject(arg0)->inherits(&JSDOMStringList::s_info)))))
> +    if (argsCount == 1)

Where does this change come from?

[Kentaro Hara]

(In reply to comment #16)
> So changing DOMString[] to DOMStringList is harmless.

Correction: So changing the native type for DOMString[] (from DOMStringList* to RefPtr<DOMStringList>) is harmless.

[Charles Wei]

Created attachment 147069 [details]
Patch

[Kentaro Hara]

Comment on attachment 147069 [details]
Patch

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

Almost looks OK.

> Source/WebCore/UseJSC.cmake:60
> +    bindings/js/JSDOMStringListCustom.cpp

This would not be the only place where you need to add the file name.

$ grep -r JSDOMStringMapCustom.cpp *

will tell you the list of build files you need to touch.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2830
> -    "DOMString[]" => "DOMStringList*",
> +    "DOMString[]" => "RefPtr<DOMStringList>",

Don't you need to add "DOMStringList" => "RefPtr<DOMStringList>"?

[Charles Wei]

Comment on attachment 147069 [details]
Patch

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

>> Source/WebCore/UseJSC.cmake:60
>> +    bindings/js/JSDOMStringListCustom.cpp
> 
> This would not be the only place where you need to add the file name.
> 
> $ grep -r JSDOMStringMapCustom.cpp *
> 
> will tell you the list of build files you need to touch.

Well, there are so many portings and we don't have all the building system. We can only guarantee that all the systems that connect to the build-bot works, I don't want to blindly add this to other systems without verification. Any porting that are not added to the build-bot need to add this to their system by themselves.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2830
>> +    "DOMString[]" => "RefPtr<DOMStringList>",
> 
> Don't you need to add "DOMStringList" => "RefPtr<DOMStringList>"?

No, We don't. DOMStringList IS a native type and don't need to be listed here. Further, GetNativeType and GetNativeTypeForCallbacks already take care of both. So we don't need to add DOMStringList here.

[Kentaro Hara]

Comment on attachment 147069 [details]
Patch

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

>>> Source/WebCore/UseJSC.cmake:60
>>> +    bindings/js/JSDOMStringListCustom.cpp
>> 
>> This would not be the only place where you need to add the file name.
>> 
>> $ grep -r JSDOMStringMapCustom.cpp *
>> 
>> will tell you the list of build files you need to touch.
> 
> Well, there are so many portings and we don't have all the building system. We can only guarantee that all the systems that connect to the build-bot works, I don't want to blindly add this to other systems without verification. Any porting that are not added to the build-bot need to add this to their system by themselves.

What ports are you intending to support?

> We can only guarantee that all the systems that connect to the build-bot works

To guarantee that, you need to at least add the file to project.pbxproj (mac), GNUmakefile.list.am (Gtk), Target.pri (qt), WebCore.vcproj (win) and CMakeLists.txt (Efl). The reason why build-bots are green without adding the file is simply that IndexedDB is not yet enabled on none of the build-bots that use JSC. This is the reason why I do not want to land patches before enabling IndexedDB. I am afraid that a bunch of stuff would break when IndexedDB is enabled.

> Any porting that are not added to the build-bot need to add this to their system by themselves.

At least for those build-bots, (basically) it is our responsibility to make it work.

[Charles Wei]

Comment on attachment 147069 [details]
Patch

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

>>>> Source/WebCore/UseJSC.cmake:60
>>>> +    bindings/js/JSDOMStringListCustom.cpp
>>> 
>>> This would not be the only place where you need to add the file name.
>>> 
>>> $ grep -r JSDOMStringMapCustom.cpp *
>>> 
>>> will tell you the list of build files you need to touch.
>> 
>> Well, there are so many portings and we don't have all the building system. We can only guarantee that all the systems that connect to the build-bot works, I don't want to blindly add this to other systems without verification. Any porting that are not added to the build-bot need to add this to their system by themselves.
> 
> What ports are you intending to support?

I am intending to support all JSC bindings.

[Charles Wei]

(In reply to comment #21)
> (From update of attachment 147069 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147069&action=review
> 
> >>> Source/WebCore/UseJSC.cmake:60
> >>> +    bindings/js/JSDOMStringListCustom.cpp
> >> 
> >> This would not be the only place where you need to add the file name.
> >> 
> >> $ grep -r JSDOMStringMapCustom.cpp *
> >> 
> >> will tell you the list of build files you need to touch.
> > 
> > Well, there are so many portings and we don't have all the building system. We can only guarantee that all the systems that connect to the build-bot works, I don't want to blindly add this to other systems without verification. Any porting that are not added to the build-bot need to add this to their system by themselves.
> 
> What ports are you intending to support?
> 
> > We can only guarantee that all the systems that connect to the build-bot works
> 
> To guarantee that, you need to at least add the file to project.pbxproj (mac), GNUmakefile.list.am (Gtk), Target.pri (qt), WebCore.vcproj (win) and CMakeLists.txt (Efl). The reason why build-bots are green without adding the file is simply that IndexedDB is not yet enabled on none of the build-bots that use JSC. This is the reason why I do not want to land patches before enabling IndexedDB. I am afraid that a bunch of stuff would break when IndexedDB is enabled.
> 
> > Any porting that are not added to the build-bot need to add this to their system by themselves.
> 
> At least for those build-bots, (basically) it is our responsibility to make it work.


1. To ask a submitter to do that is kind of hard,  you can't expect the patch submitter to have all the setups for mac, Gtk, qt, win, elf to build, test and verify.  I think it's the port-specific's responsibility to do that.

2. As you pointed out, the patch works without breaking any of above porting, because the patch is only used by IndexedDB while IndexedDB is not enabled on all above porting.  I just can't enable IndexedDB for above porting because that's also a porting-specific decision and might need some effort to do that , like it even doesn't compile when you just enable IndexedDB.

3. I am now doing this and verified the patch on BlackBerry porting, which is in the process of upstreaming.

4. If you really like, I can blindly add this to other porting, but I can't verify it on them.

[Kentaro Hara]

I cannot say a strong comment since I am not familiar with Blackberry nor IndexedDB, we might want to reach a consensus about how to enable IndexedDB in JSC bindings. Comments from experts are appreciated.

(In reply to comment #23)
> 1. To ask a submitter to do that is kind of hard,  you can't expect the patch submitter to have all the setups for mac, Gtk, qt, win, elf to build, test and verify.  I think it's the port-specific's responsibility to do that.
> 
> 2. As you pointed out, the patch works without breaking any of above porting, because the patch is only used by IndexedDB while IndexedDB is not enabled on all above porting.  I just can't enable IndexedDB for above porting because that's also a porting-specific decision and might need some effort to do that , like it even doesn't compile when you just enable IndexedDB.
> 
> 3. I am now doing this and verified the patch on BlackBerry porting, which is in the process of upstreaming.
> 
> 4. If you really like, I can blindly add this to other porting, but I can't verify it on them.


>> What ports are you intending to support?
>
> I am intending to support all JSC bindings.
>

- I am a bit confused. Is your working context to enable IndexedDB on Blackberry only? Or are you planning to enable IndexedDB on, for example, AppleWebKit? (Of course this would require consensus of AppleWebKit folks.)

- Do you have any reviewer who is familiar with Blackberry?

- I would like to know your overall plan to enable IndexedDB on JSC bindings (i.e. we do not want to land a lot of patches without being enabled.) Is your plan to land the patch in bug 45110 step by step?

(In reply to comment #23)
> 1. To ask a submitter to do that is kind of hard,  you can't expect the patch submitter to have all the setups for mac, Gtk, qt, win, elf to build, test and verify.  I think it's the port-specific's responsibility to do that.
> 

I think it's reasonable to ask a committer to be able to compile at least two distinct ports.


> 2. As you pointed out, the patch works without breaking any of above porting, because the patch is only used by IndexedDB while IndexedDB is not enabled on all above porting.  I just can't enable IndexedDB for above porting because that's also a porting-specific decision and might need some effort to do that , like it even doesn't compile when you just enable IndexedDB.

You changed tests, I'd expect them to break on all other JSC ports?

> 
> 3. I am now doing this and verified the patch on BlackBerry porting, which is in the process of upstreaming.
> 
> 4. If you really like, I can blindly add this to other porting, but I can't verify it on them.

A common practice is to add a new, empty file in a separate CL that just updates all the build files. I admit that changing the build files is a cumbersome process. However, the EWS will already tell you whether the files are being picked up and compile correctly.

[Charles Wei]

(In reply to comment #24)
> I cannot say a strong comment since I am not familiar with Blackberry nor IndexedDB, we might want to reach a consensus about how to enable IndexedDB in JSC bindings. Comments from experts are appreciated.
> 
> (In reply to comment #23)
> > 1. To ask a submitter to do that is kind of hard,  you can't expect the patch submitter to have all the setups for mac, Gtk, qt, win, elf to build, test and verify.  I think it's the port-specific's responsibility to do that.
> > 
> > 2. As you pointed out, the patch works without breaking any of above porting, because the patch is only used by IndexedDB while IndexedDB is not enabled on all above porting.  I just can't enable IndexedDB for above porting because that's also a porting-specific decision and might need some effort to do that , like it even doesn't compile when you just enable IndexedDB.
> > 
> > 3. I am now doing this and verified the patch on BlackBerry porting, which is in the process of upstreaming.
> > 
> > 4. If you really like, I can blindly add this to other porting, but I can't verify it on them.
> 
> 
> >> What ports are you intending to support?
> >
> > I am intending to support all JSC bindings.
> >
> 
> - I am a bit confused. Is your working context to enable IndexedDB on Blackberry only? Or are you planning to enable IndexedDB on, for example, AppleWebKit? (Of course this would require consensus of AppleWebKit folks.)

1. My working context is BlackBerry porting with CMake build system,  and I verified that it works on BlackBerry porting,  the EFL porting also uses CMAKE and automatically picks this up also.
2. The patch is cross-platform, it works the same on all other platforms, even though not verified on other platforms,  we have the good faith that it will.
3. We (BlackBerry porting) are the first to enable the IDB for JSC binding,  and all portings that use JSC need this patch to support indexedDB.  They need to pick this up when they enable indexedDB. 
4. As the first poring of this for JSC, we should not be responsible to make sure all portings to get this effort-free,  just like IndexedDB first introduced for V8, they didn't make sure it also works for JSC. 

> 
> - Do you have any reviewer who is familiar with Blackberry?
> 
Yes,  we do have some reviewers for BlackBerry, but we would like reviewers from other porting to review and comment.

> - I would like to know your overall plan to enable IndexedDB on JSC bindings (i.e. we do not want to land a lot of patches without being enabled.) Is your plan to land the patch in bug 45110 step by step?

My overall plan (actually I already get it done with BlackBerry porting,  now just upstreaming it),  as you pointed out about 45110:  I have identified about 8 problems to solve to make indexedDB working for JSC , this is one of them.
Without all patches landed, IndexedDB won't work, or won't work full-functional.

There's a patch in 45110 , trying to enable IDB for JSC with one big patch,  but it fails the review for sure because it's too big, and nobody feels comfortable with such a big patch. I break it down into several small issues, each is independent.
when we have all the issues resolved,  the indexedDB is ready for JSC, and each porting can just enable it with their build system.

[Charles Wei]

(In reply to comment #25)
> (In reply to comment #23)
> > 1. To ask a submitter to do that is kind of hard,  you can't expect the patch submitter to have all the setups for mac, Gtk, qt, win, elf to build, test and verify.  I think it's the port-specific's responsibility to do that.
> > 
> 
> I think it's reasonable to ask a committer to be able to compile at least two distinct ports.
> 
   Yes, it compiles all distinct port and doesn't break any porting,   just EFL and BlackBerry  (CMake build system) are really taking advantage of the patch.  All other porting will benefit this when they enable IndexedDB. 

> 
> > 2. As you pointed out, the patch works without breaking any of above porting, because the patch is only used by IndexedDB while IndexedDB is not enabled on all above porting.  I just can't enable IndexedDB for above porting because that's also a porting-specific decision and might need some effort to do that , like it even doesn't compile when you just enable IndexedDB.
> 
> You changed tests, I'd expect them to break on all other JSC ports?

No, it breaks no any porting.  The build-bot will say.

> 
> > 
> > 3. I am now doing this and verified the patch on BlackBerry porting, which is in the process of upstreaming.
> > 
> > 4. If you really like, I can blindly add this to other porting, but I can't verify it on them.
> 
> A common practice is to add a new, empty file in a separate CL that just updates all the build files. I admit that changing the build files is a cumbersome process. However, the EWS will already tell you whether the files are being picked up and compile correctly.

I can change all build system to pick up the change in this patch ( without any build failure, no regression),  but I don't have the setup to verify,  or I have to blindly change that and submit to bugzilla to take advantage of the build-bot which will try to build and verify,  but is that the good and common practice ?

[Charles Wei]

Created attachment 147260 [details]
Patch

[Charles Wei]

Comment on attachment 147260 [details]
Patch

With this patch,  I built, test and verified on Qt/Linux,  Mac , and BlackBerry porting,  for other portings, I am counting on the build-bot to verify the correctness.

[Kentaro Hara]

Comment on attachment 147260 [details]
Patch

> With this patch,  I built, test and verified on Qt/Linux,  Mac , and BlackBerry porting,  for other portings, I am counting on the build-bot to verify the correctness.

Sounds great! Let's land it after confirming green faces of all the bots.

[Kentaro Hara]

(In reply to comment #30)
> Let's land it after confirming green faces of all the bots.

That being said, the green face does not mean that IndexedDB works well on the bot, but just means that the patch does not break the build. I hope that IndexedDB is enabled in JSC bindings as soon as possible to avoid landing patches without being fully tested. (Until then, we count on your local test environment:-)

[Charles Wei]

(In reply to comment #30)
> (From update of attachment 147260 [details])
> > With this patch,  I built, test and verified on Qt/Linux,  Mac , and BlackBerry porting,  for other portings, I am counting on the build-bot to verify the correctness.
> 
> Sounds great! Let's land it after confirming green faces of all the bots.

Thanks for the review,haraken.

[WebKit Review Bot]

Comment on attachment 147260 [details]
Patch

Clearing flags on attachment: 147260

Committed r120260: <http://trac.webkit.org/changeset/120260>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Moving all JavaScriptGlue bugs to JavaScriptCore. The JavaScriptGlue framework itself is long gone. And most of the more recent bugs put in this component were put there by people who thought this was for some other aspect of “JavaScript glue” and have nothing to do with the actual original reason for the existence of this component, which was an OS-X-only framework named JavaScriptGlue.