Bug 87965 - Need customized toDOMStringList for IndexedDB JSC binding.
Summary: Need customized toDOMStringList for IndexedDB JSC binding.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Charles Wei
URL:
Keywords:
Depends on:
Blocks: 45110
  Show dependency treegraph
 
Reported: 2012-05-31 06:10 PDT by Charles Wei
Modified: 2014-04-24 16:45 PDT (History)
17 users (show)

See Also:


Attachments
Patch (5.75 KB, patch)
2012-06-11 05:28 PDT, Charles Wei
no flags Details | Formatted Diff | Diff
Patch (10.88 KB, patch)
2012-06-11 19:01 PDT, Charles Wei
no flags Details | Formatted Diff | Diff
Patch (10.72 KB, patch)
2012-06-11 23:23 PDT, Charles Wei
no flags Details | Formatted Diff | Diff
Patch (9.67 KB, patch)
2012-06-12 01:12 PDT, Charles Wei
no flags Details | Formatted Diff | Diff
Patch (9.46 KB, patch)
2012-06-12 02:11 PDT, Charles Wei
no flags Details | Formatted Diff | Diff
Patch (9.51 KB, patch)
2012-06-12 02:23 PDT, Charles Wei
no flags Details | Formatted Diff | Diff
Patch (10.45 KB, patch)
2012-06-12 06:13 PDT, Charles Wei
no flags Details | Formatted Diff | Diff
Patch (18.37 KB, patch)
2012-06-13 01:25 PDT, Charles Wei
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charles Wei 2012-05-31 06:10:30 PDT
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).
Comment 1 Charles Wei 2012-06-11 05:28:47 PDT
Created attachment 146836 [details]
Patch
Comment 2 jochen 2012-06-11 05:37:37 PDT
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
Comment 3 Charles Wei 2012-06-11 19:01:03 PDT
Created attachment 146992 [details]
Patch
Comment 4 Kentaro Hara 2012-06-11 19:12:06 PDT
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
Comment 5 Charles Wei 2012-06-11 19:35:54 PDT
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.
Comment 6 Charles Wei 2012-06-11 23:23:57 PDT
Created attachment 147016 [details]
Patch
Comment 7 Kentaro Hara 2012-06-11 23:36:28 PDT
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?
Comment 8 Kentaro Hara 2012-06-11 23:42:59 PDT
cc-ing JSC folks
Comment 9 Charles Wei 2012-06-12 00:21:27 PDT
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.
Comment 10 Charles Wei 2012-06-12 01:12:26 PDT
Created attachment 147030 [details]
Patch
Comment 11 Kentaro Hara 2012-06-12 01:27:31 PDT
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.
Comment 12 Charles Wei 2012-06-12 02:11:07 PDT
Created attachment 147039 [details]
Patch
Comment 13 WebKit Review Bot 2012-06-12 02:15:18 PDT
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.
Comment 14 Charles Wei 2012-06-12 02:16:38 PDT
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.
Comment 15 Charles Wei 2012-06-12 02:23:48 PDT
Created attachment 147041 [details]
Patch
Comment 16 Kentaro Hara 2012-06-12 03:45:40 PDT
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?
Comment 17 Kentaro Hara 2012-06-12 03:48:06 PDT
(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.
Comment 18 Charles Wei 2012-06-12 06:13:34 PDT
Created attachment 147069 [details]
Patch
Comment 19 Kentaro Hara 2012-06-12 06:43:34 PDT
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>"?
Comment 20 Charles Wei 2012-06-12 08:04:21 PDT
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.
Comment 21 Kentaro Hara 2012-06-12 08:28:32 PDT
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.
Comment 22 Charles Wei 2012-06-12 18:10:49 PDT
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.
Comment 23 Charles Wei 2012-06-12 18:20:26 PDT
(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.
Comment 24 Kentaro Hara 2012-06-12 18:48:02 PDT
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?
Comment 25 jochen 2012-06-12 19:06:50 PDT
(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.
Comment 26 Charles Wei 2012-06-12 19:51:05 PDT
(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.
Comment 27 Charles Wei 2012-06-12 19:58:36 PDT
(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 ?
Comment 28 Charles Wei 2012-06-13 01:25:12 PDT
Created attachment 147260 [details]
Patch
Comment 29 Charles Wei 2012-06-13 01:28:25 PDT
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.
Comment 30 Kentaro Hara 2012-06-13 01:38:44 PDT
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.
Comment 31 Kentaro Hara 2012-06-13 01:42:34 PDT
(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:-)
Comment 32 Charles Wei 2012-06-13 15:31:52 PDT
(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.
Comment 33 WebKit Review Bot 2012-06-13 16:42:14 PDT
Comment on attachment 147260 [details]
Patch

Clearing flags on attachment: 147260

Committed r120260: <http://trac.webkit.org/changeset/120260>
Comment 34 WebKit Review Bot 2012-06-13 16:42:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Darin Adler 2014-04-24 16:45:35 PDT
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.