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.

Sign up for GitHub

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

Jump to bottom

Bug 1866293 - Add support for installing an add-on a from local file. #4568

Merged
merged 1 commit into from Nov 29, 2023
Merged

Bug 1866293 - Add support for installing an add-on a from local file. #4568

merged 1 commit into from Nov 29, 2023

Conversation

Amejia481
Copy link
Contributor

@Amejia481 Amejia481 commented Nov 22, 2023 •
edited by github-actions bot

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1866293

@github-actions github-actions bot added the work in progress Not ready to land yet. Work in progress (WIP). label Nov 22, 2023
@github-actions GitHub Actions
Copy link
Contributor

🚧 Commit message is using the wrong format: WIP - Install local add-ons.

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

@github-actions GitHub Actions
Copy link
Contributor

🚧 Pull request title is using the wrong format: WIP - Install local add-ons.

The pull request title should look like:

  Bug xxxx - Short description of your change

@github-actions GitHub Actions
Copy link
Contributor

🚧 Pull request title is using the wrong format: WIP - Install local add-ons.

The pull request title should look like:

  Bug xxxx - Short description of your change

@Amejia481 Amejia481 changed the title WIP - Install local add-ons. Bug 1866293 - Add support for installing an add-on a from local file. Nov 23, 2023
@Amejia481 Amejia481 marked this pull request as ready for review November 23, 2023 18:16
@github-actions github-actions bot added 🕵️‍♀️ needs review PRs that need to be reviewed and removed work in progress Not ready to land yet. Work in progress (WIP). labels Nov 23, 2023
@Amejia481 Amejia481 force-pushed the WEBEXT-1552 branch 2 times, most recently from da88359 to 49412c1 Compare November 23, 2023 19:28
@Amejia481 Amejia481 mentioned this pull request Nov 23, 2023
Closed
4 tasks
@willdurand
Copy link
Member

So I think to move forward, I'd like to (1) discuss if we can change installAddon() to receive a url argument instead of a (possibly) empty Addon instance, and (2) investigate why it isn't possible to load a ZIP file with this patch.

For (2), we might want to defer that to a different bug if we want to, but that is going to be a valid use case for Nightly and maybe Beta users (well, extension developers mainly).

@Amejia481
Copy link
Contributor Author

Amejia481 commented Nov 27, 2023 •
edited

So I think to move forward, I'd like to (1) discuss if we can change installAddon() to receive a url argument instead of a (possibly) empty Addon instance, and (2) investigate why it isn't possible to load a ZIP file with this patch.

That's sound good, I'm taking a look to the API to see how we can make it work!

For (2), we might want to defer that to a different bug if we want to, but that is going to be a valid use case for Nightly and maybe Beta users (well, extension developers mainly).

Perfect, I'll add a validations.

@github-actions GitHub Actions
Copy link
Contributor

🚧 Commit message is using the wrong format: Adding feedback

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

@Amejia481
Copy link
Contributor Author

@willdurand I pushed another commit with you suggestion. Before landing I'll squash changes.

willdurand
willdurand requested changes Nov 28, 2023
View reviewed changes
Copy link
Member

@willdurand willdurand 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 looking great but I have questions on the overall design. I'd like to avoid duplicating methods since that will lead to confusion in 6 months..

...ts/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngine.kt Outdated Show resolved Hide resolved
...t/engine/src/main/java/mozilla/components/concept/engine/webextension/WebExtensionRuntime.kt Show resolved Hide resolved
...ts/components/feature/addons/src/main/java/mozilla/components/feature/addons/AddonManager.kt Show resolved Hide resolved
...ponents/feature/addons/src/main/java/mozilla/components/feature/addons/ui/AddonFilePicker.kt Outdated Show resolved Hide resolved
...ponents/feature/addons/src/main/java/mozilla/components/feature/addons/ui/AddonFilePicker.kt Outdated Show resolved Hide resolved
...ponents/feature/addons/src/main/java/mozilla/components/feature/addons/ui/AddonFilePicker.kt Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

🚧 Commit message is using the wrong format: Adding feedback

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

@Amejia481
Copy link
Contributor Author

@willdurand address some of the comments and added some clarification comments.

@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 28, 2023

This pull request has conflicts when rebasing. Could you fix it @Amejia481? 🙏

@Amejia481
Copy link
Contributor Author

@willdurand I updated the PR with the suggested changes!

@Amejia481
Copy link
Contributor Author

Also I squashed the commits

willdurand
willdurand approved these changes Nov 29, 2023
View reviewed changes
Copy link
Member

@willdurand willdurand left a comment

Choose a reason for hiding this comment

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

Thanks, r+wc

...t/engine/src/main/java/mozilla/components/concept/engine/webextension/WebExtensionRuntime.kt Outdated Show resolved Hide resolved
...ponents/feature/addons/src/main/java/mozilla/components/feature/addons/ui/AddonFilePicker.kt Outdated Show resolved Hide resolved
...nts/feature/addons/src/test/java/mozilla/components/feature/addons/ui/AddonFilePickerTest.kt Outdated Show resolved Hide resolved
fenix/app/src/test/java/org/mozilla/fenix/settings/SettingsFragmentTest.kt Outdated Show resolved Hide resolved
@Amejia481 Amejia481 added 🛬 needs landing PRs that are ready to land approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Nov 29, 2023
@mergify mergify bot merged commit 36a3e74 into mozilla-mobile:main Nov 29, 2023
145 of 148 checks passed
@jackyzy823
Copy link
Contributor

Hello @Amejia481 , should we clean up the cache folder "/data/data/org.mozilla.firefox/cache/XPIS" after add-on installation success/failure ? (A similar issue like this https://bugzilla.mozilla.org/show_bug.cgi?id=1860472)

@willdurand
Copy link
Member

Hello @Amejia481 , should we clean up the cache folder "/data/data/org.mozilla.firefox/cache/XPIS" after add-on installation success/failure ? (A similar issue like this bugzilla.mozilla.org/show_bug.cgi?id=1860472)

If the cache is never cleared, I think it would make sense yeah, please file a bug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willdurand willdurand willdurand approved these changes

Assignees

@willdurand willdurand

Labels
approved PR that has been approved 🛬 needs landing PRs that are ready to land
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Amejia481 @willdurand @jackyzy823