-
Notifications
You must be signed in to change notification settings - Fork 231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Thunderbird addons gallery #77
base: master
Are you sure you want to change the base?
Add support for Thunderbird addons gallery #77
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for this PR! I have left some review comments below.
And in this PR, everything is converted to Thunderbird where possible, except for the search function at:
Lines 1627 to 1631 in 0c8a572
var amodomain = get_amo_domain(urlInput.value || getParam('crx')); | |
var amodescription = amoOptions.querySelector('.amodescription'); | |
var slugorid = amoOptions.querySelector('input[name="amoslugorid"]').value; | |
amodescription.textContent = 'Searching for add-ons with slug or ID: ' + slugorid; | |
getXpis(amodomain, slugorid, function(description, results) { |
By default get_amo_domain
returns addons.mozilla.org
, so if there is no URL given at all, the AMO search form will search on AMO. Leaving it at that is OK; if you want to have different behavior feel free to propose reasonable patches.
src/cws_pattern.js
Outdated
var amo_match_patterns = [ | ||
'*://addons.mozilla.org/*addon/*', | ||
'*://addons.mozilla.org/*review/*', | ||
'*://addons.allizom.org/*addon/*', | ||
'*://addons.allizom.org/*review/*', | ||
'*://addons-dev.allizom.org/*addon/*', | ||
'*://addons-dev.allizom.org/*review/*', | ||
'*://addons.thunderbird.net/*addon/*', | ||
'*://addons.thunderbird.net/*review/*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a trailing comma please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/cws_pattern.js
Outdated
]; | ||
var amo_file_version_match_patterns = [ | ||
'*://addons.mozilla.org/*firefox/files/browse/*', | ||
'*://addons.allizom.org/*firefox/files/browse/*', | ||
'*://addons-dev.allizom.org/*firefox/files/browse/*', | ||
'*://addons.thunderbird.net/*thunderbird/files/browse/*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add trailing comma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/cws_pattern.js
Outdated
@@ -96,7 +99,7 @@ function get_crx_url(extensionID_or_url) { | |||
} | |||
match = amo_file_version_pattern.exec(extensionID_or_url); | |||
if (match) { | |||
return 'https://' + match[1] + '/firefox/downloads/file/' + match[2] + (match[3] || '/addon.xpi'); | |||
return 'https://' + match[1] + '/'+(match[1]=="addons.thunderbird.net" ? "thunderbird" : "firefox")+'/downloads/file/' + match[2] + (match[4] || '/addon.xpi'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is becoming somewhat unreadable...
At the very least, this should be:
return 'https://' + match[1] + '/' + match[2] + '/downloads/file/' + match[3] + (match[4] || '/addon.xpi');
... but the resulting code is barely readable nor maintainable.
So instead of that, use (?:
instead of (
before firefox|thunderbird
in the regexp, and use the function that I suggested before:
return get_amo_base_url(match[1]) + '/downloads/file/' + match[2] + (match[3] || '/addon.xpi');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the very least, this should be:
return 'https://' + match[1] + '/' + match[2] + '/downloads/file/' + match[3] + (match[4] || '/addon.xpi');
Indeed, I missed that..
Done.
src/cws_pattern.js
Outdated
@@ -70,7 +73,7 @@ function get_xpi_url(amoDomain, addonSlug) { | |||
platformId = 2; | |||
} | |||
|
|||
var url = 'https://' + amoDomain + '/firefox/downloads/latest/'; | |||
var url = 'https://' + amoDomain + '/'+(amoDomain=="addons.thunderbird.net" ? "thunderbird" : "firefox")+'/downloads/latest/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This amount of repetition is unmaintainable. Please introduce a new function with signature get_amo_base_url(amoDomain)
that returns 'https://' + amoDomain + '/firefox'
for the current case (and thunderbird for TB).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/background.js
Outdated
@@ -293,5 +295,5 @@ function tryTriggerDownloadFallback(url, filename) { | |||
triggerDownload(url, filename); | |||
} | |||
// else when the event page goes away, the URL will be revoked. | |||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please undo unnecessary whitespace changes in the whole pull request. Feel free to submit a new pull request (or re-use this PR, but have a separate commit for these unrelated whitespace changes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that, my editor did quite a mess. I cleaned up a bit, but three occurrences slipped through. Now It's fixed.
75da315
to
57dbd73
Compare
I had to revert and force push to properly clean the whitespace changes. I refactored the exact way you suggested, doing basic testing by hand afterwards. If needed, I can test more extensively tomorrow. Still, It works fine for every use case of mine.
Agree. Right now defaulting to addons.mozilla.org is still a sane strategy, and speaking for myself I don't use that feauture. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the follow-up. Could you make the following changes and squash the commits + choose a good commit title (your PR title is good)?
src/cws_pattern.js
Outdated
@@ -162,7 +165,7 @@ function get_webstore_url(url) { | |||
} | |||
var amo = get_amo_slug(url); | |||
if (amo) { | |||
return 'https://' + get_amo_domain(url) + '/firefox/addon/' + amo; | |||
return 'https://' + get_amo_domain(url) + '/'+(get_amo_domain(url)=="addons.thunderbird.net" ? "thunderbird" : "firefox")+'/addon/' + amo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use get_amo_base_url(get_amo_domain(url))
here.
src/cws_pattern.js
Outdated
@@ -70,7 +73,7 @@ function get_xpi_url(amoDomain, addonSlug) { | |||
platformId = 2; | |||
} | |||
|
|||
var url = 'https://' + amoDomain + '/firefox/downloads/latest/'; | |||
var url = get_amo_base_url(amoDomain)+'/downloads/latest/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add spaces around +
.
src/cws_pattern.js
Outdated
@@ -96,7 +99,7 @@ function get_crx_url(extensionID_or_url) { | |||
} | |||
match = amo_file_version_pattern.exec(extensionID_or_url); | |||
if (match) { | |||
return 'https://' + match[1] + '/firefox/downloads/file/' + match[2] + (match[3] || '/addon.xpi'); | |||
return get_amo_base_url(match[1])+'/downloads/file/' + match[2] + (match[3] || '/addon.xpi'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add spaces around +
.
src/cws_pattern.js
Outdated
@@ -182,6 +185,10 @@ function get_zip_name(url, /*optional*/filename) { | |||
return filename.replace(/\.(crx|jar|nex|xpi|zip)$/i, '') + '.zip'; | |||
} | |||
|
|||
function get_amo_base_url(amoDomain) { | |||
return 'https://' + amoDomain + '/' + (amoDomain=="addons.thunderbird.net" ? "thunderbird" : "firefox") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be consistent in use of '
and "
, and whitespace around operators. Change this to:
if (amoDomain === 'addons.thunderbird.net') {
return 'https://addons.thunderbird.net/thunderbird';
}
return 'https://' + amoDomain + '/firefox';
Add trail commas Fixed regex amo_file_version_pattern and refactored amo url crafting Enforce consistency, minor get_amo_base_url refactor
57dbd73
to
bf06e70
Compare
@Rob--W any possibility to merge and release this? |
Hello @Rob--W
Following your work on ecbe019 for https://github.com/Rob--W/crxviewer/issues/72, this PR add pageAction support for the Thunderbird addons gallery.
Using the contextual menu already worked, but this seemed a +1 on usability to me.
Since https://github.com/thundernest/addons-server is a fork of the one used by Mozilla, I upgraded the amo regexes instead of adding new ones.
Also, it is present quite a bit of name hardcoding. I hope it's fine, considering I tryied to match what was already there.
Tested in Firefox 64.0 and Chrome 71.0.3578.98