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

Fixes #4370 - History metadata enums and struct access #4371

Merged
merged 9 commits into from Jul 30, 2021
Merged

Fixes #4370 - History metadata enums and struct access #4371

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3186c62
Fixes #4370 - History metadata enums and struct access
nbhasin2 Jul 29, 2021
2595b0c
Cleanup
nbhasin2 Jul 29, 2021
688c8a0
Refactored history observation enum to struct
nbhasin2 Jul 29, 2021
acf1bc3
Made both document type and history metadata codable so that we can e…
nbhasin2 Jul 29, 2021
7094604
Cleanup lint issues
nbhasin2 Jul 29, 2021
59f9762
lint cleanup
nbhasin2 Jul 29, 2021
9c1dd9d
Fixed a broken test case
nbhasin2 Jul 29, 2021
8737c0c
Fixed test case
nbhasin2 Jul 29, 2021
5df754d
Merge branch 'main' into nb/4370-history_metadata_access
nbhasin2 Jul 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions components/places/ios/Places/HistoryMetadata.swift
View file
Open in desktop
Expand Up @@ -10,6 +10,15 @@ import Foundation
public enum DocumentType: Int32 {
case regular = 0
case media = 1
case unknown = -1

public init(rawValue: Int32) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So what's your use case for this? Can't you just do DocumentType.media when creating instances? I suppose this may be helpful when deserializing, but I don't think you'd need to persist a DocumentType yourself manually.

Also, these numbers aren't entirely random - they need to align w/ what other platforms are using. And currently we don't have a -1 present elsewhere. So unless you absolutely need this change, I'd remove at least the unknown case.

switch rawValue {
case 0: self = .regular
case 1: self = .media
default: self = .unknown
}
}
}

/**
Expand All @@ -19,6 +28,12 @@ public struct HistoryMetadataKey {
public let url: String
public let searchTerm: String?
public let referrerUrl: String?

public init(url: String, searchTerm: String?, referrerUrl: String?) {
self.url = url
self.searchTerm = searchTerm
self.referrerUrl = referrerUrl
}
}

/**
Expand All @@ -28,6 +43,16 @@ public enum HistoryMetadataObservation {
case titleObservation(String)
case viewTimeObservation(Int32)
case documentTypeObservation(DocumentType)
case unknown
Copy link
Contributor

Choose a reason for hiding this comment

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

What is an unknown observation, and why do you need it? Does seem necessary to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I really don't need unknown but removing that causes swift errors for our initializers so I added additional case for default as unknonw. Happy to change / rename it as needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about function overloading? Separate init functions for each data type? Then you can remove unknown, i think.


public init(value: Any) {
switch value {
case let title as String: self = .titleObservation(title)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we have string observations that aren't about title, or Int32 that aren't time? Also, what's the use case here for the init, can't you just do HistoryMetadataObservation.titleObservation("hello")?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The major issue is that we need some sort of initializer for using it on Firefox iOS side otherwise structs and enums are inaccessible. This is just something how swift hasn't gotten to or a limitation I would say as all this is extra work 😓

Copy link
Contributor

@grigoryk grigoryk Jul 29, 2021 •
edited

Choose a reason for hiding this comment

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

Oh wow, so that's actually a thing! https://docs.swift.org/swift-book/LanguageGuide/AccessControl.html#//apple_ref/doc/uid/TP40014097-CH41-ID3 (memberwise init section). That's pretty annoying :)

case let time as Int32: self = .viewTimeObservation(time)
case let document as DocumentType: self = .documentTypeObservation(document)
default: self = .unknown
}
}
}

/**
Expand Down
4 changes: 3 additions & 1 deletion components/places/ios/Places/Places.swift
View file
Open in desktop
Expand Up @@ -887,7 +887,7 @@ public class PlacesWriteConnection: PlacesReadConnection {

// MARK: History Metadata

open func noteHistoryMetadataObservation(
open func noteHistoryMetadataObservation (
key: HistoryMetadataKey,
observation: HistoryMetadataObservation
) throws {
Expand All @@ -910,6 +910,8 @@ public class PlacesWriteConnection: PlacesReadConnection {
msg.documentType = documentType.rawValue
case let .titleObservation(title):
msg.title = title
case .unknown:
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a bug that may be easy to miss - e.g. somehow you end up init-ing HistoryMetadataObservation wrong, then pass it along here, and never notice that nothing actually happens.

Partially why I don't like being able to represent invalid states! E.g. if unknown isn't a thing, you can't pass invalid data into this method at all.

break
}

let data = try! msg.serializedData()
Expand Down