Noissue - Always show "Install" button for Microsoft Edge

Noissue - Always show "Install" button for Microsoft Edge

[Sebastian Noack, 2017-09-11 17:14:40]

I plan to land this change once we submit Adblock Plus 0.9.10 to the Windows
Store.

[juliandoucette, 2017-09-11 22:27:15]

On 2017/09/11 17:14:40, Sebastian Noack wrote:
> I plan to land this change once we submit Adblock Plus 0.9.10 to the Windows
> Store.

Ah man :( ... I don't have the Creators update.

[juliandoucette, 2017-09-11 22:28:11]

On 2017/09/11 22:27:15, juliandoucette wrote:
> Ah man :( ... I don't have the Creators update.

At least not that I know of :/

[Oleksandr, 2017-09-12 00:25:57]

On 2017/09/11 22:27:15, juliandoucette wrote:
> On 2017/09/11 17:14:40, Sebastian Noack wrote:
> > I plan to land this change once we submit Adblock Plus 0.9.10 to the Windows
> > Store.
> 
> Ah man :( ... I don't have the Creators update.

FWIW, here is my UserAgent string:

"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like
Gecko) Chrome/52.0.2743.116 Safari/537.36 Edge/15.15063"

LGTM

https://codereview.adblockplus.org/29541876/diff/29541877/static/js/index.js
File static/js/index.js (right):

https://codereview.adblockplus.org/29541876/diff/29541877/static/js/index.js#...
static/js/index.js:42: if (match && match[1] >= 15063 &&
content.classList.contains("edge"))
It's outside the scope of this change, but I just noticed that this also
captures Microsoft Edge on Xbox, which we do not support. Based on this the
User-Agent is very similar: https://ctrl.blog/entry/xbox-edge-user-agent. Same
goes for Windows 10 Mobile, which we don't support either. I don't think it's a
big deal, just FYI.

[Sebastian Noack, 2017-09-12 00:58:20]

https://codereview.adblockplus.org/29541876/diff/29541877/static/js/index.js
File static/js/index.js (right):

https://codereview.adblockplus.org/29541876/diff/29541877/static/js/index.js#...
static/js/index.js:42: if (match && match[1] >= 15063 &&
content.classList.contains("edge"))
On 2017/09/12 00:25:57, Oleksandr wrote:
> It's outside the scope of this change, but I just noticed that this also
> captures Microsoft Edge on Xbox, which we do not support. Based on this the
> User-Agent is very similar: https://ctrl.blog/entry/xbox-edge-user-agent. Same
> goes for Windows 10 Mobile, which we don't support either. I don't think it's
a
> big deal, just FYI.

What would users see on XBox and Windows Phone, when they click the
ms-windows-store:// link? If they see a message that the app is not supported on
their advice, this might be a better user experience than hiding the link and
telling them that they need the Creators Update (which they don't).

[juliandoucette, 2017-09-12 11:47:17]

LGTM

Please don't close this issue until issue(s) are created for the edge cases
discussed [no creators update, xbox, windows phone].

https://codereview.adblockplus.org/29541876/diff/29541877/static/js/index.js
File static/js/index.js (right):

https://codereview.adblockplus.org/29541876/diff/29541877/static/js/index.js#...
static/js/index.js:42: if (match && match[1] >= 15063 &&
content.classList.contains("edge"))
On 2017/09/12 00:25:57, Oleksandr wrote:
> It's outside the scope of this change, but I just noticed that this also
> captures Microsoft Edge on Xbox, which we do not support. Based on this the
> User-Agent is very similar: https://ctrl.blog/entry/xbox-edge-user-agent. Same
> goes for Windows 10 Mobile, which we don't support either. I don't think it's
a
> big deal, just FYI.

I second Sebastian's question. Can you test this?

I think that we should create an issue to identify these edge cases and display
an appropriate message.

[Oleksandr, 2017-09-12 17:20:09]

https://codereview.adblockplus.org/29541876/diff/29541877/static/js/index.js
File static/js/index.js (right):

https://codereview.adblockplus.org/29541876/diff/29541877/static/js/index.js#...
static/js/index.js:42: if (match && match[1] >= 15063 &&
content.classList.contains("edge"))
On 2017/09/12 11:47:17, juliandoucette wrote:
> On 2017/09/12 00:25:57, Oleksandr wrote:
> > It's outside the scope of this change, but I just noticed that this also
> > captures Microsoft Edge on Xbox, which we do not support. Based on this the
> > User-Agent is very similar: https://ctrl.blog/entry/xbox-edge-user-agent.
Same
> > goes for Windows 10 Mobile, which we don't support either. I don't think
it's
> a
> > big deal, just FYI.
> 
> I second Sebastian's question. Can you test this?
> 
> I think that we should create an issue to identify these edge cases and
display
> an appropriate message.

I am actually fairly confident Windows Store would handle this. Unfortunately I
have neither Xbox nor Windows 10 Mobile phone. I can only assume the Store will
say the application isn't supported on this device, because we have explicitly
set our package is available only on PC. But I have no way of testing this.

[Sebastian Noack, 2017-09-12 20:40:28]

As discussed on IRC, we agreed to show the button no matter what. This is
consistent with how "Install" links for other browsers are handled, simplifies
the code quite a bit, and we no longer have to keep updating the website when
advancing the minimum Windows build required.

If the user isn't running Windows or is using an unsupported Windows version or
device family, the Windows Store will show a corresponding message anyway.
(Initially this was not the case, which is why we added that message in the
first place.)

https://codereview.adblockplus.org/29541876/diff/29542748/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29541876/diff/29542748/includes/index.tmpl...
includes/index.tmpl:82: <a class="install-button"
href="https://www.microsoft.com/store/p/adblock-plus/9nblggh4r9nz"
id="install-edge">{{"Agree and Install for Microsoft
Edge"|translate("install-edge")}}</a>
If I understand Ollie correctly, both links have the same effect on Windows 10.
Both, the https://www.microsoft.com/store/ and the ms-windows-store:// link is
opened in the Windows Store app. However, the https:// link will also work on
other operating systems, showing our store page along a message that your device
is not supported.

[juliandoucette, 2017-09-13 12:03:28]

LGTM