Issue 38470044: Add trafficShaping script

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment
d :	mark current comment as done

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

Issue 38470044: Add trafficShaping script (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
10 years, 9 months ago by wrightt

Modified:
10 years, 7 months ago

Reviewers:
klm

Visibility:
Public.

Description

https://github.com/WPO-Foundation/webpagetest/issues/147 OBSOLETE replaced with https://codereview.appspot.com/58340043

Patch Set 1 #

Total comments: 4

Created: 10 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+423 lines, -211 lines)			Patch
M	agent/js/src/traffic_shaper.js	View	3 chunks	+82 lines, -209 lines	2 comments	Download
A	agent/js/src/traffic_shaper_ipfw.js	View	1 chunk	+296 lines, -0 lines	2 comments	Download
M	agent/js/test/traffic_shaper_tests_small.js	View	3 chunks	+45 lines, -2 lines	0 comments	Download

Messages

Total messages: 2

Expand All Messages | Collapse All Messages

klm

https://codereview.appspot.com/38470044/diff/1/agent/js/src/traffic_shaper.js File agent/js/src/traffic_shaper.js (right): https://codereview.appspot.com/38470044/diff/1/agent/js/src/traffic_shaper.js#newcode36 agent/js/src/traffic_shaper.js:36: * #param {string=} deviceSerial the device to drive. The ...

10 years, 9 months ago (2013-12-13 16:25:00 UTC) #1

https://codereview.appspot.com/38470044/diff/1/agent/js/src/traffic_shaper.js
File agent/js/src/traffic_shaper.js (right):

https://codereview.appspot.com/38470044/diff/1/agent/js/src/traffic_shaper.js...
agent/js/src/traffic_shaper.js:36: *   #param {string=} deviceSerial the device
to drive.
The agent is common for desktop and mobile browsers, at least desktop Chrome,
where the serial# isn't applicable. Perhaps Appurify needs it? Please mention in
the comment that it's undefined for desktop browsers.

https://codereview.appspot.com/38470044/diff/1/agent/js/src/traffic_shaper.js...
agent/js/src/traffic_shaper.js:49:
args.trafficShaper.match(/\/?traffic_shaper_ipfw\.js$/)) {
I really don't like how it's done here, especially how it's different from the
browser module/class selection. Could we do the same as we do for browsers? My
impression is we pass a flag with the module+class name and then pass our full
flags to the constructor, so each specific class is free to take whatever it
needs from the flags.

Alternatively, I would not be against fully shelling this out, just like you
already do for the video -- meaning even ipfw would be shelled out to a script
with generic args. You would manage ipfw rule#'s e.g. in a locked file, perhaps
managing the lock explicitly as a separate file, in case some of our OS
(Windows?) don't support locked files, or if file locking is not properly
exposed in NodeJS.

https://codereview.appspot.com/38470044/diff/1/agent/js/src/traffic_shaper_ip...
File agent/js/src/traffic_shaper_ipfw.js (right):

https://codereview.appspot.com/38470044/diff/1/agent/js/src/traffic_shaper_ip...
agent/js/src/traffic_shaper_ipfw.js:54: // We'll assume the default
configuration is:
This is essentially the module comment. Please move this to the top of the
module, before or after the imports.

https://codereview.appspot.com/38470044/diff/1/agent/js/src/traffic_shaper_ip...
agent/js/src/traffic_shaper_ipfw.js:134:
TrafficShaperIpfw.prototype.scheduleIpfw_ = function(opts) {
I assume this is an unmodified copy of the existing ipfw handling code, correct?

Replaced with 58340043

Expand All Messages | Collapse All Messages