Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1684653003:
sandwich: Adds web page replay support for HTTP (Closed)
Descriptionsandwich: Adds web page replay support for HTTP
Adds to option --wpr-{archive,record} to respectively run with or
generate the benchmark's Web Page Replay archive. Https urls
support are postponed to another CL.
BUG=582080
Committed: https://crrev.com/90f06edabf6cb2880141ad3dfecd775a6559db2d
Cr-Commit-Position: refs/heads/master@{#374662}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Unmaps only forwarder side ports #
Total comments: 1
Patch Set 3 : Fixes pylint failure with [] as a default argument #
Dependent Patchsets: Messages
Total messages: 21 (8 generated)
gabadie@chromium.org changed reviewers: + lizeb@chromium.org, pasko@chromium.org
lizeb@chromium.org: Pasko being OOO, please review changes in tools/android/loading/
gabadie@chromium.org changed reviewers: + mattcary@chromium.org
Adding matt to the review. PTAL changes in tools/android/loading/ =D
https://codereview.chromium.org/1684653003/diff/1/tools/android/loading/devic... File tools/android/loading/device_setup.py (right): https://codereview.chromium.org/1684653003/diff/1/tools/android/loading/devic... tools/android/loading/device_setup.py:20: from devil.android import forwarder Wrong import ordering (move to line 19). https://codereview.chromium.org/1684653003/diff/1/tools/android/loading/devic... tools/android/loading/device_setup.py:30: from telemetry.internal.util import webpagereplay Is webpagereplay in catapult now? https://codereview.chromium.org/1684653003/diff/1/tools/android/loading/devic... tools/android/loading/device_setup.py:115: """Launch web page replay host. nit: Launches the https://codereview.chromium.org/1684653003/diff/1/tools/android/loading/devic... tools/android/loading/device_setup.py:154: forwarder.Forwarder.UnmapAllDevicePorts(device) I don't know whether that's relevant here, but this removes all mappings at once, so wouldn't that conflict with other redirections, such as the devtools one?
lgtm https://codereview.chromium.org/1684653003/diff/1/tools/android/loading/devic... File tools/android/loading/device_setup.py (right): https://codereview.chromium.org/1684653003/diff/1/tools/android/loading/devic... tools/android/loading/device_setup.py:149: yield [ This is fine for now. Note that if the WPR interface gets more complicated, it might be nicer to have something like class WprHostImpl(object): def __init__(self, flags): self.chrome_flags = flags Then have this return a WprHostImpl instance rather than just a flag list. That would make the context manager return value read a little more naturally. But since all we have now are the flags I don't think we gain anything by generalizing the interface.
lizeb@chromium.org: PTAL changes in tools/android/loading/. https://codereview.chromium.org/1684653003/diff/1/tools/android/loading/devic... File tools/android/loading/device_setup.py (right): https://codereview.chromium.org/1684653003/diff/1/tools/android/loading/devic... tools/android/loading/device_setup.py:20: from devil.android import forwarder On 2016/02/10 13:58:27, Benoit L wrote: > Wrong import ordering (move to line 19). Done. https://codereview.chromium.org/1684653003/diff/1/tools/android/loading/devic... tools/android/loading/device_setup.py:30: from telemetry.internal.util import webpagereplay On 2016/02/10 13:58:27, Benoit L wrote: > Is webpagereplay in catapult now? Webpage replay it self is in its own third_party tools. But here I reused a class from telemetry to avoid code effort: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... https://codereview.chromium.org/1684653003/diff/1/tools/android/loading/devic... tools/android/loading/device_setup.py:115: """Launch web page replay host. On 2016/02/10 13:58:27, Benoit L wrote: > nit: Launches the Done. https://codereview.chromium.org/1684653003/diff/1/tools/android/loading/devic... tools/android/loading/device_setup.py:149: yield [ On 2016/02/10 14:01:01, mattcary wrote: > This is fine for now. Note that if the WPR interface gets more complicated, it > might be nicer to have something like > > class WprHostImpl(object): > def __init__(self, flags): > self.chrome_flags = flags > > Then have this return a WprHostImpl instance rather than just a flag list. That > would make the context manager return value read a little more naturally. > > But since all we have now are the flags I don't think we gain anything by > generalizing the interface. Acknowledged. https://codereview.chromium.org/1684653003/diff/1/tools/android/loading/devic... tools/android/loading/device_setup.py:154: forwarder.Forwarder.UnmapAllDevicePorts(device) On 2016/02/10 13:58:27, Benoit L wrote: > I don't know whether that's relevant here, but this removes all mappings at > once, so wouldn't that conflict with other redirections, such as the devtools > one? Indeed but since there is a dependency, that is running fine. I'll remove only the two port then. Done.
https://codereview.chromium.org/1684653003/diff/20001/tools/android/loading/d... File tools/android/loading/device_setup.py (right): https://codereview.chromium.org/1684653003/diff/20001/tools/android/loading/d... tools/android/loading/device_setup.py:29: sys.path.append(chromium_config.GetTelemetryDir()) You can avoid adding tools/perf to the import path by adding third_party/catapult/telemetry.
On 2016/02/10 14:29:44, Benoit L wrote: > https://codereview.chromium.org/1684653003/diff/20001/tools/android/loading/d... > File tools/android/loading/device_setup.py (right): > > https://codereview.chromium.org/1684653003/diff/20001/tools/android/loading/d... > tools/android/loading/device_setup.py:29: > sys.path.append(chromium_config.GetTelemetryDir()) > You can avoid adding tools/perf to the import path by adding > third_party/catapult/telemetry. I thought it was a not good idea since they did it exactly the way I did here: https://code.google.com/p/chromium/codesearch#chromium/src/tools/cygprofile/p...
On 2016/02/10 14:41:59, gabadie wrote: > On 2016/02/10 14:29:44, Benoit L wrote: > > > https://codereview.chromium.org/1684653003/diff/20001/tools/android/loading/d... > > File tools/android/loading/device_setup.py (right): > > > > > https://codereview.chromium.org/1684653003/diff/20001/tools/android/loading/d... > > tools/android/loading/device_setup.py:29: > > sys.path.append(chromium_config.GetTelemetryDir()) > > You can avoid adding tools/perf to the import path by adding > > third_party/catapult/telemetry. > > I thought it was a not good idea since they did it exactly the way I did here: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/cygprofile/p... I see. lgtm, thank you.
The CQ bit was checked by gabadie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattcary@chromium.org Link to the patchset: https://codereview.chromium.org/1684653003/#ps20001 (title: "Unmaps only forwarder side ports")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1684653003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684653003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by gabadie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org, mattcary@chromium.org Link to the patchset: https://codereview.chromium.org/1684653003/#ps40001 (title: "Fixes pylint failure with [] as a default argument")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1684653003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684653003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
sandwich: Adds web page replay support for HTTP
Adds to option --wpr-{archive,record} to respectively run with or
generate the benchmark's Web Page Replay archive. Https urls
support are postponed to another CL.
BUG=582080
==========
to
==========
sandwich: Adds web page replay support for HTTP
Adds to option --wpr-{archive,record} to respectively run with or
generate the benchmark's Web Page Replay archive. Https urls
support are postponed to another CL.
BUG=582080
Committed: https://crrev.com/90f06edabf6cb2880141ad3dfecd775a6559db2d
Cr-Commit-Position: refs/heads/master@{#374662}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/90f06edabf6cb2880141ad3dfecd775a6559db2d Cr-Commit-Position: refs/heads/master@{#374662} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
