Fix up diverging structure in bookmark trees

src/error.rs

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::{error, fmt, result};
+use std::{error, fmt, result, str::Utf8Error, string::FromUtf16Error};
 
 use guid::Guid;
 use tree::Kind;
@@ -28,35 +28,36 @@ impl Error {
     }
 }
 
-impl error::Error for Error {}
+impl error::Error for Error {
+    fn source(&self) -> Option<&(dyn error::Error + 'static)> {
+        match self.kind() {
+            ErrorKind::MalformedString(err) => Some(err.as_ref()),
+            _ => None
+        }
+    }
+}
 
 impl From<ErrorKind> for Error {
     fn from(kind: ErrorKind) -> Error {
         Error(kind)
     }
 }
 
-impl fmt::Display for Error {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "{}", self)
+impl From<FromUtf16Error> for Error {
+    fn from(error: FromUtf16Error) -> Error {
+        Error(ErrorKind::MalformedString(error.into()))
     }
 }
 
-#[derive(Debug)]
-pub enum ErrorKind {
-    MismatchedItemKind(Kind, Kind),
-    DuplicateItem(Guid),
-    InvalidParent(Guid, Guid),
-    MissingParent(Guid, Guid),
-    Storage(&'static str, u32),
-    MergeConflict,
-    UnmergedLocalItems,
-    UnmergedRemoteItems,
+impl From<Utf8Error> for Error {
+    fn from(error: Utf8Error) -> Error {
+        Error(ErrorKind::MalformedString(error.into()))
+    }
 }
 
-impl fmt::Display for ErrorKind {
+impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match self {
+        match self.kind() {
             ErrorKind::MismatchedItemKind(local_kind, remote_kind) => {
                 write!(f, "Can't merge local kind {} and remote kind {}",
                        local_kind, remote_kind)
@@ -72,9 +73,6 @@ impl fmt::Display for ErrorKind {
                 write!(f, "Can't insert item {} into nonexistent parent {}",
                        child_guid, parent_guid)
             },
-            ErrorKind::Storage(message, result) => {
-                write!(f, "Storage error: {} ({})", message, result)
-            },
             ErrorKind::MergeConflict => {
                 write!(f, "Local tree changed during merge")
             },
@@ -83,7 +81,28 @@ impl fmt::Display for ErrorKind {
             },
             ErrorKind::UnmergedRemoteItems => {
                 write!(f, "Merged tree doesn't mention all items from remote tree")
-            }
+            },
+            ErrorKind::InvalidGuid(invalid_guid) => {
+                write!(f, "Merged tree contains invalid GUID {}", invalid_guid)
+            },
+            ErrorKind::InvalidByte(b) => {
+                write!(f, "Invalid byte {} in UTF-16 encoding", b)
+            },
+            ErrorKind::MalformedString(err) => err.fmt(f),
         }
     }
 }
+
+#[derive(Debug)]
+pub enum ErrorKind {
+    MismatchedItemKind(Kind, Kind),
+    DuplicateItem(Guid),
+    InvalidParent(Guid, Guid),
+    MissingParent(Guid, Guid),
+    MergeConflict,
+    UnmergedLocalItems,
+    UnmergedRemoteItems,
+    InvalidGuid(Guid),
+    InvalidByte(u16),
+    MalformedString(Box<dyn error::Error + Send + Sync + 'static>),
+}

src/guid.rs

@@ -14,6 +14,8 @@
 
 use std::{fmt, str, ops};
 
+use error::{Result, ErrorKind};
+
 #[derive(Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
 pub struct Guid(Repr);
 
@@ -30,12 +32,15 @@ enum Repr {
 /// The Places root GUID, used to root all items in a bookmark tree.
 pub const ROOT_GUID: Guid = Guid(Repr::Fast(*b"root________"));
 
+/// The "Other Bookmarks" GUID, used to hold items without a parent.
+pub const UNFILED_GUID: Guid = Guid(Repr::Fast(*b"unfiled_____"));
+
 /// The syncable Places roots. All synced items should descend from these
 /// roots.
 pub const USER_CONTENT_ROOTS: [Guid; 4] = [
     Guid(Repr::Fast(*b"toolbar_____")),
     Guid(Repr::Fast(*b"menu________")),
-    Guid(Repr::Fast(*b"unfiled_____")),
+    UNFILED_GUID,
     Guid(Repr::Fast(*b"mobile______")),
 ];
 
@@ -51,49 +56,37 @@ const VALID_GUID_BYTES: [u8; 255] =
      0, 0, 0, 0, 0, 0, 0];
 
 impl Guid {
-    pub fn new(s: &str) -> Guid {
-        let repr = if Guid::is_valid(s.as_bytes()) {
-            assert!(s.is_char_boundary(12));
-            let mut bytes = [0u8; 12];
-            bytes.copy_from_slice(s.as_bytes());
-            Repr::Fast(bytes)
-        } else {
-            Repr::Slow(s.into())
-        };
-        Guid(repr)
-    }
-
-    pub fn from_utf8(b: &[u8]) -> Option<Guid> {
+    pub fn from_utf8(b: &[u8]) -> Result<Guid> {
         let repr = if Guid::is_valid(b) {
             let mut bytes = [0u8; 12];
             bytes.copy_from_slice(b);
             Repr::Fast(bytes)
         } else {
             match str::from_utf8(b) {
                 Ok(s) => Repr::Slow(s.into()),
-                Err(_) => return None
+                Err(err) => return Err(err.into()),
             }
         };
-        Some(Guid(repr))
+        Ok(Guid(repr))
     }
 
-    pub fn from_utf16(b: &[u16]) -> Option<Guid> {
+    pub fn from_utf16(b: &[u16]) -> Result<Guid> {
         let repr = if Guid::is_valid(b) {
             let mut bytes = [0u8; 12];
             for (index, byte) in b.iter().enumerate() {
                 match byte.into_byte() {
                     Some(byte) => bytes[index] = byte,
-                    None => return None
+                    None => return Err(ErrorKind::InvalidByte(*byte).into()),
                 };
             }
             Repr::Fast(bytes)
         } else {
             match String::from_utf16(b) {
                 Ok(s) => Repr::Slow(s),
-                Err(_) => return None
+                Err(err) => return Err(err.into()),
             }
         };
-        Some(Guid(repr))
+        Ok(Guid(repr))
     }
 
     #[inline]
@@ -117,6 +110,13 @@ impl Guid {
         }
     }
 
+    pub fn valid(&self) -> bool {
+        match self.0 {
+            Repr::Fast(_) => true,
+            Repr::Slow(_) => false,
+        }
+    }
+
     /// Equivalent to `PlacesUtils.isValidGuid`.
     #[inline]
     fn is_valid<T: Copy + IntoByte>(bytes: &[T]) -> bool {
@@ -129,7 +129,15 @@ impl Guid {
 impl<'a> From<&'a str> for Guid {
     #[inline]
     fn from(s: &'a str) -> Guid {
-        Guid::new(s)
+        let repr = if Guid::is_valid(s.as_bytes()) {
+            assert!(s.is_char_boundary(12));
+            let mut bytes = [0u8; 12];
+            bytes.copy_from_slice(s.as_bytes());
+            Repr::Fast(bytes)
+        } else {
+            Repr::Slow(s.into())
+        };
+        Guid(repr)
     }
 }
 

src/lib.rs

@@ -27,5 +27,5 @@ mod tests;
 pub use error::{Error, ErrorKind, Result};
 pub use guid::{Guid, ROOT_GUID, USER_CONTENT_ROOTS};
 pub use merge::{Deletion, Merger, StructureCounts};
-pub use store::{Store, merge};
-pub use tree::{Content, Item, Kind, MergeState, MergedNode, Tree};
+pub use store::Store;
+pub use tree::{Child, Content, Item, Kind, MergeState, MergedNode, ParentGuidFrom, Tree};