Add support for Thunderbird addons gallery #77

Jackymancs4 · 2019-01-08T09:22:11Z

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

Rob--W

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:

crxviewer/src/crxviewer.js

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.

Rob--W · 2019-01-08T12:22:05Z

src/cws_pattern.js

    '*://addons-dev.allizom.org/*addon/*',
    '*://addons-dev.allizom.org/*review/*',
+    '*://addons.thunderbird.net/*addon/*',
+    '*://addons.thunderbird.net/*review/*'


Add a trailing comma please.

Rob--W · 2019-01-08T12:22:14Z

src/cws_pattern.js

    '*://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/*'


Add trailing comma.

Rob--W · 2019-01-08T12:30:41Z

src/cws_pattern.js

    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');


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');

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.

Rob--W · 2019-01-08T12:42:43Z

src/cws_pattern.js

    }

-    var url = 'https://' + amoDomain + '/firefox/downloads/latest/';
+    var url = 'https://' + amoDomain + '/'+(amoDomain=="addons.thunderbird.net" ? "thunderbird" : "firefox")+'/downloads/latest/';


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).

Rob--W · 2019-01-08T12:46:47Z

src/background.js

            triggerDownload(url, filename);
        }
        // else when the event page goes away, the URL will be revoked.
-    }); 


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).

Sorry about that, my editor did quite a mess. I cleaned up a bit, but three occurrences slipped through. Now It's fixed.

Jackymancs4 · 2019-01-09T17:42:09Z

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.

And in this PR, everything is converted to Thunderbird where possible, except for the search function at: /src/crxviewer.js@0c8a572#L1627-L1631

Leaving it at that is OK

Agree. Right now defaulting to addons.mozilla.org is still a sane strategy, and speaking for myself I don't use that feauture.

Rob--W

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)?

Rob--W · 2019-01-09T19:53:33Z

src/cws_pattern.js

    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;


Use get_amo_base_url(get_amo_domain(url)) here.

Rob--W · 2019-01-09T19:54:05Z

src/cws_pattern.js

    }

-    var url = 'https://' + amoDomain + '/firefox/downloads/latest/';
+    var url = get_amo_base_url(amoDomain)+'/downloads/latest/';


Add spaces around +.

Rob--W · 2019-01-09T19:54:18Z

src/cws_pattern.js

    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');


Add spaces around +.

Rob--W · 2019-01-09T19:55:46Z

src/cws_pattern.js

 }

+function get_amo_base_url(amoDomain) {
+    return 'https://' + amoDomain + '/' + (amoDomain=="addons.thunderbird.net" ? "thunderbird" : "firefox")


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

benjaoming · 2021-08-03T20:47:29Z

@Rob--W any possibility to merge and release this?

Rob--W requested changes Jan 8, 2019

View reviewed changes

Jackymancs4 force-pushed the feature/support-thunderbird-gallery branch from 75da315 to 57dbd73 Compare January 9, 2019 17:25

Rob--W requested changes Jan 9, 2019

View reviewed changes

Add support for thunderbird addons gallery

bf06e70

Add trail commas Fixed regex amo_file_version_pattern and refactored amo url crafting Enforce consistency, minor get_amo_base_url refactor

Jackymancs4 force-pushed the feature/support-thunderbird-gallery branch from 57dbd73 to bf06e70 Compare January 11, 2019 15:40

	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) {

Add support for Thunderbird addons gallery #77

Are you sure you want to change the base?

Add support for Thunderbird addons gallery #77

Uh oh!

Conversation

Jackymancs4 commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rob--W left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jackymancs4 Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jackymancs4 commented Jan 9, 2019

Uh oh!

Rob--W left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaoming commented Aug 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Jackymancs4 commented Jan 8, 2019 •

edited

Loading

Jackymancs4 Jan 9, 2019 •

edited

Loading