Issue 3122 - Add testpages.web.sitekey_frame URL handler

Issue 3122 - Add testpages.web.sitekey_frame URL handler

Code is a stripped down version of Wladimir's example from the sitekey howto PDF.

(Depends on https://codereview.adblockplus.org/29328527/ )

[kzar, 2015-09-24 15:48:18]

Patch Set 1

[Sebastian Noack, 2015-09-29 11:36:30]

Why doesn't that go into the testpages repo?

[Sebastian Noack, 2015-09-29 12:09:32]

On 2015/09/29 11:36:30, Sebastian Noack wrote:
> Why doesn't that go into the testpages repo?

Never mind, I see, this is dynamic content.

https://codereview.adblockplus.org/29328589/diff/29328590/.sitescripts.example
File .sitescripts.example (right):

https://codereview.adblockplus.org/29328589/diff/29328590/.sitescripts.exampl...
.sitescripts.example:193:
sitekeyFrameTemplate=%(root)s/testpages.adblockplus.org/templates/sitekey_frame.tmpl
Wouldn't these assets rather belong into sitescripts now?

https://codereview.adblockplus.org/29328589/diff/29328590/sitescripts/testpag...
File sitescripts/testpages/web/sitekey_frame.py (right):

https://codereview.adblockplus.org/29328589/diff/29328590/sitescripts/testpag...
sitescripts/testpages/web/sitekey_frame.py:34: key.sign_update("\0".join((
Can't you just call sign_update() for each substring rather than doing string
concatenation?

https://codereview.adblockplus.org/29328589/diff/29328590/sitescripts/testpag...
sitescripts/testpages/web/sitekey_frame.py:35: environ.get("REQUEST_URI",
environ.get("PATH_INFO")),
I think wsgiref.util.request_uri() is what you want here.

https://codereview.adblockplus.org/29328589/diff/29328590/sitescripts/testpag...
sitescripts/testpages/web/sitekey_frame.py:42: [("Content-Type", "text/html"),
Charset is missing.

[kzar, 2015-10-07 16:08:45]

Patch Set 2 : Addressed feedback

https://codereview.adblockplus.org/29328589/diff/29328590/.sitescripts.example
File .sitescripts.example (right):

https://codereview.adblockplus.org/29328589/diff/29328590/.sitescripts.exampl...
.sitescripts.example:193:
sitekeyFrameTemplate=%(root)s/testpages.adblockplus.org/templates/sitekey_frame.tmpl
On 2015/09/29 12:09:32, Sebastian Noack wrote:
> Wouldn't these assets rather belong into sitescripts now?

Well in the testpages repository there is an extremely similar frame template
used for the generic hide / block tests. So I would rather sitekey_frame.tmpl
lived there so that I can avoid duplication by using an include for both.

(To see how the templates look so far have a look at my feature branch for the
related testpages changes
https://github.com/kzar/testpages.adblockplus.org/tree/3122-sitekey/templates )

https://codereview.adblockplus.org/29328589/diff/29328590/sitescripts/testpag...
File sitescripts/testpages/web/sitekey_frame.py (right):

https://codereview.adblockplus.org/29328589/diff/29328590/sitescripts/testpag...
sitescripts/testpages/web/sitekey_frame.py:34: key.sign_update("\0".join((
On 2015/09/29 12:09:32, Sebastian Noack wrote:
> Can't you just call sign_update() for each substring rather than doing string
> concatenation?

Done.

https://codereview.adblockplus.org/29328589/diff/29328590/sitescripts/testpag...
sitescripts/testpages/web/sitekey_frame.py:35: environ.get("REQUEST_URI",
environ.get("PATH_INFO")),
On 2015/09/29 12:09:32, Sebastian Noack wrote:
> I think wsgiref.util.request_uri() is what you want here.

I ended up taking part of that function's logic, apparently we wanted only the
path part of the URL.

https://codereview.adblockplus.org/29328589/diff/29328590/sitescripts/testpag...
sitescripts/testpages/web/sitekey_frame.py:42: [("Content-Type", "text/html"),
On 2015/09/29 12:09:32, Sebastian Noack wrote:
> Charset is missing.

Done.

[Sebastian Noack, 2015-10-07 16:17:08]

https://codereview.adblockplus.org/29328589/diff/29328590/sitescripts/testpag...
File sitescripts/testpages/web/sitekey_frame.py (right):

https://codereview.adblockplus.org/29328589/diff/29328590/sitescripts/testpag...
sitescripts/testpages/web/sitekey_frame.py:34: key.sign_update("\0".join((
On 2015/10/07 16:08:44, kzar wrote:
> On 2015/09/29 12:09:32, Sebastian Noack wrote:
> > Can't you just call sign_update() for each substring rather than doing
string
> > concatenation?
> 
> Done.

Well, know you use both, string concatenation and progressive signature updates.
However, its not too important. So LGTM with or w/o this nit addreessed.

[kzar, 2015-10-08 12:06:38]

Patch Set 3 : Removed unrelated commit that was accidentally uploaded previously
Patch Set 4 : Further improvements

https://codereview.adblockplus.org/29328589/diff/29328590/sitescripts/testpag...
File sitescripts/testpages/web/sitekey_frame.py (right):

https://codereview.adblockplus.org/29328589/diff/29328590/sitescripts/testpag...
sitescripts/testpages/web/sitekey_frame.py:34: key.sign_update("\0".join((
On 2015/10/07 16:17:07, Sebastian Noack wrote:
> On 2015/10/07 16:08:44, kzar wrote:
> > On 2015/09/29 12:09:32, Sebastian Noack wrote:
> > > Can't you just call sign_update() for each substring rather than doing
> string
> > > concatenation?
> > 
> > Done.
> 
> Well, know you use both, string concatenation and progressive signature
updates.
> However, its not too important. So LGTM with or w/o this nit addreessed.

Yea, you're right. I tried a few different approaches here, but I think using
"\0".join(...) is probably cleaner than calling key.sign_update 5 times so I've
done it that way.

https://codereview.adblockplus.org/29328589/diff/29328957/sitescripts/web.py
File sitescripts/web.py (right):

https://codereview.adblockplus.org/29328589/diff/29328957/sitescripts/web.py#...
sitescripts/web.py:42: #
https://www.python.org/dev/peps/pep-0333/#url-reconstruction
I realised this function should probably live in sitescripts.web , it will
likely be useful for other scripts.

[Sebastian Noack, 2015-10-09 12:31:39]

LGTM