Split Dictionary's get and convert into DictionaryHelper.

Split Dictionary's get and convert into DictionaryHelper.

Dictionary's get and convert depends on both core and modules code.
So the get and convert logic is split into DictionaryHelper.

DictionaryHelper is a class which has static template methods: get and convert:
- the methods are implemented in DictionaryHelperForCore.cpp and DictionaryHelperForModules.cpp.
- DictionaryHelperForCore.cpp has get and convert for core code.
- DictionaryHelperForModules.cpp has get and convert for modules code.

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

[tasak, 2014-07-09 07:21:53]

Would you review this CL?

The most important part of this patch is DictionaryHelper.h,
DictonaryHelperForCore.cpp, and DictionaryHelperForModules.cpp.

[bashi, 2014-07-09 07:58:13]

On 2014/07/09 07:21:53, tasak wrote:
> Would you review this CL?
> 
> The most important part of this patch is DictionaryHelper.h,
> DictonaryHelperForCore.cpp, and DictionaryHelperForModules.cpp.

(Not actually reviewing)

As chatted offline, this CL would conflict my current work (IDL dictionary
support), but my work will take time so please feel free to proceed:)

[tasak, 2014-07-09 08:07:40]

bashi-san,
Thank you.

haraken-san, 
I'm sorry. I'm now working on fixing compile failure in win_blink.

On 2014/07/09 07:58:13, bashi1 wrote:
> On 2014/07/09 07:21:53, tasak wrote:
> > Would you review this CL?
> > 
> > The most important part of this patch is DictionaryHelper.h,
> > DictonaryHelperForCore.cpp, and DictionaryHelperForModules.cpp.
> 
> (Not actually reviewing)
> 
> As chatted offline, this CL would conflict my current work (IDL dictionary
> support), but my work will take time so please feel free to proceed:)

[Nils Barth (inactive), 2014-07-09 13:20:38]

A couple GYP/Python nits.

https://codereview.chromium.org/373423002/diff/50001/Source/bindings/core/v8/...
File Source/bindings/core/v8/v8.gypi (right):

https://codereview.chromium.org/373423002/diff/50001/Source/bindings/core/v8/...
Source/bindings/core/v8/v8.gypi:34: 'DictionaryHelperForCore.cpp',
alpha (longer after shorter)

https://codereview.chromium.org/373423002/diff/50001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/373423002/diff/50001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:200:
includes.add('bindings/core/v8/Dictionary.h')
set.update, so:
includes.update(['bindings/core/v8/Dictionary.h',
                 'bindings/core/v8/DictionaryHelper.h'])

Even better:
v8_types.add_includes_for_type('Dictionary')

[tasak, 2014-07-10 04:52:08]

Thank you for reviewing.

https://codereview.chromium.org/373423002/diff/50001/Source/bindings/core/v8/...
File Source/bindings/core/v8/v8.gypi (right):

https://codereview.chromium.org/373423002/diff/50001/Source/bindings/core/v8/...
Source/bindings/core/v8/v8.gypi:34: 'DictionaryHelperForCore.cpp',
On 2014/07/09 13:20:37, Nils Barth wrote:
> alpha (longer after shorter)

Done.

https://codereview.chromium.org/373423002/diff/50001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/373423002/diff/50001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:200:
includes.add('bindings/core/v8/Dictionary.h')
On 2014/07/09 13:20:37, Nils Barth wrote:
> set.update, so:
> includes.update(['bindings/core/v8/Dictionary.h',
>                  'bindings/core/v8/DictionaryHelper.h'])
> 
> Even better:
> v8_types.add_includes_for_type('Dictionary')

Done.

[haraken, 2014-07-10 15:05:48]

Sorry about the review delay. Mostly looks good.

It looks like you're basically just splitting the existing code into "ForCore"
and "ForModule". However, as far as I see the number of edited lines, you're
newly adding more than 350 lines. Where does the 350 lines come from?

(BTW, the current Dictionary code is full of code duplication and templates. We
should clean them up but that's a separate issue from this CL.)

https://codereview.chromium.org/373423002/diff/70001/Source/bindings/core/v8/...
File Source/bindings/core/v8/DictionaryHelper.h (right):

https://codereview.chromium.org/373423002/diff/70001/Source/bindings/core/v8/...
Source/bindings/core/v8/DictionaryHelper.h:12: struct DictionaryHelper {

I'd put DictionaryHelper into Dictionary.h, so that Dictionary users don't need
to include two files.

https://codereview.chromium.org/373423002/diff/70001/Source/bindings/core/v8/...
File Source/bindings/core/v8/DictionaryHelperForCore.cpp (right):

https://codereview.chromium.org/373423002/diff/70001/Source/bindings/core/v8/...
Source/bindings/core/v8/DictionaryHelperForCore.cpp:733: bool
DictionaryHelper::convert(const Dictionary& dictionary,
Dictionary::ConversionContext& context, const String& key, Nullable< int>&
value)

Nullable< int> => Nullable<int>

[tasak, 2014-07-11 04:51:23]

Thank you for reviewing.

> It looks like you're basically just splitting the existing code into "ForCore"
and "ForModule". However, as far as I see the number of edited lines, you're
newly adding more than 350 lines. Where does the 350 lines come from?

Now (+949 lines, -817 lines) = +132lines.
24x3 = 72 lines are caused by LICENSE.

The left +60lines, I think, are caused by manual template instantiation (for get
and convert).

https://codereview.chromium.org/373423002/diff/70001/Source/bindings/core/v8/...
File Source/bindings/core/v8/DictionaryHelper.h (right):

https://codereview.chromium.org/373423002/diff/70001/Source/bindings/core/v8/...
Source/bindings/core/v8/DictionaryHelper.h:12: struct DictionaryHelper {
On 2014/07/10 15:05:47, haraken wrote:
> 
> I'd put DictionaryHelper into Dictionary.h, so that Dictionary users don't
need
> to include two files.

Done.

https://codereview.chromium.org/373423002/diff/70001/Source/bindings/core/v8/...
File Source/bindings/core/v8/DictionaryHelperForCore.cpp (right):

https://codereview.chromium.org/373423002/diff/70001/Source/bindings/core/v8/...
Source/bindings/core/v8/DictionaryHelperForCore.cpp:733: bool
DictionaryHelper::convert(const Dictionary& dictionary,
Dictionary::ConversionContext& context, const String& key, Nullable< int>&
value)
On 2014/07/10 15:05:47, haraken wrote:
> 
> Nullable< int> => Nullable<int>

Done.

[haraken, 2014-07-11 05:23:46]

OK, LGTM.

The code is insanely dirty, but it's not worse than the current code. Let's
consider cleaning up the Dictionary code when implementing the IDL dictionary.

[tasak, 2014-07-11 06:32:51]

Thank you for reviewing.

On 2014/07/11 05:23:46, haraken wrote:
> OK, LGTM.
> 
> The code is insanely dirty, but it's not worse than the current code. Let's
> consider cleaning up the Dictionary code when implementing the IDL dictionary.

Yeah, the code after applying this patch is as dirty as the previous code.

In the future, the get/convert code will be automatically generated if needed.

[tasak, 2014-07-11 06:56:26]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-07-11 06:57:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tasak@google.com/373423002/110001

[commit-bot: I haz the power, 2014-07-11 07:03:15]

Change committed as 177908