Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(435)

Issue 251933005: [ChromeDriver] Support mobile emulation on desktop Chrome. (Closed)

Created:
6 years, 8 months ago by sam.rawlins
Modified:
6 years, 6 months ago
Reviewers:
samuong, stgao
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

[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

Patch Set 1 #

Total comments: 15

Patch Set 2 : Cleaning up Mobile Emulation for ChromeDriver after code review #

Patch Set 3 : Convert Capability to mobileEmulation; add integration- and unit-tests #

Total comments: 12

Patch Set 4 : #

Total comments: 34

Patch Set 5 : Cleaning up DeviceMetrics architecture; adding a test #

Total comments: 20

Patch Set 6 : Addressing review comments #

Total comments: 11

Patch Set 7 : Addressing recent comments about Mobile Device Emulation #

Patch Set 8 : Fix implicit bool conversion warning #

Unified diffs Side-by-side diffs Delta from patch set Stats (+643 lines, -72 lines) Patch
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 5 chunks +28 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/capabilities.h View 1 2 3 4 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/capabilities.cc View 1 2 3 4 5 6 3 chunks +66 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/capabilities_unittest.cc View 1 2 3 4 5 1 chunk +95 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome/chrome.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome/chrome_desktop_impl.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome/chrome_desktop_impl.cc View 1 2 3 4 5 6 7 2 chunks +6 lines, -1 line 0 comments Download
M chrome/test/chromedriver/chrome/chrome_impl.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome/chrome_impl.cc View 1 2 3 4 2 chunks +8 lines, -1 line 0 comments Download
A chrome/test/chromedriver/chrome/device_metrics.h View 1 2 3 4 5 1 chunk +21 lines, -0 lines 0 comments Download
A chrome/test/chromedriver/chrome/device_metrics.cc View 1 2 3 4 5 1 chunk +16 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome/devtools_http_client.h View 1 2 3 4 4 chunks +5 lines, -1 line 0 comments Download
M chrome/test/chromedriver/chrome/devtools_http_client.cc View 1 2 3 4 4 chunks +10 lines, -3 lines 0 comments Download
A chrome/test/chromedriver/chrome/mobile_device.h View 1 2 3 4 5 6 7 1 chunk +24 lines, -0 lines 0 comments Download
A chrome/test/chromedriver/chrome/mobile_device.cc View 1 2 3 4 5 6 1 chunk +87 lines, -0 lines 0 comments Download
A + chrome/test/chromedriver/chrome/mobile_emulation_override_manager.h View 1 2 3 4 3 chunks +12 lines, -13 lines 0 comments Download
A chrome/test/chromedriver/chrome/mobile_emulation_override_manager.cc View 1 2 3 4 1 chunk +55 lines, -0 lines 0 comments Download
A + chrome/test/chromedriver/chrome/mobile_emulation_override_manager_unittest.cc View 1 2 3 4 5 3 chunks +32 lines, -41 lines 0 comments Download
M chrome/test/chromedriver/chrome/stub_chrome.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome/stub_chrome.cc View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome/web_view_impl.h View 1 2 3 4 5 6 3 chunks +7 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome/web_view_impl.cc View 1 2 3 4 2 chunks +5 lines, -1 line 0 comments Download
M chrome/test/chromedriver/chrome_launcher.cc View 1 2 3 4 5 4 chunks +10 lines, -3 lines 0 comments Download
M chrome/test/chromedriver/client/chromedriver.py View 1 2 2 chunks +5 lines, -1 line 0 comments Download
M chrome/test/chromedriver/client/webelement.py View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
A chrome/test/chromedriver/embed_mobile_devices_in_cpp.py View 1 2 3 4 5 6 1 chunk +48 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/session.h View 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/session_commands.cc View 1 2 3 4 2 chunks +2 lines, -1 line 0 comments Download
M chrome/test/chromedriver/test/run_py_tests.py View 1 2 3 4 5 6 4 chunks +56 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/test/webserver.py View 1 2 3 3 chunks +26 lines, -6 lines 0 comments Download

Messages

Total messages: 24 (0 generated)
stgao
https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/capabilities.cc File chrome/test/chromedriver/capabilities.cc (right): https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/capabilities.cc#newcode93 chrome/test/chromedriver/capabilities.cc:93: if (!metrics->GetInteger("width", &(capabilities->device_metrics.width)) || Code in chromedriver seems not ...
6 years, 7 months ago (2014-04-30 03:32:55 UTC) #1
stgao
https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/capabilities.h File chrome/test/chromedriver/capabilities.h (right): https://codereview.chromium.org/251933005/diff/1/chrome/test/chromedriver/capabilities.h#newcode96 chrome/test/chromedriver/capabilities.h:96: DeviceMetrics device_metrics; I made a comment about this in ...
6 years, 7 months ago (2014-05-03 00:15:19 UTC) #2
sam.rawlins
Hi Shuotao, Sam, I implemented everything we talked about yesterday, EXCEPT for the python script ...
6 years, 7 months ago (2014-05-06 23:51:45 UTC) #3
samuong
Mostly just minor style comments from me. There are a few files where you've got ...
6 years, 7 months ago (2014-05-07 23:39:36 UTC) #4
sam.rawlins
Thanks Sam! You advice regarding where to put the DeviceMetrics was just what I needed! ...
6 years, 7 months ago (2014-05-09 05:35:16 UTC) #5
samuong
Thanks Sam. I have just a few more comments. https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver/chrome/device_metrics.h File chrome/test/chromedriver/chrome/device_metrics.h (right): https://codereview.chromium.org/251933005/diff/50001/chrome/test/chromedriver/chrome/device_metrics.h#newcode8 chrome/test/chromedriver/chrome/device_metrics.h:8: ...
6 years, 7 months ago (2014-05-09 17:58:54 UTC) #6
stgao
Sorry for my late review. Look like we are much more closer to get the ...
6 years, 7 months ago (2014-05-14 05:10:13 UTC) #7
sam.rawlins
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#newcode422 ...
6 years, 7 months ago (2014-05-16 21:09:37 UTC) #8
samuong
Thanks Sam! I noticed there's one more small bug with the user agent. https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver/capabilities.cc File ...
6 years, 7 months ago (2014-05-16 22:56:21 UTC) #9
samuong
Also, I noticed that we don't need the no-arg DeviceMetrics() constructor, and we can get ...
6 years, 7 months ago (2014-05-16 23:00:35 UTC) #10
sam.rawlins
Thanks for looking, Sam! All cleaned up. https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver/capabilities.cc File chrome/test/chromedriver/capabilities.cc (right): https://codereview.chromium.org/251933005/diff/70001/chrome/test/chromedriver/capabilities.cc#newcode100 chrome/test/chromedriver/capabilities.cc:100: "--user-agent='" + ...
6 years, 7 months ago (2014-05-16 23:35:52 UTC) #11
samuong
Thanks, LGTM. Feel free to commit if Shuotao is happy with everything.
6 years, 7 months ago (2014-05-16 23:46:40 UTC) #12
stgao
lgtm with a few comments on some nits. Once they are addressed and chromedriver_unittests/chromedriver_tests pass, ...
6 years, 7 months ago (2014-05-19 17:34:01 UTC) #13
sam.rawlins
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#newcode462 chrome/chrome_tests.gypi:462: 'test/chromedriver/mobile_device.h', On 2014/05/19 17:34:02, Shuotao wrote: > I think ...
6 years, 7 months ago (2014-05-19 21:19:35 UTC) #14
stgao
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#newcode462 chrome/chrome_tests.gypi:462: 'test/chromedriver/mobile_device.h', On 2014/05/19 21:19:35, sam.rawlins wrote: > On 2014/05/19 ...
6 years, 7 months ago (2014-05-19 23:12:55 UTC) #15
sam.rawlins
Alright, everything addressed. Looks good. https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver/capabilities.cc File chrome/test/chromedriver/capabilities.cc (right): https://codereview.chromium.org/251933005/diff/90001/chrome/test/chromedriver/capabilities.cc#newcode99 chrome/test/chromedriver/capabilities.cc:99: capabilities->switches.SetUnparsedSwitch( On 2014/05/19 17:34:02, ...
6 years, 7 months ago (2014-05-20 00:25:20 UTC) #16
stgao
lgtm let's go for CQ!
6 years, 7 months ago (2014-05-20 00:50:55 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sam.rawlins@gmail.com/251933005/110001
6 years, 7 months ago (2014-05-20 00:51:29 UTC) #18
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are ...
6 years, 7 months ago (2014-05-20 04:24:04 UTC) #19
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 7 months ago (2014-05-20 04:29:58 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/builds/68557)
6 years, 7 months ago (2014-05-20 04:29:58 UTC) #21
samuong
On 2014/05/20 04:29:58, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
6 years, 7 months ago (2014-05-20 17:24:28 UTC) #22
stgao
We also hit a warning. Sorry, I might mislead you for without "!=NULL". FAILED: ninja ...
6 years, 7 months ago (2014-05-20 17:31:15 UTC) #23
stgao
6 years, 6 months ago (2014-05-29 17:29:45 UTC) #24
Message was sent while issue was closed.
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

Powered by Google App Engine
This is Rietveld 408576698