Make DOMTypedArray exported.

Make DOMTypedArray exported.

Instantiate DOMTypedArray methods used from modules.

https://docs.google.com/spreadsheets/d/1UYY-GIIMhsP9r2EZRAyo2NVKxftdoJsnPxiZL6nbnE8/edit?usp=sharing

BUG=358074
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192534

[tasak, 2015-03-19 11:17:24]

tasak@google.com changed reviewers:
+ haraken@chromium.org

[tasak, 2015-03-19 11:17:24]

PTAL?

https://codereview.chromium.org/1017183003/diff/1/Source/core/dom/DOMTypedArr...
File Source/core/dom/DOMTypedArray.cpp (right):

https://codereview.chromium.org/1017183003/diff/1/Source/core/dom/DOMTypedArr...
Source/core/dom/DOMTypedArray.cpp:61: template
PassRefPtr<DOMTypedArray<WTF::Int8Array, v8::Int8Array>>
DOMTypedArray<WTF::Int8Array, v8::Int8Array>::create(const
WTF::Int8Array::ValueType*, unsigned length);
Since WTF::Int8Array doesn't have createOrNull, the following compile error
occurrs if instantiating class DOMTypedArray<WTF::Int8Array, v8::Int8Array>>:

c:\src\blink\local\workspace0\src\third_party\webkit\source\core\dom\domtypedarray.h(56)
: error C2783: 'WTF::PassRefPtr<U> WTF::TypedArrayBase<T>::createOrNull(unsigned
int)' : could not deduce template argument for 'Subclass'
        with
        [
            T=unsigned char
        ]
       
c:\src\blink\local\workspace0\src\third_party\webkit\source\wtf\typedarraybase.h(121)
: see declaration of 'WTF::TypedArrayBase<T>::createOrNull'
        with
        [
            T=unsigned char
        ]
       
c:\src\blink\local\workspace0\src\third_party\webkit\source\core\dom\domtypedarray.h(55)
: while compiling class template member function
'WTF::PassRefPtr<blink::DOMTypedArray<WTF::Uint8Array,v8::Uint8Array>>
blink::DOMTypedArray<WTF::Uint8Array,v8::Uint8Array>::createOrNull(unsigned
int)'
       
c:\src\blink\local\workspace0\src\third_party\webkit\source\core\dom\domtypedarray.cpp(50)
: see reference to class template instantiation
'blink::DOMTypedArray<WTF::Uint8Array,v8::Uint8Array>' being compiled
ninja: build stopped: subcommand failed.

[haraken, 2015-03-19 11:18:17]

shiino-san: Would you take a look at this?

[Yuki, 2015-03-19 11:56:01]

yukishiino@chromium.org changed reviewers:
+ yukishiino@chromium.org

[Yuki, 2015-03-19 11:56:02]

https://codereview.chromium.org/1017183003/diff/1/Source/core/dom/DOMTypedArr...
File Source/core/dom/DOMTypedArray.cpp (right):

https://codereview.chromium.org/1017183003/diff/1/Source/core/dom/DOMTypedArr...
Source/core/dom/DOMTypedArray.cpp:61: template
PassRefPtr<DOMTypedArray<WTF::Int8Array, v8::Int8Array>>
DOMTypedArray<WTF::Int8Array, v8::Int8Array>::create(const
WTF::Int8Array::ValueType*, unsigned length);
On 2015/03/19 11:17:24, tasak wrote:
> Since WTF::Int8Array doesn't have createOrNull, the following compile error
> occurrs if instantiating class DOMTypedArray<WTF::Int8Array, v8::Int8Array>>:
> 
>
c:\src\blink\local\workspace0\src\third_party\webkit\source\core\dom\domtypedarray.h(56)
> : error C2783: 'WTF::PassRefPtr<U>
WTF::TypedArrayBase<T>::createOrNull(unsigned
> int)' : could not deduce template argument for 'Subclass'
>         with
>         [
>             T=unsigned char
>         ]
>        
>
c:\src\blink\local\workspace0\src\third_party\webkit\source\wtf\typedarraybase.h(121)
> : see declaration of 'WTF::TypedArrayBase<T>::createOrNull'
>         with
>         [
>             T=unsigned char
>         ]
>        
>
c:\src\blink\local\workspace0\src\third_party\webkit\source\core\dom\domtypedarray.h(55)
> : while compiling class template member function
> 'WTF::PassRefPtr<blink::DOMTypedArray<WTF::Uint8Array,v8::Uint8Array>>
> blink::DOMTypedArray<WTF::Uint8Array,v8::Uint8Array>::createOrNull(unsigned
> int)'
>        
>
c:\src\blink\local\workspace0\src\third_party\webkit\source\core\dom\domtypedarray.cpp(50)
> : see reference to class template instantiation
> 'blink::DOMTypedArray<WTF::Uint8Array,v8::Uint8Array>' being compiled
> ninja: build stopped: subcommand failed.

It seems we have two options.

1. Add WTF::Int8Array::createOrNull.
The implementation will be ASSERT_NOT_REACHED() + return nullptr;
Someone (likely me) needs to clean up WTF::ArrayBuffer family anyway,
so leave it to him.

2. Remove DOMTypedArray::createOrNull.
It seems DOMTypedArray::createOrNull is used only at two places:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
So, inline the implementation of DOMTypedArray::createOrNull there.
I think it's not so bad.  Maybe better than option 1.

I'm okay with either of the above options.

[tasak, 2015-03-20 04:34:47]

Thank you for review.

https://codereview.chromium.org/1017183003/diff/1/Source/core/dom/DOMTypedArr...
File Source/core/dom/DOMTypedArray.cpp (right):

https://codereview.chromium.org/1017183003/diff/1/Source/core/dom/DOMTypedArr...
Source/core/dom/DOMTypedArray.cpp:61: template
PassRefPtr<DOMTypedArray<WTF::Int8Array, v8::Int8Array>>
DOMTypedArray<WTF::Int8Array, v8::Int8Array>::create(const
WTF::Int8Array::ValueType*, unsigned length);
On 2015/03/19 11:56:02, Yuki wrote:
> On 2015/03/19 11:17:24, tasak wrote:
> > Since WTF::Int8Array doesn't have createOrNull, the following compile error
> > occurrs if instantiating class DOMTypedArray<WTF::Int8Array,
v8::Int8Array>>:
> > 
> >
>
c:\src\blink\local\workspace0\src\third_party\webkit\source\core\dom\domtypedarray.h(56)
> > : error C2783: 'WTF::PassRefPtr<U>
> WTF::TypedArrayBase<T>::createOrNull(unsigned
> > int)' : could not deduce template argument for 'Subclass'
> >         with
> >         [
> >             T=unsigned char
> >         ]
> >        
> >
>
c:\src\blink\local\workspace0\src\third_party\webkit\source\wtf\typedarraybase.h(121)
> > : see declaration of 'WTF::TypedArrayBase<T>::createOrNull'
> >         with
> >         [
> >             T=unsigned char
> >         ]
> >        
> >
>
c:\src\blink\local\workspace0\src\third_party\webkit\source\core\dom\domtypedarray.h(55)
> > : while compiling class template member function
> > 'WTF::PassRefPtr<blink::DOMTypedArray<WTF::Uint8Array,v8::Uint8Array>>
> > blink::DOMTypedArray<WTF::Uint8Array,v8::Uint8Array>::createOrNull(unsigned
> > int)'
> >        
> >
>
c:\src\blink\local\workspace0\src\third_party\webkit\source\core\dom\domtypedarray.cpp(50)
> > : see reference to class template instantiation
> > 'blink::DOMTypedArray<WTF::Uint8Array,v8::Uint8Array>' being compiled
> > ninja: build stopped: subcommand failed.
> 
> It seems we have two options.
> 
> 1. Add WTF::Int8Array::createOrNull.
> The implementation will be ASSERT_NOT_REACHED() + return nullptr;
> Someone (likely me) needs to clean up WTF::ArrayBuffer family anyway,
> so leave it to him.
> 
> 2. Remove DOMTypedArray::createOrNull.
> It seems DOMTypedArray::createOrNull is used only at two places:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> So, inline the implementation of DOMTypedArray::createOrNull there.
> I think it's not so bad.  Maybe better than option 1.
> 
> I'm okay with either of the above options.

I would like to instantiate template in per-method manner, because I found the
following compiler warnings when I tried option 1 or option2:

third_party\webkit\source\core\dom\domtypedarray.cpp(40) : warning C4661: 'const
blink::WrapperTypeInfo
&blink::DOMTypedArray<WTF::Int8Array,v8::Int8Array>::s_wrapperTypeInfo' : no
suitable definition provided for explicit template instantiation request
        third_party\webkit\source\core\dom\domtypedarray.h(26) : see declaration
of 'blink::DOMTypedArray<WTF::Int8Array,v8::Int8Array>::s_wrapperTypeInfo'
...

If we want to instantiate DOMTypedArray<> classes, we need suitable
s_wrapperTypeInfo definitions in DOMTypedArray.cpp. But the s_wrapperTypeInfo
are defined in generated binding code.

[Yuki, 2015-03-23 04:22:49]

Got it.  We need some work on this issue, but let's move forward with this way
for the time being.

lgtm

[haraken, 2015-03-23 04:31:47]

LGTM

[tasak, 2015-03-24 10:15:31]

Would you review this CL again?

According to offline discussion, a new patch is uploaded, and the patch is
completely different from the first one.

https://codereview.chromium.org/1017183003/diff/20001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/1017183003/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:34: const WrapperTypeInfo&
{{cpp_class}}::s_wrapperTypeInfo = {{v8_class}}::wrapperTypeInfo;
The code is removed because DOMTypedArray doesn't have s_wrapperTypeInfo.

https://codereview.chromium.org/1017183003/diff/20001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8Uint8ClampedArray.cpp (right):

https://codereview.chromium.org/1017183003/diff/20001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8Uint8ClampedArray.cpp:28: //
bindings/core/v8/ScriptWrappable.h.
Updated run-bindings-tests result because TestUint8ClampedArray (DOMTypedArray)
doesn't have s_wrapperTypeInfo.

https://codereview.chromium.org/1017183003/diff/20001/Source/core/dom/DOMType...
File Source/core/dom/DOMTypedArray.cpp (right):

https://codereview.chromium.org/1017183003/diff/20001/Source/core/dom/DOMType...
Source/core/dom/DOMTypedArray.cpp:49: // TODO(tasak): The following trais should
be auto-generated by binding script.
s/trais/traits/g

[Yuki, 2015-03-24 11:12:57]

https://codereview.chromium.org/1017183003/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/1017183003/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.h:220: // Declares only
'wrapperTypeInfo' not defines.
nit: Declares 'wrapperTypeInfo' method without definition.

Or "Declares 'wrapperTypeInfo' method." does because "declare" in most context
means declaration without definition.

https://codereview.chromium.org/1017183003/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.h:230: #define
DECLARE_WRAPPERTYPEINFO_ONLY() \
nit: DECLARE_WRAPPERTYPEINFO()

"declare A only" sounds like there could be cases that we declare not only A but
also B, C, etc.

https://codereview.chromium.org/1017183003/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.h:232: virtual const WrapperTypeInfo*
wrapperTypeInfo() const override
We need a trick of "private: typedef end_of_declare_wrappertypeinfo_t" as same
as the above macros.

https://codereview.chromium.org/1017183003/diff/40001/Source/core/dom/DOMType...
File Source/core/dom/DOMTypedArray.h (right):

https://codereview.chromium.org/1017183003/diff/40001/Source/core/dom/DOMType...
Source/core/dom/DOMTypedArray.h:85: #if !defined(WIN32)
I think this is not a correct way.  Microsoft's interpretation of "extern" is a
little unique, and seems to me that it's different from C++ spec...

Anyway, I think we need these "explicit instantiation declaration"s here even
with MSVC.

The problem here is that MSVC does not interpret "extern" as "declaration",
instead MSVC interprets "extern" as "dllimport" and MSVC determines linkage
based on dllexport/dllimport.

So what we should do is, IIUC,
    extern template class CORE_EXPORT ...
with proper compilers, and
    template class CORE_EXPORT ...
with MSVC.

You can do this by
    #if COMPILER(MSVC)
    #define TEMPLATE_DECL_EXTERN
    #else
    #define TEMPLATE_DECL extern
    #endif
    ....
    #undef TEMPLATE_DECL_EXTERN

Note that this is compiler-specific issue rather than platform-specific.  So use
COMPILER(MSVC).

https://codereview.chromium.org/1017183003/diff/40001/Source/modules/webaudio...
File Source/modules/webaudio/AudioBuffer.h (right):

https://codereview.chromium.org/1017183003/diff/40001/Source/modules/webaudio...
Source/modules/webaudio/AudioBuffer.h:70: static PassRefPtr<DOMFloat32Array>
createFloat32Array(size_t length);
nit: I'd slightly prefer createFloat32ArrayOrNull so that it's clear that the
return value can be nullptr.

[tasak, 2015-03-25 08:48:34]

Patchset #4 (id:60001) has been deleted

[tasak, 2015-03-25 08:55:07]

Patchset #4 (id:80001) has been deleted

[tasak, 2015-03-25 09:01:13]

Patchset #4 (id:100001) has been deleted

[tasak, 2015-03-25 09:01:58]

Thank you for review.

https://codereview.chromium.org/1017183003/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/1017183003/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.h:220: // Declares only
'wrapperTypeInfo' not defines.
On 2015/03/24 11:12:56, Yuki wrote:
> nit: Declares 'wrapperTypeInfo' method without definition.
> 
> Or "Declares 'wrapperTypeInfo' method." does because "declare" in most context
> means declaration without definition.

Done.

https://codereview.chromium.org/1017183003/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.h:230: #define
DECLARE_WRAPPERTYPEINFO_ONLY() \
On 2015/03/24 11:12:56, Yuki wrote:
> nit: DECLARE_WRAPPERTYPEINFO()
> 
> "declare A only" sounds like there could be cases that we declare not only A
but
> also B, C, etc.

Done.

https://codereview.chromium.org/1017183003/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.h:232: virtual const WrapperTypeInfo*
wrapperTypeInfo() const override
On 2015/03/24 11:12:56, Yuki wrote:
> We need a trick of "private: typedef end_of_declare_wrappertypeinfo_t" as same
> as the above macros.

Done.

https://codereview.chromium.org/1017183003/diff/40001/Source/core/dom/DOMType...
File Source/core/dom/DOMTypedArray.h (right):

https://codereview.chromium.org/1017183003/diff/40001/Source/core/dom/DOMType...
Source/core/dom/DOMTypedArray.h:85: #if !defined(WIN32)
On 2015/03/24 11:12:56, Yuki wrote:
> I think this is not a correct way.  Microsoft's interpretation of "extern" is
a
> little unique, and seems to me that it's different from C++ spec...
> 
> Anyway, I think we need these "explicit instantiation declaration"s here even
> with MSVC.
> 
> The problem here is that MSVC does not interpret "extern" as "declaration",
> instead MSVC interprets "extern" as "dllimport" and MSVC determines linkage
> based on dllexport/dllimport.
> 
> So what we should do is, IIUC,
>     extern template class CORE_EXPORT ...
> with proper compilers, and
>     template class CORE_EXPORT ...
> with MSVC.
> 
> You can do this by
>     #if COMPILER(MSVC)
>     #define TEMPLATE_DECL_EXTERN
>     #else
>     #define TEMPLATE_DECL extern
>     #endif
>     ....
>     #undef TEMPLATE_DECL_EXTERN
> 
> Note that this is compiler-specific issue rather than platform-specific.  So
use
> COMPILER(MSVC).

Updated according to offline discussion.

https://codereview.chromium.org/1017183003/diff/40001/Source/modules/webaudio...
File Source/modules/webaudio/AudioBuffer.h (right):

https://codereview.chromium.org/1017183003/diff/40001/Source/modules/webaudio...
Source/modules/webaudio/AudioBuffer.h:70: static PassRefPtr<DOMFloat32Array>
createFloat32Array(size_t length);
On 2015/03/24 11:12:56, Yuki wrote:
> nit: I'd slightly prefer createFloat32ArrayOrNull so that it's clear that the
> return value can be nullptr.

Done.

[Yuki, 2015-03-25 09:09:09]

lgtm with a nit.

https://codereview.chromium.org/1017183003/diff/120001/Source/core/dom/DOMTyp...
File Source/core/dom/DOMTypedArray.h (right):

https://codereview.chromium.org/1017183003/diff/120001/Source/core/dom/DOMTyp...
Source/core/dom/DOMTypedArray.h:84: #if COMPILER(MSVC) &&
defined(BLINK_CORE_IMPLEMENTATION) && BLINK_CORE_IMPLEMENTATION
defined(BLINK_CORE_IMPLEMENTATION) is just redundant.
If it's not defined, it evaluates to zero.

[tasak, 2015-03-25 09:26:07]

https://codereview.chromium.org/1017183003/diff/120001/Source/core/dom/DOMTyp...
File Source/core/dom/DOMTypedArray.h (right):

https://codereview.chromium.org/1017183003/diff/120001/Source/core/dom/DOMTyp...
Source/core/dom/DOMTypedArray.h:84: #if COMPILER(MSVC) &&
defined(BLINK_CORE_IMPLEMENTATION) && BLINK_CORE_IMPLEMENTATION
On 2015/03/25 09:09:08, Yuki wrote:
> defined(BLINK_CORE_IMPLEMENTATION) is just redundant.
> If it's not defined, it evaluates to zero.

I felt that WebKit prefers such "#if defined(X) && X".
So in this CL, I will use the style.

I will update the code later when we clean up all such "#if defined(X) && X"
(i.e. replace "#if X" instead of "#if defined(X) && X".)

[tasak, 2015-03-25 09:28:26]

> I will update the code later when we clean up all such "#if defined(X) && X"

Sorry. 

> (i.e. replace "#if defined(X) && X" with "#if X")

is correct.

[haraken, 2015-03-25 09:52:55]

LGTM based on shiino-san's LG.

https://codereview.chromium.org/1017183003/diff/120001/Source/core/dom/DOMTyp...
File Source/core/dom/DOMTypedArray.h (right):

https://codereview.chromium.org/1017183003/diff/120001/Source/core/dom/DOMTyp...
Source/core/dom/DOMTypedArray.h:81: // Should not use CORE_EXPORT when compiling
core by using msvc, because

by using => when using

[tasak, 2015-03-25 11:39:03]

Thank you for review.

https://codereview.chromium.org/1017183003/diff/120001/Source/core/dom/DOMTyp...
File Source/core/dom/DOMTypedArray.h (right):

https://codereview.chromium.org/1017183003/diff/120001/Source/core/dom/DOMTyp...
Source/core/dom/DOMTypedArray.h:81: // Should not use CORE_EXPORT when compiling
core by using msvc, because
On 2015/03/25 09:52:55, haraken wrote:
> 
> by using => when using

Probably, with.

[tasak, 2015-03-25 11:39:07]

The CQ bit was checked by tasak@google.com

[tasak, 2015-03-25 11:39:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/1017183003/#ps140001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-03-25 11:39:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1017183003/140001

[commit-bot: I haz the power, 2015-03-25 13:09:54]

Committed patchset #5 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192534