[ChromeDriver] Support mobile emulation on desktop Chrome.

[ChromeDriver] Support mobile emulation on desktop Chrome.

Add Device Metrics override support to ChromeDriver via Capabilities; only width, height, pixel_ratio supported

BUG=chromedriver:399

[stgao, 2014-04-30 03:32:55]

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/cap...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/cap...
chrome/test/chromedriver/capabilities.cc:93: if (!metrics->GetInteger("width",
&(capabilities->device_metrics.width)) ||
Code in chromedriver seems not to use "()" to wrap the variable like
"capabilities->device_metrics.width". Maybe remove it if no compile error
occurs?

Same for the two below.

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/cap...
chrome/test/chromedriver/capabilities.cc:95: !metrics->GetDouble("pixel_ratio",
&(capabilities->device_metrics.device_scale_factor)))
over 80 chars.

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/cap...
File chrome/test/chromedriver/capabilities.h (right):

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/cap...
chrome/test/chromedriver/capabilities.h:95: // Device metrics for use in Device
Emulation
style nit: A comment ends with a dot.

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/web_view.h (right):

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/web_view.h:24: struct DeviceMetrics;
Not in order.

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/web_view_impl.cc (right):

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/web_view_impl.cc:373: return
device_metrics_override_manager_->OverrideDeviceMetrics(device_metrics);
Over 80 chars.

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/web_view_impl.h (right):

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/web_view_impl.h:87: virtual Status
OverrideDeviceMetrics(const DeviceMetrics& device_metrics) OVERRIDE;
Over 80 chars.

[stgao, 2014-05-03 00:15:19]

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/cap...
File chrome/test/chromedriver/capabilities.h (right):

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/cap...
chrome/test/chromedriver/capabilities.h:96: DeviceMetrics device_metrics;
I made a comment about this in the design doc. Please have a check.

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/device_metrics_override_manager.h (right):

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/device_metrics_override_manager.h:30: Status
OverrideDeviceMetrics(const DeviceMetrics& DeviceMetrics);
Maybe we don't have to add this function, as we don't change the metrics setting
at runtime. We could pass it in through the constructor.

For each tab (new or existing), before we run window-based commands, we will
connect to it first. And "OnConnected" will be automatically called before we do
anything else.

We just have to send the override there in "OnConnected".

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/web_view.h (right):

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/web_view.h:144: virtual Status
OverrideDeviceMetrics(const DeviceMetrics& device_metrics) = 0;
Same as my comment in device_metrics_override_manager.h, we could use the
"OnConnected" event to override the metrics through DevTools.
So we don't have to override device metrics for each WebView explicitly.

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session_commands.cc (right):

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands.cc:164: if
(capabilities.device_metrics.width != 0) {
Instead of creating an instance of struct |device_metrics| in capabilities.h, we
could wrap it up with a scoped_ptr.

In this way, we could test the scoped_ptr to check if an emulation is requested.

[sam.rawlins, 2014-05-06 23:51:45]

Hi Shuotao, Sam,

I implemented everything we talked about yesterday, EXCEPT for the python script
:) So this CL still has the inline list of 3x devices in capabilities.cc.

Also, you can see here the huge amount of changes required to pass a
DeviceMetrics into the WebViewImpl constructor (chrome_launcher has to send the
DeviceMetrics to ChromeDesktopImpl, so that it can use it in GetWebViewIds, to
send it to WebViewImpl, so that WebViewImpl can construct the
DeviceMetricsOverrideManager, adding itself to the DevToolsClient as a
listener.) I think it might be better if a DevToolsClient were to manage the
DeviceMetricsOverrideManager, but this current code works for now. What do you
think?

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/cap...
File chrome/test/chromedriver/capabilities.h (right):

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/cap...
chrome/test/chromedriver/capabilities.h:95: // Device metrics for use in Device
Emulation
On 2014/04/30 03:32:55, Shuotao wrote:
> style nit: A comment ends with a dot.

Done.

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/device_metrics_override_manager.h (right):

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/device_metrics_override_manager.h:30: Status
OverrideDeviceMetrics(const DeviceMetrics& DeviceMetrics);
On 2014/05/03 00:15:20, Shuotao wrote:
> Maybe we don't have to add this function, as we don't change the metrics
setting
> at runtime. We could pass it in through the constructor.
> 
> For each tab (new or existing), before we run window-based commands, we will
> connect to it first. And "OnConnected" will be automatically called before we
do
> anything else.
> 
> We just have to send the override there in "OnConnected".

Done.

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/web_view.h (right):

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/web_view.h:24: struct DeviceMetrics;
On 2014/04/30 03:32:55, Shuotao wrote:
> Not in order.

Done.

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/web_view.h:144: virtual Status
OverrideDeviceMetrics(const DeviceMetrics& device_metrics) = 0;
On 2014/05/03 00:15:20, Shuotao wrote:
> Same as my comment in device_metrics_override_manager.h, we could use the
> "OnConnected" event to override the metrics through DevTools.
> So we don't have to override device metrics for each WebView explicitly.

Done.

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session_commands.cc (right):

https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands.cc:164: if
(capabilities.device_metrics.width != 0) {
On 2014/05/03 00:15:20, Shuotao wrote:
> Instead of creating an instance of struct |device_metrics| in capabilities.h,
we
> could wrap it up with a scoped_ptr.
> 
> In this way, we could test the scoped_ptr to check if an emulation is
requested.

Done.

[samuong, 2014-05-07 23:39:36]

Mostly just minor style comments from me.

There are a few files where you've got lines longer than 80 characters. I think
the presubmit script is supposed to automatically catch these for you when you
try to upload the CL, but if not you can run "git cl presubmit" and it will tell
you what to fix.

I think it makes sense that the DeviceMetricsOverridesManager is be owned by
WebViewImpl, but yeah I can see there's a lot of plumbing involved.

How about we make it so that DevToolsHttpClient owns the DeviceMetrics object?
Then ChromeDesktopImpl/ChromeImpl can fetch it out of devtools_http_client_ when
constructing the WebViewImpl.

This would save you from having to pass around the DeviceMetrics object all over
the place, and would be similar to how DevToolsHttpClient owns the BrowserInfo
object, which is used by NavigationTracker.

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:104: return Status(kUnknownError, "must
be a valid device");
It would be good to also mention what the invalid device was in the error
message.

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:127: return Status(kUnknownError, "must
be a dictionary");
There are a few error messages that just say "must be a dictionary", and when
something fails it can be hard to figure out what the error message is referring
to. It would be good to mention what it is that must be a dictionary.

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:155:
capabilities->device_metrics.font_scale_factor = 1;
This is done a few times. Should we put this in a constructor?

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:359: base::Bind(&ParseMobileEmulation);
Can be one line.

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.h (right):

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.h:19: #include
"chrome/test/chromedriver/mobile_device.h"
This #include is unnecessary here, but should be moved to capabilities.cc since
it's missing from that file.

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/test/webserver.py (right):

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/test/webserver.py:168: self._path_data_lock.acquire()
Rename this lock?

[sam.rawlins, 2014-05-09 05:35:16]

Thanks Sam! You advice regarding where to put the DeviceMetrics was just what I
needed! Moving it to DevtoolsHttpClient removed a lot of code.

I added the python script which seems to work beautifully, and JSONReader can
parse the resultant string just fine. This should be feature complete now.

I added a bunch of unittests to capabilities. Let me know if it was too many :P

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:104: return Status(kUnknownError, "must
be a valid device");
On 2014/05/07 23:39:36, samuong wrote:
> It would be good to also mention what the invalid device was in the error
> message.

Done.

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:127: return Status(kUnknownError, "must
be a dictionary");
On 2014/05/07 23:39:36, samuong wrote:
> There are a few error messages that just say "must be a dictionary", and when
> something fails it can be hard to figure out what the error message is
referring
> to. It would be good to mention what it is that must be a dictionary.

Done.

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:155:
capabilities->device_metrics.font_scale_factor = 1;
On 2014/05/07 23:39:36, samuong wrote:
> This is done a few times. Should we put this in a constructor?

Done.

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:359: base::Bind(&ParseMobileEmulation);
On 2014/05/07 23:39:36, samuong wrote:
> Can be one line.

Done.

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.h (right):

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.h:19: #include
"chrome/test/chromedriver/mobile_device.h"
On 2014/05/07 23:39:36, samuong wrote:
> This #include is unnecessary here, but should be moved to capabilities.cc
since
> it's missing from that file.

Done.

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/test/webserver.py (right):

https://codereview.chromium.org/251933005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/test/webserver.py:168: self._path_data_lock.acquire()
On 2014/05/07 23:39:36, samuong wrote:
> Rename this lock?

Done.

[samuong, 2014-05-09 17:58:54]

Thanks Sam. I have just a few more comments.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/device_metrics.h (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics.h:8: class DeviceMetrics {
I think it was OK having this as a struct, otherwise we'd have to name the
member variables differently and create a whole set of accessor functions.

Rather than check if width<0, can't we just check whether device_metrics==NULL?
(this might involve using a scoped_ptr in Capabilities)

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/devtools_http_client.h (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/devtools_http_client.h:83:
scoped_ptr<DeviceMetrics> device_metrics_;
This should be private, with an accessor function like

const DeviceMetrics* device_metrics();

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/mobile_device.cc (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/mobile_device.cc:36: (*it)->GetAsList(&device);
GetAsList, as well as GetString, StringToInt and StringToDouble below, all
return a bool which we should be checking for.

[stgao, 2014-05-14 05:10:13]

Sorry for my late review.

Look like we are much more closer to get the CL into ChromeDriver.

https://codereview.chromium.org/251933005/diff/50001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:422:
'test/chromedriver/chrome/device_metrics_override_manager.cc',
Do you mind renaming it to mobile_emulation_manager so that we could reuse it
later for similar features like orientation?

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:104: Capabilities* capabilities) {
indent.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:137: if
(mobile_emulation->HasKey("userAgent")) {
Do we want to support the case that the user provides only one of deviceMetrics
and userAgent?

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.h (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.h:96: DeviceMetrics device_metrics;
We could wrap this into a scoped_ptr like the |local_state| below.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities_unittest.cc (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities_unittest.cc:407: ASSERT_EQ("'Mozilla/5.0
(Linux; Android 4.2.1; en-us; Nexus 5 Build/JOP40D) AppleWebKit/535.19 (KHTML,
like Gecko) Chrome/18.0.1025.166 Mobile Safari/535.19'",
capabilities.switches.GetSwitchValue("user-agent"));
over 80 chars.

It could be like this:
ASSERT_EQ(
    "balba... 80 chars"
    "balba... 80 chars"
    "now ends",
    capabilities.switches.GetSwitchValue("user-agent");

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/device_metrics.cc (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics.cc:11: height(height),
indent

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/device_metrics_override_manager.cc (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics_override_manager.cc:14: if
(overridden_device_metrics_->IsValid()) {
It is better to check "overridden_device_metrics_ == NULL" like you do below in
DeviceMetricsOverrideManager::ApplyOverrideIfNeeded.

And this will cause a segment fall in the case of "new WebViewImpl(*it,
&browser_info_, client.Pass(), NULL)".

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics_override_manager.cc:16: }
"{}" could be removed.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/device_metrics_override_manager.h (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics_override_manager.h:28: explicit
DeviceMetricsOverrideManager(DevToolsClient* client,
explicit is used only for constructor with a single parameter like the one
above.
So it needs removing for this constructor.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome_launcher.cc (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome_launcher.cc:199: capabilities,
&devtools_client);
For this case "Existing Chrome Session", the browser is already launched, we
won't be able to set the command line switch "user-agent".

Maybe pass a NULL here.
In WaitForDevToolsAndCheckVersion check if it is NULL and do appropriate bailout
there.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/client/webelement.py (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/client/webelement.py:30: Command.GET_ELEMENT_TEXT)
make it one line instead of two?

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/mobile_device.cc (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/mobile_device.cc:25: "Could not parse mobile device
list because " +
lowercase for error message. "Could" -> "could".

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:831: def
respondWithUserAgentString(request):
This method could be moved into GlobalSetUp, as it is not related a the
test/setup directly.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:856: "'Mozilla/5.0 (Linux; Android
4.2.1; en-us; Nexus 5 Build/JOP40D) AppleWebKit/535.19 (KHTML, like Gecko)
Chrome/18.0.1025.166 Mobile Safari/535.19'",
over 80 chars.
We use 'str' instead of "str".

It could be like this:
self.assertEqual(
    'balbla...80chars'
    'balbla...ends',
    body_tag.GetText())

[sam.rawlins, 2014-05-16 21:09:37]

Reviewed all comments. Thanks much, this is looking much better!!!

https://codereview.chromium.org/251933005/diff/50001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:422:
'test/chromedriver/chrome/device_metrics_override_manager.cc',
On 2014/05/14 05:10:14, Shuotao wrote:
> Do you mind renaming it to mobile_emulation_manager so that we could reuse it
> later for similar features like orientation?

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:104: Capabilities* capabilities) {
On 2014/05/14 05:10:14, Shuotao wrote:
> indent.

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:137: if
(mobile_emulation->HasKey("userAgent")) {
On 2014/05/14 05:10:14, Shuotao wrote:
> Do we want to support the case that the user provides only one of
deviceMetrics
> and userAgent?
I think this is fine. The behavior wouldn't be undefined or unexpected. A user
might be confused if they set deviceMetrics without userAgent, and only got the
desktop version of a website. We just have to rely on general web knowledge.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.h (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.h:96: DeviceMetrics device_metrics;
On 2014/05/14 05:10:14, Shuotao wrote:
> We could wrap this into a scoped_ptr like the |local_state| below.

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities_unittest.cc (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities_unittest.cc:407: ASSERT_EQ("'Mozilla/5.0
(Linux; Android 4.2.1; en-us; Nexus 5 Build/JOP40D) AppleWebKit/535.19 (KHTML,
like Gecko) Chrome/18.0.1025.166 Mobile Safari/535.19'",
capabilities.switches.GetSwitchValue("user-agent"));
On 2014/05/14 05:10:14, Shuotao wrote:
> over 80 chars.
> 
> It could be like this:
> ASSERT_EQ(
>     "balba... 80 chars"
>     "balba... 80 chars"
>     "now ends",
>     capabilities.switches.GetSwitchValue("user-agent");

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/device_metrics.cc (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics.cc:11: height(height),
On 2014/05/14 05:10:14, Shuotao wrote:
> indent

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/device_metrics.h (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics.h:8: class DeviceMetrics {
On 2014/05/09 17:58:54, samuong wrote:
> I think it was OK having this as a struct, otherwise we'd have to name the
> member variables differently and create a whole set of accessor functions.
> 
> Rather than check if width<0, can't we just check whether
device_metrics==NULL?
> (this might involve using a scoped_ptr in Capabilities)

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/device_metrics_override_manager.cc (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics_override_manager.cc:14: if
(overridden_device_metrics_->IsValid()) {
On 2014/05/14 05:10:14, Shuotao wrote:
> It is better to check "overridden_device_metrics_ == NULL" like you do below
in
> DeviceMetricsOverrideManager::ApplyOverrideIfNeeded.
> 
> And this will cause a segment fall in the case of "new WebViewImpl(*it,
> &browser_info_, client.Pass(), NULL)".

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics_override_manager.cc:16: }
On 2014/05/14 05:10:14, Shuotao wrote:
> "{}" could be removed.

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/device_metrics_override_manager.h (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics_override_manager.h:28: explicit
DeviceMetricsOverrideManager(DevToolsClient* client,
On 2014/05/14 05:10:14, Shuotao wrote:
> explicit is used only for constructor with a single parameter like the one
> above.
> So it needs removing for this constructor.

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/devtools_http_client.h (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/devtools_http_client.h:83:
scoped_ptr<DeviceMetrics> device_metrics_;
On 2014/05/09 17:58:54, samuong wrote:
> This should be private, with an accessor function like
> 
> const DeviceMetrics* device_metrics();

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome_launcher.cc (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome_launcher.cc:199: capabilities,
&devtools_client);
On 2014/05/14 05:10:14, Shuotao wrote:
> For this case "Existing Chrome Session", the browser is already launched, we
> won't be able to set the command line switch "user-agent".
> 
> Maybe pass a NULL here.
> In WaitForDevToolsAndCheckVersion check if it is NULL and do appropriate
bailout
> there.

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/client/webelement.py (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/client/webelement.py:30: Command.GET_ELEMENT_TEXT)
On 2014/05/14 05:10:14, Shuotao wrote:
> make it one line instead of two?

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/mobile_device.cc (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/mobile_device.cc:25: "Could not parse mobile device
list because " +
On 2014/05/14 05:10:14, Shuotao wrote:
> lowercase for error message. "Could" -> "could".

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/mobile_device.cc:36: (*it)->GetAsList(&device);
On 2014/05/09 17:58:54, samuong wrote:
> GetAsList, as well as GetString, StringToInt and StringToDouble below, all
> return a bool which we should be checking for.

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:831: def
respondWithUserAgentString(request):
On 2014/05/14 05:10:14, Shuotao wrote:
> This method could be moved into GlobalSetUp, as it is not related a the
> test/setup directly.

Done.

https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:856: "'Mozilla/5.0 (Linux; Android
4.2.1; en-us; Nexus 5 Build/JOP40D) AppleWebKit/535.19 (KHTML, like Gecko)
Chrome/18.0.1025.166 Mobile Safari/535.19'",
On 2014/05/14 05:10:14, Shuotao wrote:
> over 80 chars.
> We use 'str' instead of "str".
> 
> It could be like this:
> self.assertEqual(
>     'balbla...80chars'
>     'balbla...ends',
>     body_tag.GetText())

Done.

[samuong, 2014-05-16 22:56:21]

Thanks Sam! I noticed there's one more small bug with the user agent.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:100: "--user-agent='" +
device->user_agent + "'");
I noticed that if you use SetUnparsedSwitch, the single-quote characters become
part of your user agent. To see this, try going to chrome://version (or
whatsmyuseragent.com) and checking "User Agent" both with and without enabling
your mobile emulation feature.

So we actually don't want these quotes here and should change it to:

capabilities->switches.SetUnparsedSwitch("--user-agent=" + device->user_agent);

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:148: "--user-agent='" + user_agent +
"'");
Same as above.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities_unittest.cc (right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities_unittest.cc:376: ASSERT_EQ("'Agent
Smith'",
Should be ASSERT_EQ("Agent Smith"

i.e. no single quote characters - these shouldn't be part of the user agent

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/device_metrics.h (right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics.h:9: public:
We usually leave off the "public:" for structs.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:861: '535.19\'',
Don't surround the user agent in single quotes.

[samuong, 2014-05-16 23:00:35]

Also, I noticed that we don't need the no-arg DeviceMetrics() constructor, and
we can get rid of it if you make the following changes.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/device_metrics.cc (right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics.cc:7:
DeviceMetrics::DeviceMetrics() {}
Delete.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/device_metrics.h (right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics.h:10: DeviceMetrics();
Delete.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File
chrome/test/chromedriver/chrome/mobile_emulation_override_manager_unittest.cc
(right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/mobile_emulation_override_manager_unittest.cc:83:
DeviceMetrics device_metrics = DeviceMetrics(1, 2, 3.0);
Change this to:

DeviceMetrics device_metrics(1, 2, 3.0);

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/mobile_emulation_override_manager_unittest.cc:97:
DeviceMetrics device_metrics = DeviceMetrics(1, 2, 3.0);
Change this to:

DeviceMetrics device_metrics(1, 2, 3.0);

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome_launcher.cc (right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome_launcher.cc:164: *device_metrics =
*capabilities->device_metrics;
Change these two lines to:

device_metrics.reset(new DeviceMetrics(*capabilities->device_metrics));

And once you've done this you can also get rid of the braces.

[sam.rawlins, 2014-05-16 23:35:52]

Thanks for looking, Sam! All cleaned up.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:100: "--user-agent='" +
device->user_agent + "'");
On 2014/05/16 22:56:21, samuong wrote:
> I noticed that if you use SetUnparsedSwitch, the single-quote characters
become
> part of your user agent. To see this, try going to chrome://version (or
> http://whatsmyuseragent.com) and checking "User Agent" both with and without
enabling
> your mobile emulation feature.
> 
> So we actually don't want these quotes here and should change it to:
> 
> capabilities->switches.SetUnparsedSwitch("--user-agent=" +
device->user_agent);

Done.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:148: "--user-agent='" + user_agent +
"'");
On 2014/05/16 22:56:21, samuong wrote:
> Same as above.

Done.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities_unittest.cc (right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities_unittest.cc:376: ASSERT_EQ("'Agent
Smith'",
On 2014/05/16 22:56:21, samuong wrote:
> Should be ASSERT_EQ("Agent Smith"
> 
> i.e. no single quote characters - these shouldn't be part of the user agent

Done.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/device_metrics.cc (right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics.cc:7:
DeviceMetrics::DeviceMetrics() {}
On 2014/05/16 23:00:35, samuong wrote:
> Delete.

Done.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/device_metrics.h (right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics.h:9: public:
On 2014/05/16 22:56:21, samuong wrote:
> We usually leave off the "public:" for structs.

Done.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/device_metrics.h:10: DeviceMetrics();
On 2014/05/16 23:00:35, samuong wrote:
> Delete.

Done.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File
chrome/test/chromedriver/chrome/mobile_emulation_override_manager_unittest.cc
(right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/mobile_emulation_override_manager_unittest.cc:83:
DeviceMetrics device_metrics = DeviceMetrics(1, 2, 3.0);
On 2014/05/16 23:00:35, samuong wrote:
> Change this to:
> 
> DeviceMetrics device_metrics(1, 2, 3.0);

Done.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/mobile_emulation_override_manager_unittest.cc:97:
DeviceMetrics device_metrics = DeviceMetrics(1, 2, 3.0);
On 2014/05/16 23:00:35, samuong wrote:
> Change this to:
> 
> DeviceMetrics device_metrics(1, 2, 3.0);

Done.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome_launcher.cc (right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome_launcher.cc:164: *device_metrics =
*capabilities->device_metrics;
On 2014/05/16 23:00:35, samuong wrote:
> Change these two lines to:
> 
> device_metrics.reset(new DeviceMetrics(*capabilities->device_metrics));
> 
> And once you've done this you can also get rid of the braces.

Done.

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:861: '535.19\'',
On 2014/05/16 22:56:21, samuong wrote:
> Don't surround the user agent in single quotes.

Done.

[samuong, 2014-05-16 23:46:40]

Thanks, LGTM. Feel free to commit if Shuotao is happy with everything.

[stgao, 2014-05-19 17:34:01]

lgtm with a few comments on some nits.

Once they are addressed and chromedriver_unittests/chromedriver_tests pass,
please feel free to check the commit box for CQ of this CL.

https://codereview.chromium.org/251933005/diff/90001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/251933005/diff/90001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:462: 'test/chromedriver/mobile_device.h',
I think we'd better move these two files into "test/chromedriver/chrome/", as
logically they are information about Chrome.

WDYT?

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:99:
capabilities->switches.SetUnparsedSwitch(
Use the method below instead?

void SetSwitch(const std::string& name, const std::string& value);

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:147:
capabilities->switches.SetUnparsedSwitch("--user-agent=" + user_agent);
Same as above.

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/web_view.h (right):

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/web_view.h:22: struct DeviceMetrics;
Seems unused.

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:815: class
MobileEmulationCapabilityTest(ChromeDriverBaseTest):
This test suite have to be disabled on Android.
Please refer to how "ChromeExtensionsCapabilityTest.*" is disabled at the
beginning of this file.

[sam.rawlins, 2014-05-19 21:19:35]

https://codereview.chromium.org/251933005/diff/90001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/251933005/diff/90001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:462: 'test/chromedriver/mobile_device.h',
On 2014/05/19 17:34:02, Shuotao wrote:
> I think we'd better move these two files into "test/chromedriver/chrome/", as
> logically they are information about Chrome.
> 
> WDYT?

I agree, principally. However, this does mean that there will be files:
* test/chromedriver/chrome/mobile_device.cc
* test/chromedriver/chrome/mobile_device.h
* test/chromedriver/chrome/mobile_devices.cc (generated by
embed_mobile_devices_in_cpp.py)
* test/chromedriver/chrome/mobile_devices.h

Should I also rename the auto-generated mobile_devices files?

[stgao, 2014-05-19 23:12:55]

https://codereview.chromium.org/251933005/diff/90001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/251933005/diff/90001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:462: 'test/chromedriver/mobile_device.h',
On 2014/05/19 21:19:35, sam.rawlins wrote:
> On 2014/05/19 17:34:02, Shuotao wrote:
> > I think we'd better move these two files into "test/chromedriver/chrome/",
as
> > logically they are information about Chrome.
> > 
> > WDYT?
> 
> I agree, principally. However, this does mean that there will be files:
> * test/chromedriver/chrome/mobile_device.cc
> * test/chromedriver/chrome/mobile_device.h
> * test/chromedriver/chrome/mobile_devices.cc (generated by
> embed_mobile_devices_in_cpp.py)
> * test/chromedriver/chrome/mobile_devices.h
> 
> Should I also rename the auto-generated mobile_devices files?

Yes, sure. Maybe rename them to mobile_device_list.h/cc or a better name.

[sam.rawlins, 2014-05-20 00:25:20]

Alright, everything addressed. Looks good.

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:99:
capabilities->switches.SetUnparsedSwitch(
On 2014/05/19 17:34:02, Shuotao wrote:
> Use the method below instead?
> 
> void SetSwitch(const std::string& name, const std::string& value);

Done.

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:147:
capabilities->switches.SetUnparsedSwitch("--user-agent=" + user_agent);
On 2014/05/19 17:34:02, Shuotao wrote:
> Same as above.

Done.

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/web_view.h (right):

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/web_view.h:22: struct DeviceMetrics;
On 2014/05/19 17:34:02, Shuotao wrote:
> Seems unused.

Done. Moved to web_view_impl.h

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:815: class
MobileEmulationCapabilityTest(ChromeDriverBaseTest):
On 2014/05/19 17:34:02, Shuotao wrote:
> This test suite have to be disabled on Android.
> Please refer to how "ChromeExtensionsCapabilityTest.*" is disabled at the
> beginning of this file.

Done.

[stgao, 2014-05-20 00:50:55]

lgtm

let's go for CQ!

[commit-bot: I haz the power, 2014-05-20 00:51:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sam.rawlins@gmail.com/251933005/110001

[commit-bot: I haz the power, 2014-05-20 04:24:04]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-20 04:29:58]

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

[commit-bot: I haz the power, 2014-05-20 04:29:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[samuong, 2014-05-20 17:24:28]

On 2014/05/20 04:29:58, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

This is failing because your @gmail.com email address isn't in the AUTHORS file:

** Presubmit Warnings **
sam.rawlins@gmail.com is not in AUTHORS file. If you are a new contributor,
please visit
http://www.chromium.org/developers/contributing-code and read the "Legal"
section
If you are a chromite, verify the contributor signed the CLA.

Do you have an @chromium.org account? Or maybe you need to submit this using
your @google.com address...

[stgao, 2014-05-20 17:31:15]

We also hit a warning.
Sorry, I might mislead you for without "!=NULL".

FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma/gomacc
"E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe"
/nologo /showIncludes /FC
@obj\chrome\test\chromedriver\chrome\automation_client_lib.chrome_desktop_impl.obj.rsp
/c ..\..\chrome\test\chromedriver\chrome\chrome_desktop_impl.cc
/Foobj\chrome\test\chromedriver\chrome\automation_client_lib.chrome_desktop_impl.obj
/Fdobj\chrome\automation_client_lib.cc.pdb
e:\b\build\slave\win\build\src\chrome\test\chromedriver\chrome\chrome_desktop_impl.cc(160)
:error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win\build\src\chrome\test\chromedriver\chrome\chrome_desktop_impl.cc(160)
: warning C4800: 'const DeviceMetrics *' : forcing value to bool
'true' or 'false' (performance warning)
ninja: build stopped: subcommand failed.




On Tue, May 20, 2014 at 10:24 AM, <samuong@chromium.org> wrote:

> On 2014/05/20 04:29:58, I haz the power (commit-bot) wrote:
>
>> Try jobs failed on following builders:
>>    chromium_presubmit on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> chromium_presubmit/builds/68557)
>
> This is failing because your @gmail.com email address isn't in the
> AUTHORS file:
>
> ** Presubmit Warnings **
> sam.rawlins@gmail.com is not in AUTHORS file. If you are a new
> contributor,
> please visit
> http://www.chromium.org/developers/contributing-code and read the "Legal"
> section
> If you are a chromite, verify the contributor signed the CLA.
>
> Do you have an @chromium.org account? Or maybe you need to submit this
> using
> your @google.com address...
>
> https://codereview.chromium.org/251933005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stgao, 2014-05-29 17:29:45]

Just for record: this CL was copied to
https://codereview.chromium.org/288193004/ and committed as
https://src.chromium.org/viewvc/chrome?revision=271947&view=revision