Issue 3122 - Add sitekey frame files ready for sitekey testcases

Issue 3122 - Add sitekey frame files ready for sitekey testcases

(Depends on https://codereview.adblockplus.org/29325987/ and https://codereview.adblockplus.org/29328589/ )

[kzar, 2015-09-24 17:00:19]

Patch Set 1
Patch Set 2 : Deleted some erroneous whitespace

[kzar, 2015-10-08 12:33:35]

Patch Set 3 : Set data-adblockkey attribute of html element

[rossg, 2015-10-12 12:51:33]

On 2015/10/08 12:33:35, kzar wrote:
> Patch Set 3 : Set data-adblockkey attribute of html element

LGTM

[saroyanm, 2015-10-13 09:55:18]

Noticed the repository bit messy, I'll do a high level review while I'm not
really familiar with most of the dependent changes/repo and seems like for most
of other dependent implementation you got green light.

https://codereview.adblockplus.org/29328600/diff/29328986/templates/sitekey_f...
File templates/sitekey_frame.tmpl (right):

https://codereview.adblockplus.org/29328600/diff/29328986/templates/sitekey_f...
templates/sitekey_frame.tmpl:5: function transform_url(href, hash)
What does this function suppose to do ?
It's called "transform_url", it doesn't use href parameter, but uses hash to
transform URL ?

[kzar, 2015-10-13 11:35:36]

Yes exactly, this repository is a little messy so far but I thought it would be
a good idea to loop you in. I suppose going forward we should start to tidy this
repository up to match the coding standards we have elsewhere.

https://codereview.adblockplus.org/29328600/diff/29328986/templates/sitekey_f...
File templates/sitekey_frame.tmpl (right):

https://codereview.adblockplus.org/29328600/diff/29328986/templates/sitekey_f...
templates/sitekey_frame.tmpl:5: function transform_url(href, hash)
On 2015/10/13 09:55:18, saroyanm wrote:
> What does this function suppose to do ?
> It's called "transform_url", it doesn't use href parameter, but uses hash to
> transform URL ?

I've tried to give these bits better names and make them clearer. Hopefully it's
more obvious now?

The idea is that we so far have two places that use the frame template, the
generic hide tests and this new sitekey_frame template. They are identical
except they need to set the frame URL based on the parent location differently.
So we called the `transform_url` (now `frame_url`) function to figure out what
the frame's URL should be. That way we can avoid duplicating everything else.

(Also have a look at `templates/frame.tmpl` and
`pages/testcases/generic/index.html` if you're interested in seeing the other
parts.)

[saroyanm, 2015-10-13 12:24:44]

https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
File templates/sitekey_frame.tmpl (right):

https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
templates/sitekey_frame.tmpl:1: {% extends "frame.tmpl" %}
"TemplateNotFound: frame.tmpl"
maybe -> {% extends "templates/frame" %} ?

[kzar, 2015-10-15 15:06:21]

https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
File templates/sitekey_frame.tmpl (right):

https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
templates/sitekey_frame.tmpl:1: {% extends "frame.tmpl" %}
On 2015/10/13 12:24:44, saroyanm wrote:
> "TemplateNotFound: frame.tmpl"
> maybe -> {% extends "templates/frame" %} ?

I'm not able to reproduce that problem, when exactly do you hit it? (This
template sitekey_frame.tmpl is designed to be rendered by the related URL
handler in the sitescripts repository.)

[saroyanm, 2015-10-15 15:38:16]

https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
File templates/sitekey_frame.tmpl (right):

https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
templates/sitekey_frame.tmpl:1: {% extends "frame.tmpl" %}
On 2015/10/15 15:06:21, kzar wrote:
> On 2015/10/13 12:24:44, saroyanm wrote:
> > "TemplateNotFound: frame.tmpl"
> > maybe -> {% extends "templates/frame" %} ?
> 
> I'm not able to reproduce that problem, when exactly do you hit it? (This
> template sitekey_frame.tmpl is designed to be rendered by the related URL
> handler in the sitescripts repository.)

Assigne this template to some page `template=sitekey_frame`
Ex.: change template of `pages/testcases/generic/01.md` from "testcase" to
"sitekey_frame"

[kzar, 2015-10-15 16:19:13]

https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
File templates/sitekey_frame.tmpl (right):

https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
templates/sitekey_frame.tmpl:1: {% extends "frame.tmpl" %}
On 2015/10/15 15:38:16, saroyanm wrote:
> On 2015/10/15 15:06:21, kzar wrote:
> > On 2015/10/13 12:24:44, saroyanm wrote:
> > > "TemplateNotFound: frame.tmpl"
> > > maybe -> {% extends "templates/frame" %} ?
> > 
> > I'm not able to reproduce that problem, when exactly do you hit it? (This
> > template sitekey_frame.tmpl is designed to be rendered by the related URL
> > handler in the sitescripts repository.)
> 
> Assigne this template to some page `template=sitekey_frame`
> Ex.: change template of `pages/testcases/generic/01.md` from "testcase" to
> "sitekey_frame"

Ah right I see, well it's designed to be rendered by sitescripts and not used as
a CMS page template. I wouldn't expect that to work.

[saroyanm, 2015-10-15 16:23:24]

https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
File templates/sitekey_frame.tmpl (right):

https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
templates/sitekey_frame.tmpl:1: {% extends "frame.tmpl" %}
On 2015/10/15 16:19:12, kzar wrote:
> On 2015/10/15 15:38:16, saroyanm wrote:
> > On 2015/10/15 15:06:21, kzar wrote:
> > > On 2015/10/13 12:24:44, saroyanm wrote:
> > > > "TemplateNotFound: frame.tmpl"
> > > > maybe -> {% extends "templates/frame" %} ?
> > > 
> > > I'm not able to reproduce that problem, when exactly do you hit it? (This
> > > template sitekey_frame.tmpl is designed to be rendered by the related URL
> > > handler in the sitescripts repository.)
> > 
> > Assigne this template to some page `template=sitekey_frame`
> > Ex.: change template of `pages/testcases/generic/01.md` from "testcase" to
> > "sitekey_frame"
> 
> Ah right I see, well it's designed to be rendered by sitescripts and not used
as
> a CMS page template. I wouldn't expect that to work.

But if you use -> {% extends "templates/frame" %} looks to work fine.

[kzar, 2015-10-15 16:28:52]

https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
File templates/sitekey_frame.tmpl (right):

https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
templates/sitekey_frame.tmpl:1: {% extends "frame.tmpl" %}
On 2015/10/15 16:23:23, saroyanm wrote:
> On 2015/10/15 16:19:12, kzar wrote:
> > On 2015/10/15 15:38:16, saroyanm wrote:
> > > On 2015/10/15 15:06:21, kzar wrote:
> > > > On 2015/10/13 12:24:44, saroyanm wrote:
> > > > > "TemplateNotFound: frame.tmpl"
> > > > > maybe -> {% extends "templates/frame" %} ?
> > > > 
> > > > I'm not able to reproduce that problem, when exactly do you hit it?
(This
> > > > template sitekey_frame.tmpl is designed to be rendered by the related
URL
> > > > handler in the sitescripts repository.)
> > > 
> > > Assigne this template to some page `template=sitekey_frame`
> > > Ex.: change template of `pages/testcases/generic/01.md` from "testcase" to
> > > "sitekey_frame"
> > 
> > Ah right I see, well it's designed to be rendered by sitescripts and not
used
> as
> > a CMS page template. I wouldn't expect that to work.
> 
> But if you use -> {% extends "templates/frame" %} looks to work fine.

So this template is designed to be rendered by
sitescripts.testpages.web.sitekey_frame . If I omit the ".tmpl" extension that
URL handler can no longer render the template - it breaks. This template is not
designed to be used by CMS pages, it does not matter if it does not work for
them.

[saroyanm, 2015-10-15 16:46:38]

On 2015/10/15 16:28:52, kzar wrote:
>
https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
> File templates/sitekey_frame.tmpl (right):
> 
>
https://codereview.adblockplus.org/29328600/diff/29329086/templates/sitekey_f...
> templates/sitekey_frame.tmpl:1: {% extends "frame.tmpl" %}
> On 2015/10/15 16:23:23, saroyanm wrote:
> > On 2015/10/15 16:19:12, kzar wrote:
> > > On 2015/10/15 15:38:16, saroyanm wrote:
> > > > On 2015/10/15 15:06:21, kzar wrote:
> > > > > On 2015/10/13 12:24:44, saroyanm wrote:
> > > > > > "TemplateNotFound: frame.tmpl"
> > > > > > maybe -> {% extends "templates/frame" %} ?
> > > > > 
> > > > > I'm not able to reproduce that problem, when exactly do you hit it?
> (This
> > > > > template sitekey_frame.tmpl is designed to be rendered by the related
> URL
> > > > > handler in the sitescripts repository.)
> > > > 
> > > > Assigne this template to some page `template=sitekey_frame`
> > > > Ex.: change template of `pages/testcases/generic/01.md` from "testcase"
to
> > > > "sitekey_frame"
> > > 
> > > Ah right I see, well it's designed to be rendered by sitescripts and not
> used
> > as
> > > a CMS page template. I wouldn't expect that to work.
> > 
> > But if you use -> {% extends "templates/frame" %} looks to work fine.
> 
> So this template is designed to be rendered by
> sitescripts.testpages.web.sitekey_frame . If I omit the ".tmpl" extension that
> URL handler can no longer render the template - it breaks. This template is
not
> designed to be used by CMS pages, it does not matter if it does not work for
> them.

Ahh got it! LGTM