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?
Issue 38470044: Add trafficShaping script
(Closed)
Created 10 years, 9 months ago by wrightt
Modified 10 years, 7 months ago
Reviewers: klm
Base URL:
Comments: 4