Introduce DEFINE_ELEMENT_DATA_TYPE_CASTS, and use it

Introduce DEFINE_ELEMENT_DATA_TYPE_CASTS, and use it

As a step to use toFoo, this cl introduces DEFINE_ELEMENT_DATA_TYPE_CASTS, which generates
toUniqueElementData() and toShareableElementData(). Those functions will detect bad type cast.

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

[gyuyoung-inactive, 2014-07-21 10:59:40]

PTAL

[Inactive, 2014-07-21 12:14:00]

https://codereview.chromium.org/405923002/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/405923002/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:3120: const_cast<Element&>(other).m_elementData =
toUniqueElementData(other.m_elementData.get())->makeShareableCopy();
nit: Do we still need the .get() here? I am thinking the macro may generate
helpers for smart pointers already.

https://codereview.chromium.org/405923002/diff/1/Source/core/dom/ElementData.h
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/405923002/diff/1/Source/core/dom/ElementData....
Source/core/dom/ElementData.h:151:
DEFINE_ELEMENT_DATA_TYPE_CASTS(ShareableElementData, false);
nit: I would have introduced a "isShareable()" method and used it here. I find
it not very readable to have a boolean macro parameter here.

https://codereview.chromium.org/405923002/diff/1/Source/core/dom/ElementData....
Source/core/dom/ElementData.h:191:
DEFINE_ELEMENT_DATA_TYPE_CASTS(UniqueElementData, true);
ditto: s/true/isUnique

[gyuyoung-inactive, 2014-07-21 13:04:08]

https://codereview.chromium.org/405923002/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/405923002/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:3120: const_cast<Element&>(other).m_elementData =
toUniqueElementData(other.m_elementData.get())->makeShareableCopy();
On 2014/07/21 12:14:00, Chris Dumez wrote:
> nit: Do we still need the .get() here? I am thinking the macro may generate
> helpers for smart pointers already.

yes, right. I missed to remove it. It will be fixed in next patch.

[Inactive, 2014-07-21 19:37:48]

https://codereview.chromium.org/405923002/diff/20001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/405923002/diff/20001/Source/core/dom/ElementD...
Source/core/dom/ElementData.h:116: template<typename T> inline thisType*
to##thisType(const RefPtr<T>& data) { return to##thisType(data.get()); } \
Sorry, I thought the macro was doing this for us already. If it doesn't, it may
be on purpose (people might find it confusing that a toX() calls converts a
smart pointer into a raw one). My proposal would be to not add it.

Also, why not use DEFINE_TYPE_CASTS() directly instead of having this
intermediate macro? It would probably make introducing isShareable() useless as
we could likely use:
DEFINE_TYPE_CASTS(ShareableElementData, ElementData, data, !data->isUnique(),
!data.isUnique());

[gyuyoung-inactive, 2014-07-22 00:20:33]

https://codereview.chromium.org/405923002/diff/20001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/405923002/diff/20001/Source/core/dom/ElementD...
Source/core/dom/ElementData.h:116: template<typename T> inline thisType*
to##thisType(const RefPtr<T>& data) { return to##thisType(data.get()); } \
On 2014/07/21 19:37:47, Chris Dumez wrote:
> Sorry, I thought the macro was doing this for us already. If it doesn't, it
may
> be on purpose (people might find it confusing that a toX() calls converts a
> smart pointer into a raw one). My proposal would be to not add it.

I removed .get() in issue 401553002 as well. I would like to ask this to Kent
because he suggested to use it. Kent, what do you think ?

> Also, why not use DEFINE_TYPE_CASTS() directly instead of having this
> intermediate macro? It would probably make introducing isShareable() useless
as
> we could likely use:
> DEFINE_TYPE_CASTS(ShareableElementData, ElementData, data, !data->isUnique(),
> !data.isUnique());

Personally I think intermediate macro is more easier to use. I would like to ask
this to Kent too.

[Inactive, 2014-07-22 00:44:29]

https://codereview.chromium.org/405923002/diff/20001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/405923002/diff/20001/Source/core/dom/ElementD...
Source/core/dom/ElementData.h:116: template<typename T> inline thisType*
to##thisType(const RefPtr<T>& data) { return to##thisType(data.get()); } \
On 2014/07/22 00:20:32, gyuyoung wrote:
> On 2014/07/21 19:37:47, Chris Dumez wrote:
> > Sorry, I thought the macro was doing this for us already. If it doesn't, it
> may
> > be on purpose (people might find it confusing that a toX() calls converts a
> > smart pointer into a raw one). My proposal would be to not add it.
> 
> I removed .get() in issue 401553002 as well. I would like to ask this to Kent
> because he suggested to use it. Kent, what do you think ?
> 
> > Also, why not use DEFINE_TYPE_CASTS() directly instead of having this
> > intermediate macro? It would probably make introducing isShareable() useless
> as
> > we could likely use:
> > DEFINE_TYPE_CASTS(ShareableElementData, ElementData, data,
!data->isUnique(),
> > !data.isUnique());
> 
> Personally I think intermediate macro is more easier to use. I would like to
ask
> this to Kent too.

Ok, I see tkent's comment on the other CL, fine then. I don't really mind the
intermediate macro, especially now that we have the RefPtr helper. However, I
still think it would be nice if we did not have to introduce a isShareable()
method, just for this helper.

[tkent, 2014-07-22 00:57:13]

https://codereview.chromium.org/405923002/diff/20001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/405923002/diff/20001/Source/core/dom/ElementD...
Source/core/dom/ElementData.h:116: template<typename T> inline thisType*
to##thisType(const RefPtr<T>& data) { return to##thisType(data.get()); } \
On 2014/07/22 00:44:29, Chris Dumez wrote:
> On 2014/07/22 00:20:32, gyuyoung wrote:
> > On 2014/07/21 19:37:47, Chris Dumez wrote:
> > > Sorry, I thought the macro was doing this for us already. If it doesn't,
it
> > may
> > > be on purpose (people might find it confusing that a toX() calls converts
a
> > > smart pointer into a raw one). My proposal would be to not add it.
> > 
> > I removed .get() in issue 401553002 as well. I would like to ask this to
Kent
> > because he suggested to use it. Kent, what do you think ?
> > 
> > > Also, why not use DEFINE_TYPE_CASTS() directly instead of having this
> > > intermediate macro? It would probably make introducing isShareable()
useless
> > as
> > > we could likely use:
> > > DEFINE_TYPE_CASTS(ShareableElementData, ElementData, data,
> !data->isUnique(),
> > > !data.isUnique());
> > 
> > Personally I think intermediate macro is more easier to use. I would like to
> ask
> > this to Kent too.
> 
> Ok, I see tkent's comment on the other CL, fine then. I don't really mind the
> intermediate macro, especially now that we have the RefPtr helper. However, I
> still think it would be nice if we did not have to introduce a isShareable()
> method, just for this helper.

I agree with Chris.  Adding the intermediate macro is ok, but I feel adding
isShareable is unnecessary.

[gyuyoung-inactive, 2014-07-22 02:09:52]

https://codereview.chromium.org/405923002/diff/40001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/405923002/diff/40001/Source/core/dom/ElementD...
Source/core/dom/ElementData.h:152:
DEFINE_ELEMENT_DATA_TYPE_CASTS(ShareableElementData, isUnique() == 0);
Ok, what do you guys think about this style ?

[tkent, 2014-07-22 03:19:31]

https://codereview.chromium.org/405923002/diff/40001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/405923002/diff/40001/Source/core/dom/ElementD...
Source/core/dom/ElementData.h:152:
DEFINE_ELEMENT_DATA_TYPE_CASTS(ShareableElementData, isUnique() == 0);
On 2014/07/22 02:09:52, gyuyoung wrote:
> Ok, what do you guys think about this style ? 

It looks weird.

I like:

#define DEFINE_ELEMENT_DATA_TYPE_CASTS(thisType, pointerPredicate,
referencePredicate) \
    templete<...  \
    DEFINE_TYPE_CASTS(thisType, ElementData, data, pointerPredicate,
referencePredicate)

DEFINE_ELEMENT_DATA_TYPE_CASTS(ShareableElementData, !data->isUnique(),
!data.isUnique());

[gyuyoung-inactive, 2014-07-22 03:46:19]

https://codereview.chromium.org/405923002/diff/40001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/405923002/diff/40001/Source/core/dom/ElementD...
Source/core/dom/ElementData.h:152:
DEFINE_ELEMENT_DATA_TYPE_CASTS(ShareableElementData, isUnique() == 0);
On 2014/07/22 03:19:31, tkent wrote:
> On 2014/07/22 02:09:52, gyuyoung wrote:
> > Ok, what do you guys think about this style ? 
> 
> It looks weird.
> 
> I like:
> 
> #define DEFINE_ELEMENT_DATA_TYPE_CASTS(thisType, pointerPredicate,
> referencePredicate) \
>     templete<...  \
>     DEFINE_TYPE_CASTS(thisType, ElementData, data, pointerPredicate,
> referencePredicate)
> 
> DEFINE_ELEMENT_DATA_TYPE_CASTS(ShareableElementData, !data->isUnique(),
> !data.isUnique());

Done.

[tkent, 2014-07-22 05:13:23]

Have you run any tests with Patch Set 4?

[gyuyoung-inactive, 2014-07-22 06:12:00]

On 2014/07/22 05:13:23, tkent wrote:
> Have you run any tests with Patch Set 4?

[gyuyoung-inactive, 2014-07-22 06:12:51]

On 2014/07/22 06:12:00, gyuyoung wrote:
> On 2014/07/22 05:13:23, tkent wrote:
> > Have you run any tests with Patch Set 4?

I missed to add ! to toShareableElementData generation. And, I tested whole
layout test with this last patch.

[tkent, 2014-07-22 06:25:09]

lgtm

[commit-bot: I haz the power, 2014-07-22 06:25:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/405923002/70001

[commit-bot: I haz the power, 2014-07-22 07:29:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18126)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-22 08:16:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-22 08:16:59]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18147)

[gyuyoung-inactive, 2014-07-22 10:32:24]

The CQ bit was checked by gyuyoung.kim@samsung.com

[commit-bot: I haz the power, 2014-07-22 10:33:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/405923002/70001

[commit-bot: I haz the power, 2014-07-22 11:38:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18222)

[commit-bot: I haz the power, 2014-07-22 12:23:02]

Change committed as 178643