Refactor chromedriver's script execution to reduce amount of custom Value parsing.

Refactor chromedriver's script execution to reduce amount of custom Value parsing.
Also, rename utility_functions to webdriver_util and alphabetize gyp target.
BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96945

[kkania, 2011-07-28 19:00:47]

sky: chrome/test/automation
hnguyen: everything else

[sky, 2011-07-28 19:28:19]

Since Pawel reviewed earlier versions of this I'm swapping him for me.

  -Scott

[kkania, 2011-08-02 15:02:26]

ping, waiting for phajdan-jr and hnguyen

[Paweł Hajdan Jr., 2011-08-02 20:04:47]

http://codereview.chromium.org/7522024/diff/8001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7522024/diff/8001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:920: '../third_party/mongoose/mongoose.c',
How about making a separate target for it? Should be cleaner. Please re-use it
if those files are already compiled somewhere else.

http://codereview.chromium.org/7522024/diff/8001/chrome/test/webdriver/automa...
File chrome/test/webdriver/automation.cc (right):

http://codereview.chromium.org/7522024/diff/8001/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.cc:288: static_cast<int>(start.y()),
static_cast<int>(end.x()),
Why do we need those static_casts all over the place? It seems like you have
control over everything here, so try to design code so that it doesn't require
those casts.

http://codereview.chromium.org/7522024/diff/8001/chrome/test/webdriver/web_el...
File chrome/test/webdriver/web_element_id.h (right):

http://codereview.chromium.org/7522024/diff/8001/chrome/test/webdriver/web_el...
chrome/test/webdriver/web_element_id.h:66: struct
ValueConversionTraits<webdriver::WebElementId> {
Why here? I think you may risk different behavior depending on whether this
header is included or not.

[kkania, 2011-08-03 17:07:56]

http://codereview.chromium.org/7522024/diff/8001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7522024/diff/8001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:920: '../third_party/mongoose/mongoose.c',
On 2011/08/02 20:04:47, Paweł Hajdan Jr. wrote:
> How about making a separate target for it? Should be cleaner. Please re-use it
> if those files are already compiled somewhere else.

Done.

http://codereview.chromium.org/7522024/diff/8001/chrome/test/webdriver/automa...
File chrome/test/webdriver/automation.cc (right):

http://codereview.chromium.org/7522024/diff/8001/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.cc:288: static_cast<int>(start.y()),
static_cast<int>(end.x()),
On 2011/08/02 20:04:47, Paweł Hajdan Jr. wrote:
> Why do we need those static_casts all over the place? It seems like you have
> control over everything here, so try to design code so that it doesn't require
> those casts.

Done.

http://codereview.chromium.org/7522024/diff/8001/chrome/test/webdriver/web_el...
File chrome/test/webdriver/web_element_id.h (right):

http://codereview.chromium.org/7522024/diff/8001/chrome/test/webdriver/web_el...
chrome/test/webdriver/web_element_id.h:66: struct
ValueConversionTraits<webdriver::WebElementId> {
I was not quite sure where to put it myself. I thought it would be best to keep
it next to the thing it is converting though. The ValueConversionTraits are
defined in test_support_common, while this is only used in chromedriver. I
didn't want to pull chromedriver stuff into there.

If this header is not included and you try to convert a WebElementId, the code
won't compile. Plus, if you are going to convert to a WebElementId, you should
include the header in the file by the include what you use rule.

[Paweł Hajdan Jr., 2011-08-04 17:51:36]

http://codereview.chromium.org/7522024/diff/8001/chrome/test/webdriver/web_el...
File chrome/test/webdriver/web_element_id.h (right):

http://codereview.chromium.org/7522024/diff/8001/chrome/test/webdriver/web_el...
chrome/test/webdriver/web_element_id.h:66: struct
ValueConversionTraits<webdriver::WebElementId> {
On 2011/08/03 17:07:56, kkania wrote:
> I was not quite sure where to put it myself. I thought it would be best to
keep
> it next to the thing it is converting though. The ValueConversionTraits are
> defined in test_support_common, while this is only used in chromedriver. I
> didn't want to pull chromedriver stuff into there.
> 
> If this header is not included and you try to convert a WebElementId, the code
> won't compile. Plus, if you are going to convert to a WebElementId, you should
> include the header in the file by the include what you use rule.

After thinking more about it I still think it'd be safer to have all those
template instantiations in a single header file. If you have an example of the
code, docs, or an opinion of C++ expert that would suggest otherwise, I'm open
to change that opinion.

[kkania, 2011-08-04 18:01:23]

+mark, just for a question

Mark,

I have a templated struct ValueConversionTraits, with specializations in
value_conversion_traits.h. This code lives in a common test support target. I
also have another specialization for a specific class in a separate target,
chromedriver, in web_element_id.h. Pawel suggested that all the specializations
should live in the same header. If I do that though, I will have to bring in
extra sources to the common test support target that don't really belong there.

Do you have any recommendations here? Is it prudent to have all specializations
in the same place?

[kkania, 2011-08-08 19:59:58]

ping; waiting for mmentovai

[kkania, 2011-08-11 22:14:16]

+jcivelli, not for review, just for a question

Can you answer Pawel's concern?

I have a templated struct ValueConversionTraits, with specializations in
value_conversion_traits.h. This code lives in a common test support target. I
also have another specialization for a specific class in a separate target,
chromedriver, in web_element_id.h. Pawel suggested that all the specializations
should live in the same header. If I do that though, I will have to bring in
extra sources to the common test support target that don't really belong there.

Do you have any recommendations here? Is it prudent to have all specializations
in the same place?

[Jay Civelli, 2011-08-12 00:00:17]

> Can you answer Pawel's concern?
Sorry, I don't think I qualify as a C++ expert.
May be pkasting would have an opinion?

Jay

>
> I have a templated struct ValueConversionTraits, with specializations in
> value_conversion_traits.h. This code lives in a common test support target.
> I
> also have another specialization for a specific class in a separate target,
> chromedriver, in web_element_id.h. Pawel suggested that all the
> specializations
> should live in the same header. If I do that though, I will have to bring in
> extra sources to the common test support target that don't really belong
> there.
>
> Do you have any recommendations here? Is it prudent to have all
> specializations
> in the same place?
>
> http://codereview.chromium.org/7522024/
>

[Peter Kasting, 2011-08-12 01:18:06]

On 2011/08/11 22:14:16, kkania wrote:
> Do you have any recommendations here? Is it prudent to have all
specializations
> in the same place?

Looking at your change, I think what you are doing is fine.  You are giving the
specialization for WebElementId in the same header that defines WebElementId,
which means no one can write code that tries to instantiate
ValueConversionTraits<WebElementId> without seeing your header.  So I don't
think there's a safety risk, and as you note, this allows you to keep things a
little more separated.

[Paweł Hajdan Jr., 2011-08-12 16:40:14]

LGTM, thanks Peter!

[Huyen, 2011-08-12 23:24:51]

lgtm

[commit-bot: I haz the power, 2011-08-15 18:43:54]

Can't process patch for file chrome/test/webdriver/webdriver_util_mac.mm.
A +