Skip to content
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

[Extensions] Allow installation of locally downloaded extensions/addons #391

Merged
merged 21 commits into from Jan 4, 2023

Conversation

DevPika
Copy link
Contributor

@DevPika DevPika commented Dec 26, 2022

Related to #22
This PR adds a Developer Setting to Allow installation of locally downloaded extensions.

Flow:

  1. User enables Developer Option to Allow local addon installation (Settings > Developer Options > "Allow Installation of Local Addons")
  2. User downloads .xpi file from the extension's website
  3. User clicks on the corresponding entry in the Download Manager
  4. The addon is installed and enabled

The installation flow for a LocalExtension is quite different from a regular addon. The primary difference is that for local extensions, we do not have an "Addon" class instance available prior to installation when we refresh our addons list, as info related to that is populated from the extension manifest only after installation. So the local addon installation needs to happen similar to how BuiltInExtensions are installed in the existing code.

Installation workflow difference:

For regular addons (flow already exists):

  1. Get list of Addons
  2. User presses the + icon for installation
  3. ...UI Confirmations for download and install...
  4. Install method requires passing an existing instance of Addon
  5. All registered listeners receive onAddonsUpdated through notifyListeners in the same file

For local addons (added through this PR, similar to BuiltInExtension):

  1. User clicks on downloaded addon
  2. Extension installed through WolvicWebExtensionRuntime, not through AddonManager
  3. All registered listeners receiveonAddonsUpdated through notifyListeners, but this time inside LocalExtension class after installation is done

I searched through FirefoxForAndroid codebase and GeckoView API, but could not find a way to construct an Addon object from xpi file before installation. A possible change in workflow can be to register to installPrompt call, but I wasn't sure how to integrate that deeply into GeckoView. So I kept the changes straightforward for now

TODO

  • Dialog / toast message to show success/failure of local addon installation instead of a silent install
  • Tests for the feature?
  • Fixed - Rating Bar and Icon were incorrectly assigned for fresh installation of local addons, now custom/default icon is shown and rating bar gets hidden if no info
  • What happens when "same" extension clicked multiple times? Seems to only get installed once currently.
  • Update Addons List as soon as addon installation complete
  • Fix addon showing up in the Disabled section even though Enabled. This is because of AddonManager code in AC 75.0.22, which provides list of addons, ignores installedstate and forces enabled = false Fixed by updating list with correct state and adding a new section "Experimental"

com igalia wolvic-20221225-191408

com igalia wolvic-20230104-012705

@DevPika
Copy link
Contributor Author

DevPika commented Dec 26, 2022

I am opening a draft PR for feedback and suggestions

Copy link
Member

@svillar svillar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really cool. I'm pretty sure extensions devs will love it. I just have some nits here and there, but overall looks great

@DevPika DevPika marked this pull request as ready for review December 27, 2022 08:21
@DevPika
Copy link
Contributor Author

DevPika commented Dec 27, 2022

Thank you so much for your review! I have tackled all of the comments and some of the TODOs, for the remaining TODOs I am not very sure how to proceed. I am not expecting to make any major changes so I have marked the PR as Ready for Review.

@DevPika DevPika requested a review from svillar December 27, 2022 08:34
Copy link
Contributor Author

@DevPika DevPika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made the requested changes and added some supporting features like having the addons in an "Experimental" section and showing their correct enabled/disabled state. I have marked the previous comments as resolved and requested a re-review

Hide rating view if the addon does not contain rating information, otherwise make it visible
Previously the addon icon did not get updated if icon was unavailable
@DevPika
Copy link
Contributor Author

DevPika commented Dec 28, 2022

I was able to fix the bugs related to incorrect data being shown in the rating bar and icon for new addons. The PR seems pretty complete after these fixes.

Copy link
Member

@svillar svillar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're almost there. Fantastic job!

Extensions installed through this method persist between sessions so they are not temporary. Further locally-downloaded extensions are not built-in, so removing "in the runtime" part from the comment makes more sense, as local addons only get installed once whereas built-in ones get installed on each startup.
@svillar
Copy link
Member

svillar commented Jan 2, 2023

I have finally the chance to test this and it works great to me. I've a last request though (thanks for your patience), it'll be difficult, even for devs, to realize that they have to enable the setting in developer mode in order to be able to install local extensions. The current situation is that, if the switch is disabled, Wolvic will complain about "unsupported filetype", I'd very much prefer to show a dialog informing the user about the switch. Could we add that?

I've also found a bit ankward the "silent" installation process. I've just seen that it's in your radar, so I'm fine landing this without that feature and do it in a followup.

…ailable

This is a fix follow up to previous icon fix. The extra surrounding null check stopped code from setting default icon. After removing the check, the default icon is set correctly.
@DevPika
Copy link
Contributor Author

DevPika commented Jan 2, 2023

Thank you so much for your quick and helpful feedback and patience throughout! I have added two dialogs: one for blocked installation (with an explanation of where the toggle to enable is present), and another for confirming installation of addon. Please let me know if you have any further suggestions and feedback!

@svillar
Copy link
Member

svillar commented Jan 3, 2023

I'm quite happy with the final result. I just have a question before merging it. I've downloaded several xpi from https://addon.mozilla.org and after installing them, none of them displays their actual icon in the addons list. All of them get a white square one, is that expected?

I checked the addon.iconURL field for the local addons and it's always null, is it because that info is retrieved by Mozilla's android components from the web and is not part of the .xpi?

Loaded from locally available icon if mentioned in the manifest meta info after installation
@DevPika
Copy link
Contributor Author

DevPika commented Jan 4, 2023

I checked the addon.iconURL field for the local addons and it's always null, is it because that info is retrieved by Mozilla's android components from the web and is not part of the .xpi?

Yes, this was the issue. I have made some changes now so that the locally available icon from the manifest is loaded if the web icon fetch doesn't work (similar to correcting the enabled state in abab3d4). Thanks for looking into this!

I tried the changes with some of the recommended addons on AMO homepage, for some reason the Tomato Clock addon's icon doesn't load, all the others I tried seemed fine after the changes. Warning for Tomato Clock:

org.mozilla.geckoview.Image$ImageProcessingException: Could not process image: 0x80004005
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:677)

I wasn't able to find the cause though, mentioning it here for future reference in case this crops up for any other extensions.

@svillar svillar merged commit 3e1225b into Igalia:main Jan 4, 2023
@Utopiah Utopiah mentioned this pull request Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants