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

Bookmark FFI for iOS and Kotlin #743

Merged
merged 53 commits into from
Mar 19, 2019
Merged

Bookmark FFI for iOS and Kotlin #743

merged 53 commits into from
Mar 19, 2019

Conversation

thomcc
Copy link
Contributor

@thomcc thomcc commented Mar 11, 2019

This differs from the doc a bit (The operations in the doc are fairly low level, and also are not how our FFI works, so I took the doc as a "these are the things we need to support" list), but supports all the same operations and is a bit more general (e.g. move is part of update, getting the list of child GUIDs is part of the API to get a bookmark entry in general), and a lot higher level.

I'm completely willing to add the operations from the doc explicitly (implemented on top of what I have) as a further feature. This way was clearer to implement given what we had already, and mor.

The documentation in the Kotlin and Swift files is fairly thorough.

This PR is mostly complete, but still needs:

  • Integration with iOS megazord
  • Finish documenting how to integrate protobufs on iOS so the next person who has to do it doesn't have to figure out so much from scratch again like I did for this PR.
  • Document which methods can throw which errors.
  • More test coverage.
  • Changelog entry.

I should probably clean up the commit history a bit too, so that we don't have to squash-merge it. Nah lets just squash merge this rather than waste time cleaning up a history that nobody really cares about.

Fixes #727

r? @justindarc for the swift
r? @grigoryk for the kotlin
r? @linacambridge / @mhammond for the rust

Followups:

  • BookmarkTreeNode's GUID should probably not be optional.
  • Consider implementing fetch_bookmark using fetch_tree when get_direct_children is true
  • fetch_tree should return position / parent GUID info so we don't have to fix it up in fetch_public_tree (which requires a query).
  • run_maintenance should fix up bookmark tree corruption.
  • Change update_bookmark_from_message to pass a raw bookmark into update_bookmark_in_tx (which should be changed / another function added to accept an already-queried raw bookmark)
  • Too many bookmark node types!

@thomcc
Copy link
Contributor Author

thomcc commented Mar 11, 2019

r?@fzzzy since he's been reading a bunch about the FFI and seeing how it works in practice might be helpful.

@rfk rfk mentioned this pull request Mar 12, 2019
1 task
Copy link
Contributor

@garvankeeley garvankeeley left a comment

Choose a reason for hiding this comment

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

Not a reviewer, but lgtm! and dropped in some drive-by food-for-thought comments.

components/places/ios/Places/LoginRecord.swift Outdated Show resolved Hide resolved
components/places/ios/Places/LoginRecord.swift Outdated Show resolved Hide resolved
components/places/ios/Places/LoginRecord.swift Outdated Show resolved Hide resolved
components/places/ios/Places/LoginRecord.swift Outdated Show resolved Hide resolved
components/places/ios/Places/LoginRecord.swift Outdated Show resolved Hide resolved
components/places/ios/Places/LoginRecord.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@justindarc justindarc left a comment

Choose a reason for hiding this comment

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

LGTM! Some nits about naming, etc.

components/places/ios/Places/Errors/PlacesError.swift Outdated Show resolved Hide resolved
components/places/ios/Places/Places.swift Outdated Show resolved Hide resolved
components/places/ios/Places/Places.swift Show resolved Hide resolved
components/places/ios/Places/Places.swift Outdated Show resolved Hide resolved
components/places/ios/Places/Places.swift Show resolved Hide resolved
components/places/ios/Places/Places.swift Outdated Show resolved Hide resolved
components/places/ios/Places/Places.swift Outdated Show resolved Hide resolved
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.

This is excellent, @thomcc, thanks! I had mostly nits, and a suggestion for avoiding multiple reads to fetch direct children, but we can tackle those in follow-ups if you'd like. I wouldn't mind reading through this again before it lands, but I think it's in great shape. 🚀

components/places/ios/Places/Errors/PlacesError.swift Outdated Show resolved Hide resolved
components/places/src/storage/bookmarks.rs Show resolved Hide resolved
components/places/src/storage/bookmarks.rs Outdated Show resolved Hide resolved
components/places/src/storage/bookmarks.rs Show resolved Hide resolved
components/places/src/storage/bookmarks.rs Outdated Show resolved Hide resolved
components/places/src/storage/bookmarks.rs Outdated Show resolved Hide resolved
@thomcc thomcc removed the request for review from grigoryk March 13, 2019 20:26
Copy link
Contributor

@grigoryk grigoryk left a comment

Choose a reason for hiding this comment

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

I've only looked at the Kotlin APIs, from the point of view of a consuming application.

val lastModified: Long

/**
* The guid of this record's parent. It should only be null for
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: (unsure yet how this might fit w/ the rest of the api) You could enforce the "can't be null for non-root nodes" limitation at the interface level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather keep things simple than optimize for enforcing as much as possible, but if you have a concrete idea then LMK and I'll think about it.

@thomcc thomcc mentioned this pull request Mar 14, 2019
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 Thom and obviously took a heap of work - well done.

Mainly nits, and I'm not looking forward to resolving the conflicts with the bookmark-merge branch, but +1 from me!

components/places/ios/Places/Bookmark.swift Show resolved Hide resolved
components/places/ios/Places/Places.swift Outdated Show resolved Hide resolved
components/places/src/storage/bookmarks.rs Show resolved Hide resolved
components/places/src/storage/bookmarks.rs Outdated Show resolved Hide resolved
components/places/src/storage/bookmarks.rs Outdated Show resolved Hide resolved
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.

Let's 🚢 this! We can tackle anything that's left in follow-ups. Love the thorough docs, too!

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.

Implement an FFI for bookmarks
7 participants