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

[DO NOT MERGE] Expose webext-storage bridged engine logic #6180

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lougeniaC64
Copy link
Contributor

@lougeniaC64 lougeniaC64 commented Mar 27, 2024

Exposing the webext-storage bridged engine work for integration of the uniffi-ed components in desktop. This PR will not be merged until the desktop patch(es) to refactor the webext-storage logic and vendor in A-S are reviewed and approved.

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 22.64%. Comparing base (46acbeb) to head (5244fb8).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6180   +/-   ##
=======================================
  Coverage   22.64%   22.64%           
=======================================
  Files         330      330           
  Lines       29811    29811           
=======================================
  Hits         6750     6750           
  Misses      23061    23061           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lougeniaC64 lougeniaC64 force-pushed the add-webext-syncing branch 6 times, most recently from bbade2c to e575606 Compare April 1, 2024 22:01
@lougeniaC64 lougeniaC64 force-pushed the add-webext-syncing branch 2 times, most recently from 3ef93c5 to ce9babc Compare May 24, 2024 21:29
Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

This looks great - just a few nits. Obviously we still need to be careful about when exactly we land this, but it looks ready to me, nice work!

components/webext-storage/src/store.rs Outdated Show resolved Hide resolved
components/webext-storage/src/sync/bridge.rs Outdated Show resolved Hide resolved
components/webext-storage/src/sync/mod.rs Outdated Show resolved Hide resolved
components/webext-storage/src/lib.rs Show resolved Hide resolved
components/webext-storage/src/webext-storage.udl Outdated Show resolved Hide resolved
@lougeniaC64 lougeniaC64 force-pushed the add-webext-syncing branch 2 times, most recently from 19d71e5 to 1d49962 Compare May 30, 2024 00:25
@lougeniaC64 lougeniaC64 changed the title [WIP] Added get_bytes_in_use function to webext-storage UDL Expose webext-storage bridged engine logic May 30, 2024
@lougeniaC64
Copy link
Contributor Author

@linabutler @skhamis I'm adding you as reviewers to give you a chance to review these changes and offer any feedback.

@lougeniaC64 lougeniaC64 changed the title Expose webext-storage bridged engine logic [DO NOT MERGE] Expose webext-storage bridged engine logic May 30, 2024
Copy link
Member

@linabutler linabutler left a comment

Choose a reason for hiding this comment

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

Thanks so much, @lougeniaC64! This looks super reasonable to me—I see it matches TabsBridgedEngine concept-for-concept, which is great!

I don't think there's any hurry to consolidate them, especially since it would be nice to eventually bridge the other way—that is, use our Rust Sync code in Desktop, and have Desktop-specific engines implemented in JS—so I'm completely happy to approve this once it's out of draft!

@mhammond mhammond force-pushed the add-webext-syncing branch 2 times, most recently from e3e82b8 to bc56444 Compare June 12, 2024 17:30
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

4 participants