Enable CNS to serve files from different port.

Enable CNS to serve files from different port.

Add --local-server-port option that allows CNS to serve files from different server port.
This allows us to serve files from an available local Apache port.


BUG=140594
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150944

[shadi, 2012-08-03 23:08:47]

Dale, can you please take a look?

[DaleCurtis, 2012-08-07 23:53:57]

http://codereview.chromium.org/10824173/diff/2001/media/tools/constrained_net...
File media/tools/constrained_network_server/cns.py (right):

http://codereview.chromium.org/10824173/diff/2001/media/tools/constrained_net...
media/tools/constrained_network_server/cns.py:221: constrained_url =
'%s?f=%s&no_cache=%s&%s' % (
This looks similar to the self._GetLocalServerURL() can we collapse these two
methods into one? This code is already kind of ugly, no need to have it twice :)

http://codereview.chromium.org/10824173/diff/2001/media/tools/constrained_net...
media/tools/constrained_network_server/cns.py:240: """Checks if requested f
exists to be served by this server."""
s/f/file f/

http://codereview.chromium.org/10824173/diff/2001/media/tools/constrained_net...
media/tools/constrained_network_server/cns.py:248: def
_CheckFileExistOnLocalServer(self, f):
Hmmm, what is urlopen doing behind the scenes here? This is more expensive than
it's worth if it's actually pulling down the remote file.

If we want to keep this we could require --www-data-dir and --local-server-port
to be specified and just use the original os.path.isfile() check.

http://codereview.chromium.org/10824173/diff/2001/media/tools/constrained_net...
media/tools/constrained_network_server/cns.py:262:
cherrypy.lib.cptools.redirect(redirect_url, internal=False)
Do you need to return anything here to keep CherryPy happy with
ServeConstrained?

http://codereview.chromium.org/10824173/diff/2001/media/tools/constrained_net...
media/tools/constrained_network_server/cns.py:366:
parser.add_option('--www-root', default='',
just remove the default='' ?

http://codereview.chromium.org/10824173/diff/2001/media/tools/constrained_net...
File media/tools/constrained_network_server/cns_test.py (left):

http://codereview.chromium.org/10824173/diff/2001/media/tools/constrained_net...
media/tools/constrained_network_server/cns_test.py:39:
self._pa.Cleanup(_INTERFACE, all_ports=True)
Is this no longer necessary / incorrect?

[shadi, 2012-08-08 22:55:49]

https://chromiumcodereview.appspot.com/10824173/diff/2001/media/tools/constra...
File media/tools/constrained_network_server/cns.py (right):

https://chromiumcodereview.appspot.com/10824173/diff/2001/media/tools/constra...
media/tools/constrained_network_server/cns.py:221: constrained_url =
'%s?f=%s&no_cache=%s&%s' % (
On 2012/08/07 23:53:57, DaleCurtis wrote:
> This looks similar to the self._GetLocalServerURL() can we collapse these two
> methods into one? This code is already kind of ugly, no need to have it twice
:)
They are not actually that similar, but Done. :-)

https://chromiumcodereview.appspot.com/10824173/diff/2001/media/tools/constra...
media/tools/constrained_network_server/cns.py:240: """Checks if requested f
exists to be served by this server."""
On 2012/08/07 23:53:57, DaleCurtis wrote:
> s/f/file f/

Done.

https://chromiumcodereview.appspot.com/10824173/diff/2001/media/tools/constra...
media/tools/constrained_network_server/cns.py:248: def
_CheckFileExistOnLocalServer(self, f):
On 2012/08/07 23:53:57, DaleCurtis wrote:
> Hmmm, what is urlopen doing behind the scenes here? This is more expensive
than
> it's worth if it's actually pulling down the remote file.
> 
> If we want to keep this we could require --www-data-dir and
--local-server-port
> to be specified and just use the original os.path.isfile() check.
No it is not pulling down the remote file. It returns a socket file object which
pulls down all the file when a read() is done.
It pulls down the headers and little data. Using urllib allows checking the URL
itself would work and not just that the file exist.

https://chromiumcodereview.appspot.com/10824173/diff/2001/media/tools/constra...
media/tools/constrained_network_server/cns.py:262:
cherrypy.lib.cptools.redirect(redirect_url, internal=False)
On 2012/08/07 23:53:57, DaleCurtis wrote:
> Do you need to return anything here to keep CherryPy happy with
> ServeConstrained?
CherryPy has not been complaining :-)

https://chromiumcodereview.appspot.com/10824173/diff/2001/media/tools/constra...
media/tools/constrained_network_server/cns.py:366:
parser.add_option('--www-root', default='',
On 2012/08/07 23:53:57, DaleCurtis wrote:
> just remove the default='' ?
I actually put this so that www_root == '' when --local-server-port is used.

https://chromiumcodereview.appspot.com/10824173/diff/2001/media/tools/constra...
File media/tools/constrained_network_server/cns_test.py (left):

https://chromiumcodereview.appspot.com/10824173/diff/2001/media/tools/constra...
media/tools/constrained_network_server/cns_test.py:39:
self._pa.Cleanup(_INTERFACE, all_ports=True)
On 2012/08/07 23:53:57, DaleCurtis wrote:
> Is this no longer necessary / incorrect?
Not necessary, Cleanup() does a clean up on allocated ports which already have
the interface value stored in a map.

[DaleCurtis, 2012-08-09 17:28:02]

lgtm

http://codereview.chromium.org/10824173/diff/5001/media/tools/constrained_net...
File media/tools/constrained_network_server/cns.py (right):

http://codereview.chromium.org/10824173/diff/5001/media/tools/constrained_net...
media/tools/constrained_network_server/cns.py:272: base_url = '%s/%s' %
(cherrypy.url().replace('ServeConstrained',
Cleaner as:

base_url = '%s/%s' % (
    cherrypy.url().replace('ServeConstrained', self._options.www_root), f)

Or:
url = cherrypy.url()
if self._options.local_server_port:
  url = url.replace('ServeConstrained', self._options.www_root)

base_url = '%s?f=%s' % (url, f)

I'm confused though, shouldn't www_root always be blank here? Why not just use
url.replace('ServeConstrained', '') ? Then you can remove the default='' below?

http://codereview.chromium.org/10824173/diff/5001/media/tools/constrained_net...
media/tools/constrained_network_server/cns.py:278: delimeter = '&'
You can probably just roll the &, ? into the if above, I don't think it should
hurt to have these empty.

[shadi, 2012-08-09 18:26:49]

Dale, any comments concerning the use of www_root before I commit?

http://codereview.chromium.org/10824173/diff/5001/media/tools/constrained_net...
File media/tools/constrained_network_server/cns.py (right):

http://codereview.chromium.org/10824173/diff/5001/media/tools/constrained_net...
media/tools/constrained_network_server/cns.py:272: base_url = '%s/%s' %
(cherrypy.url().replace('ServeConstrained',
On 2012/08/09 17:28:02, DaleCurtis wrote:
> Cleaner as:
> 
> base_url = '%s/%s' % (
>     cherrypy.url().replace('ServeConstrained', self._options.www_root), f)
> 
> Or:
> url = cherrypy.url()
> if self._options.local_server_port:
>   url = url.replace('ServeConstrained', self._options.www_root)
> 
> base_url = '%s?f=%s' % (url, f)
> 
> I'm confused though, shouldn't www_root always be blank here? Why not just use
> url.replace('ServeConstrained', '') ? Then you can remove the default=''
below?
The www_root used with local-server-port is relative to redirecting url. Example
if www_root = media, then the redirect URL is :
http://localhost:80/media/filename.

http://codereview.chromium.org/10824173/diff/5001/media/tools/constrained_net...
media/tools/constrained_network_server/cns.py:278: delimeter = '&'
On 2012/08/09 17:28:02, DaleCurtis wrote:
> You can probably just roll the &, ? into the if above, I don't think it should
> hurt to have these empty.

Done.

[DaleCurtis, 2012-08-09 18:46:31]

Otherwise still looks good.

http://codereview.chromium.org/10824173/diff/5001/media/tools/constrained_net...
File media/tools/constrained_network_server/cns.py (right):

http://codereview.chromium.org/10824173/diff/5001/media/tools/constrained_net...
media/tools/constrained_network_server/cns.py:272: base_url = '%s/%s' %
(cherrypy.url().replace('ServeConstrained',
On 2012/08/09 18:26:49, shadi wrote:
> On 2012/08/09 17:28:02, DaleCurtis wrote:
> > Cleaner as:
> > 
> > base_url = '%s/%s' % (
> >     cherrypy.url().replace('ServeConstrained', self._options.www_root), f)
> > 
> > Or:
> > url = cherrypy.url()
> > if self._options.local_server_port:
> >   url = url.replace('ServeConstrained', self._options.www_root)
> > 
> > base_url = '%s?f=%s' % (url, f)
> > 
> > I'm confused though, shouldn't www_root always be blank here? Why not just
use
> > url.replace('ServeConstrained', '') ? Then you can remove the default=''
> below?
> The www_root used with local-server-port is relative to redirecting url.
Example
> if www_root = media, then the redirect URL is :
> http://localhost:80/media/filename. 

You need to update the --options documentation for that.

[commit-bot: I haz the power, 2012-08-09 21:10:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shadi@chromium.org/10824173/12001

[commit-bot: I haz the power, 2012-08-09 23:41:03]

Change committed as 150944