Issue 5447 - Add missing essential meta data to adblockplus.org

Issue 5447 -  Add missing essential meta data to adblockplus.org

[juliandoucette, 2017-08-10 21:32:43]

Can you help out with this Manvel and/or Thomas?

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl
File includes/index.tmpl (left):

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl...
includes/index.tmpl:31: <meta content="https://adblockplus.org/"
property="og:url">
Because og:url is in the default template now.

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl...
includes/index.tmpl:33: <meta content="{{"<fix>Adblock Plus</fix> is the most
popular ad blocker available for <fix>Firefox</fix>, <fix>Chrome</fix>,
<fix>Opera</fix>, <fix>Safari</fix>, <fix>Android</fix> and <fix>Internet
Explorer</fix>. Block all annoying ads all over the web: remove video ads on
<fix>YouTube</fix>, block <fix>Facebook</fix> ads, get rid of pop-ups and much
more. On <fix>Google Chrome</fix>, <fix>Adblock Plus</fix> is one of the fastest
growing extensions ever. <fix>Adblock Plus</fix> is used on over <fix>100</fix>
million devices. Download online for free now!"|translate("s2")}}"
name="description">
I didn't remove this because it's string ID doesn't match the description string
id in the default template. I think that we should remove these and update
crowdin separately.

The same applies on other pages.

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl...
includes/index.tmpl:34: <meta content="{{"Adblock Plus - Surf the web without
annoying ads!"|translate("s1")}}" property="og:title">
Because title and og:title are defined in the default template now and string id
"title" is defined in the page front matter above.

The same applies on other pages.

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl...
includes/index.tmpl:29: <meta content="website" property="og:type">
Because there is no og:type "product"?

[Thomas Greiner, 2017-08-11 13:15:42]

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl
File includes/index.tmpl (left):

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl...
includes/index.tmpl:31: <meta content="https://adblockplus.org/"
property="og:url">
On 2017/08/10 21:32:42, juliandoucette wrote:
> Because og:url is in the default template now.

From what I see in the code, this would mean that we'll end up having different
canonical URLs for each language variant of the page. Is that true?

If so, we may need to be very careful because while it'll likely help us with
the Facebook issue that the wrong language is shown in snippets, it could also
have a negative impact on SEO because (a) search engines would split the traffic
up among the individual languages rather than focusing it on a single page and
(b) they might punish us for having duplicate content regarding pages that are
almost exactly the same in all languages or that might not have been fully
translated from english yet.

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl...
includes/index.tmpl:29: <meta content="website" property="og:type">
On 2017/08/10 21:32:43, juliandoucette wrote:
> Because there is no og:type "product"?

Pretty much but I don't fully remember anymore.

Looking into it again, I noticed the following statement at
http://ogp.me/#types: "Any non-marked up webpage should be treated as og:type
website." So it shouldn't make a difference whether or not we even specify
"og:type", it seems, but it doesn't hurt either.

[juliandoucette, 2017-08-11 20:26:59]

Thanks Thomas! Answers below.

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl
File includes/index.tmpl (left):

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl...
includes/index.tmpl:31: <meta content="https://adblockplus.org/"
property="og:url">
On 2017/08/11 13:15:41, Thomas Greiner wrote:
> From what I see in the code, this would mean that we'll end up having
different
> canonical URLs for each language variant of the page. Is that true?

That would be true if:
- I set an og:url that included the locale
- I removed the og:url all-together

However, I added og:url to default.tmpl using get_canonical_url (a [new, core,
global] function that accepts a page name and generates a full page url with no
locale based on the siteurl in settings.ini)

> If so, we may need to be very careful because while it'll likely help us with
> the Facebook issue that the wrong language is shown in snippets, it could also
> have a negative impact on SEO because (a) search engines would split the
traffic
> up among the individual languages rather than focusing it on a single page and
> (b) they might punish us for having duplicate content regarding pages that are
> almost exactly the same in all languages or that might not have been fully
> translated from english yet.

You have the right idea.

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl...
includes/index.tmpl:29: <meta content="website" property="og:type">
On 2017/08/11 13:15:41, Thomas Greiner wrote:
> On 2017/08/10 21:32:43, juliandoucette wrote:
> > Because there is no og:type "product"?
> 
> Pretty much but I don't fully remember anymore.
> 
> Looking into it again, I noticed the following statement at
> http://ogp.me/#types: "Any non-marked up webpage should be treated as og:type
> website." So it shouldn't make a difference whether or not we even specify
> "og:type", it seems, but it doesn't hurt either.

Acknowledged.

On second thought, I think that this change was unnecessary (in this changeset;
at least).

[Thomas Greiner, 2017-08-14 11:20:53]

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl
File includes/index.tmpl (left):

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl...
includes/index.tmpl:31: <meta content="https://adblockplus.org/"
property="og:url">
On 2017/08/11 20:26:58, juliandoucette wrote:
> On 2017/08/11 13:15:41, Thomas Greiner wrote:
> > From what I see in the code, this would mean that we'll end up having
> different
> > canonical URLs for each language variant of the page. Is that true?
> 
> That would be true if:
> - I set an og:url that included the locale

You're right… seems like that's hardcoded in there so locales are indeed not an
issue.

  # Remove the locale component that `resolve_link` adds at the
  # beginning.
  page_url = page_url[len(locale) + 1:]

What about the product variants of the home page (i.e. /firefox, /chrome,
/safari)? Did we account for those somewhere as well or would their canonical
URLs differ after this change?

Note that they already do differ in the production version due to the way that
`<link rel="canonical">` was implemented in the default template. I'd consider
that to be a regression though.

[juliandoucette, 2017-08-15 17:14:26]

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl
File includes/index.tmpl (left):

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl...
includes/index.tmpl:31: <meta content="https://adblockplus.org/"
property="og:url">
On 2017/08/14 11:20:53, Thomas Greiner wrote:
> What about the product variants of the home page (i.e. /firefox, /chrome,
/safari)? Did we account for those somewhere as well or would their canonical
URLs differ after this change?

I didn't account for these. Good catch. Big oversight on my part.

I have accounted for these in the latest Patchset.

[Thomas Greiner, 2017-08-17 12:27:36]

Looks fine now. Just one suggestion regarding a more generic approach.

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl
File includes/index.tmpl (left):

https://codereview.adblockplus.org/29511696/diff/29511697/includes/index.tmpl...
includes/index.tmpl:31: <meta content="https://adblockplus.org/"
property="og:url">
On 2017/08/15 17:14:26, juliandoucette wrote:
> On 2017/08/14 11:20:53, Thomas Greiner wrote:
> > What about the product variants of the home page (i.e. /firefox, /chrome,
> /safari)? Did we account for those somewhere as well or would their canonical
> URLs differ after this change?
> 
> I didn't account for these. Good catch. Big oversight on my part.
> 
> I have accounted for these in the latest Patchset.
> 

Great, thanks!

https://codereview.adblockplus.org/29511696/diff/29516664/filters/ignore_brow...
File filters/ignore_browsers.py (right):

https://codereview.adblockplus.org/29511696/diff/29516664/filters/ignore_brow...
filters/ignore_browsers.py:11: ]
That is a valid approach if we could ensure that newly added home page variants
are also being added here.

Alternatively, what do you think about some kind of URL path override? That way
we could ensure that every home page variant will point to a single URL while
still using `get_canonical_url()` to avoid having to hardcode the entire URL in
the template.

e.g.
title=Adblock Plus - Surf the web without annoying ads!
noheading=True
localefile=index
canonical=/

This approach could also be used in the future if we want to publish the same
content across multiple websites or duplicate some other content on our website.
(e.g. Acceptable Ads criteria, blog posts, etc.)

What do you think?

[juliandoucette, 2017-08-17 13:50:07]

+CC Vasily, Jon

We ran into an issue with get_canonical_url below because adblockplus.org has
several index pages (one for each browser). To resolve this issue I manually
filtered browser pages into the index page. However, as Thomas points out, it
would be useful if we could override the canonical url (or page?) in the page
front matter.

What do you guys think?

Also @Thomas I think it would be beneficial to push this change as-is and then
update it again afterwards. We may be getting a lot of press/sharing now/soon. 

Do you also think that is wise?

https://codereview.adblockplus.org/29511696/diff/29516664/filters/ignore_brow...
File filters/ignore_browsers.py (right):

https://codereview.adblockplus.org/29511696/diff/29516664/filters/ignore_brow...
filters/ignore_browsers.py:11: ]
On 2017/08/17 12:27:36, Thomas Greiner wrote:
> That is a valid approach if we could ensure that newly added home page
variants
> are also being added here.
> 
> Alternatively, what do you think about some kind of URL path override? That
way
> we could ensure that every home page variant will point to a single URL while
> still using `get_canonical_url()` to avoid having to hardcode the entire URL
in
> the template.
> 
> e.g.
> title=Adblock Plus - Surf the web without annoying ads!
> noheading=True
> localefile=index
> canonical=/
> 
> This approach could also be used in the future if we want to publish the same
> content across multiple websites or duplicate some other content on our
website.
> (e.g. Acceptable Ads criteria, blog posts, etc.)
> 
> What do you think?

I think that is a good idea and it should be implemented in get_canonical_url
instead of a separate filter.

[Thomas Greiner, 2017-08-17 14:00:29]

On 2017/08/17 13:50:07, juliandoucette wrote:
> Also @Thomas I think it would be beneficial to push this change as-is and then
> update it again afterwards. We may be getting a lot of press/sharing now/soon.

That's fine with me.

[Vasily Kuznetsov, 2017-08-17 14:25:13]

On 2017/08/17 13:50:07, juliandoucette wrote:
> +CC Vasily, Jon
> 
> We ran into an issue with get_canonical_url below because
http://adblockplus.org has
> several index pages (one for each browser). To resolve this issue I manually
> filtered browser pages into the index page. However, as Thomas points out, it
> would be useful if we could override the canonical url (or page?) in the page
> front matter.
> 
> What do you guys think?
> 
> Also @Thomas I think it would be beneficial to push this change as-is and then
> update it again afterwards. We may be getting a lot of press/sharing now/soon.

> 
> Do you also think that is wise?
> 
>
https://codereview.adblockplus.org/29511696/diff/29516664/filters/ignore_brow...
> File filters/ignore_browsers.py (right):
> 
>
https://codereview.adblockplus.org/29511696/diff/29516664/filters/ignore_brow...
> filters/ignore_browsers.py:11: ]
> On 2017/08/17 12:27:36, Thomas Greiner wrote:
> > That is a valid approach if we could ensure that newly added home page
> variants
> > are also being added here.
> > 
> > Alternatively, what do you think about some kind of URL path override? That
> way
> > we could ensure that every home page variant will point to a single URL
while
> > still using `get_canonical_url()` to avoid having to hardcode the entire URL
> in
> > the template.
> > 
> > e.g.
> > title=Adblock Plus - Surf the web without annoying ads!
> > noheading=True
> > localefile=index
> > canonical=/
> > 
> > This approach could also be used in the future if we want to publish the
same
> > content across multiple websites or duplicate some other content on our
> website.
> > (e.g. Acceptable Ads criteria, blog posts, etc.)
> > 
> > What do you think?
> 
> I think that is a good idea and it should be implemented in get_canonical_url
> instead of a separate filter.

It seems useful to support page-specific canonical URL overrides but I'm not
sure yet what method I prefer. Seems like encoding such things into
`get_canonical_url` is not quite right, and overriding `page` variable can have
side-effects, because it's not only used for the canonical URL. Let's create a
ticket and then think about the options there.

[juliandoucette, 2017-08-17 17:04:18]

On 2017/08/17 14:25:13, Vasily Kuznetsov wrote:
> It seems useful to support page-specific canonical URL overrides but I'm not
> sure yet what method I prefer. Seems like encoding such things into
> `get_canonical_url` is not quite right, and overriding `page` variable can
have
> side-effects, because it's not only used for the canonical URL. Let's create a
> ticket and then think about the options there.

Done.

- https://issues.adblockplus.org/ticket/5530
- https://issues.adblockplus.org/ticket/5531

[juliandoucette, 2017-09-13 12:27:44]

Note: I updated the locales in to_og_locale and made the function more
permissive