Open theseanl opened 2 years ago
I'd welcome support for Whale. For future reference and testing, could you paste relevant links/examples of the pages to visit and the actions to take in order to verify that support works? Basically the test plan that verifies that (1) context menu works (2) extension button works (3) the viewer works as expected.
The current PR is missing the logic for preferring the slug in crxviewer.js; for some relevant changes, see the commit that introduced support for the Edge gallery (dbc8a54ea9a7c6c78ca6f8326827a5e13460a19d from #101).
Here are what I've verified as working:
crxviewer.html
, paste the url of the above example extension/AFAIK direct link to the "whx" (analogous to crx) are not exposed to the user-facing website.
The current PR is missing the logic for preferring the slug in crxviewer.js; for some relevant changes, see the commit that introduced support for the Edge gallery (https://github.com/Rob--W/crxviewer/commit/dbc8a54ea9a7c6c78ca6f8326827a5e13460a19d from https://github.com/Rob--W/crxviewer/pull/101).
I cannot find a reference of what "slug" means in this context. I guess it is some internal lingo used in AMO, could you elaborate?
If you are referring to the line 1577 of crxviewer.js
in the commit, it is replaced with a call to is_webstore_url
in cws_patterns.js
in HEAD, which is patched in this PR.
Currently, the extension has the following host permissions, presumably to bypass CORS header in CWS.
"*://clients2.google.com/service/update2/crx*",
"*://clients2.googleusercontent.com/crx/download/*",
Why not add the following permissions as well?
"*://edge.microsoft.com/extensionwebstorebase/v1/crx*",
"*://msedgeextensions.f.tlu.dl.delivery.mp.microsoft.com/*",
"*://store.whale.naver.com/update/whx*",
"*://store-whale.pstatic.net/update/whx/*"
It will allow downloading of the extension from Edge Add-ons store and Whale store without granting <all_urls>
permission.
From how the url of edge add-ons redirection is formed, this probably differ from user to user, but it seems likely that it will be stable for Whale.
I'll have to take a closer look at this PR, but will probably not get to that this week. I did a cursory look, and found \/
in a string literal where it should not be, but overall it seems fine. I'll check in more detail later.
AFAIK direct link to the "whx" (analogous to crx) are not exposed to the user-facing website.
What do you mean by that? In Chrome, crx cannot be installed by clicking links, but right-click + Save-as would work. A link/URL to something that is very likely an extension package (.whx file extension?) is likely a good candidate for support in crxviewer.
Slug?
The current PR is missing the logic for preferring the slug in crxviewer.js; for some relevant changes, see the commit that introduced support for the Edge gallery (dbc8a54 from https://github.com/Rob--W/crxviewer/pull/101).
I cannot find a reference of what "slug" means in this context. I guess it is some internal lingo used in AMO, could you elaborate?
A "URL slug" is generic terminology that refers to the human-readable part of a URL. E.g. in the Chrome Web Store, it is the "chrome-extension-source-v" part in https://chrome.google.com/webstore/detail/chrome-extension-source-v/jifpbeccnghkjeaalbbjmodiffmgedin
If you are referring to the line 1577 of
crxviewer.js
in the commit, it is replaced with a call tois_webstore_url
incws_patterns.js
in HEAD, which is patched in this PR.
I see, you're right. It has been almost two years since that commit.
Optional permissions
Currently, the extension has the following host permissions, presumably to bypass CORS header in CWS.
"*://clients2.google.com/service/update2/crx*", "*://clients2.googleusercontent.com/crx/download/*",
Why not add the following permissions as well?
"*://edge.microsoft.com/extensionwebstorebase/v1/crx*", "*://msedgeextensions.f.tlu.dl.delivery.mp.microsoft.com/*", "*://store.whale.naver.com/update/whx*", "*://store-whale.pstatic.net/update/whx/*"
It will allow downloading of the extension from Edge Add-ons store and Whale store without granting
<all_urls>
permission. From how the url of edge add-ons redirection is formed, this probably differ from user to user, but it seems likely that it will be stable for Whale.
Adding new extension permissions disables the extension on update, until the user has approved the new permissions. That's why I don't add new permissions.
What do you mean by that? In Chrome, crx cannot be installed by clicking links, but right-click + Save-as would work. A link/URL to something that is very likely an extension package (.whx file extension?) is likely a good candidate for support in crxviewer.
If there was such a link, I would have provided it in the test steps too, because the input form in crxviewer.html
has a description stating that it supports such links. What I meant is that there is no such link (other than those interpolated from update url), so no additional step was provided.
Adding new extension permissions disables the extension on update, until the user has approved the new permissions. That's why I don't add new permissions.
Not granting <all_urls>
does provide additional value. When <all_urls>
permission is granted, the browser icon is shown in every tab, losing its ability of signaling extension's support on the current tab. At least the current HEAD behaves in this way - It could be a different issue though. Since this extension is a developer tool, I'd argue that it is more reasonable to optimize for real values rather than user-friendliness, etc.
Still haven't got time to review this? I noticed that there are several places in README.md and crxviewer.html which lists web stores that the extension supports. Users of Whale extensions will be very minor, so I feel it doesn't need to make into such places, but if it should, please let me know.
Whale browser is based on Chromium, is run by Naver, and has a noticeable market share of %6 in South Korea. Similar to Edge, it supports installing extensions from CWS but also from its own store https://store.whale.naver.com/extensions/. Would you consider adding support for this store in crxviewer? I tried to update references to regexes throughout the sources with appropriate regex for whale web store. Also I've fixed a match pattern that wasn't working for edge add-ons store.