History Storage - Please do not merge #108

tomasstrba · 2021-05-05T19:53:04Z

Task/Issue URL: https://app.asana.com/0/72649045549333/1194460468534281/f
Tech Design URL: https://app.asana.com/0/481882893211075/1200247680782234/f
CC:

Description:
As a first part of the project, I’ve implemented a storage of the history. It’s a bit tricky to verify if the implementation is correct, but I tried to help.

Steps to test this PR:

Open Xcode, go to Logging.swift and set historyLoggingEnabled to true
Put '[History]’ into your console filter

Test case 1

Start the browser
Make sure logs contains "History cleaned and loaded successfully. On main thread: no"

Test case 2

Visit duckduckgo.com
Make sure log contains information that http://duckduckgo.com/ and https://duckduckgo.com/ were visited
Make sure log contains information that the title of https://duckduckgo.com/ was updated and the number of visits is still 1.

Test case 3

Visit your fireproof website
Click on the burn button
Make sure log contains information that cleaning was performed. (Fireproof websites aren’t cleaned and will be reloaded as a part of history again)

Test case 4

Play with the browser and look into the log to check whether websites you visited are stored

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

DuckDuckGo/Fireproofing/Model/FireproofDomains.swift

Unit Tests/Common/Extensions/NSPersistentContainerExtension.swift

DuckDuckGo/History/Model/History.swift

DuckDuckGo/BrowserTab/ViewModel/WebViewStateObserver.swift

DuckDuckGo/History/Model/HistoryCoordinator.swift

samsymons · 2021-05-10T03:59:08Z

So far I really like this, it's inspired me to use Future more often. 😄 Added some notes on time calculation, and a question about how we plan to store history data. Nothing really blocking this right now, I'll likely approve this after the last bits of feedback are discussed. Amazing work @tomasstrba, your PRs are always a pleasure to review. 👏

…ion through the history stack

Co-authored-by: Sam Symons <sam@samsymons.com>

tomasstrba · 2021-05-10T21:08:48Z

Thanks @samsymons! Very good points, did all of them.

samsymons

Nice, this is looking great! 🥳

mallexxx · 2021-05-11T07:48:26Z

DuckDuckGo/History/Services/HistoryStore.swift

+                do {
+                    try self.context.save()
+                } catch {
+                    promise(.failure(HistoryStoreError.savingFailed))


it seems return has been lost here

Well, it’s not necessary because the Future can only succeed or fail once, but it’s a good practice to put it there to avoid a bug in the future. Thanks

mallexxx · 2021-05-11T07:58:27Z

DuckDuckGo/History/Model/HistoryCoordinator.swift

+            entry.addVisit()
+
+            historyDictionary[url] = entry
+            self?.historyDictionary = historyDictionary


just in terms of my beloved micro-optimisations I'd say self?.historyDictionary?[url, default: .init(url: url)].addVisit() ?? { os_log..; return }(), but never mind, just thinking out loudly 😀

:D I’ll keep your suggestion and use it when max-lines warning appears. Also it’s fine not to have optional for .makeHistory(from:). But thanks, I refreshed the default parameter for dictionaries :)

mallexxx · 2021-05-11T08:00:27Z

DuckDuckGo/History/Model/HistoryCoordinator.swift

+
+fileprivate extension Date {
+
+    static var weekAgo: Date! {


Should we move these Date extensions into a separate DateExtensions file as I would expect these to be useful in other places like download history cleanup, etc..

Moved, good point 👍

mallexxx · 2021-05-11T08:09:55Z

DuckDuckGo/BrowserTab/Model/Tab.swift

        webView.goForward()
        loginDetectionService?.handle(navigationEvent: .userAction)
    }

    func goBack() {
+        shouldStoreNextVisit = false


@tomasstrba It seems to me here may be a potential issue of not storing a history visit when going back and then opening another url (or link), seems like a common scenario e.g. for the SERP. Is this intentional or should the backForwardList be checked instead?

I think the use case you described should be fine. When goBack is called, shouldStoreNextVisit is set to false. Then webview displays previous website and updates its url. That triggers adding url into the history - specifically addVisit(of url: URL). The visit is ignored because of the shouldStoreNextVisit flag is false and it's rewritten to true. The next navigation triggers another addVisit(of url: URL) which calls historyCoordinating.addVisit(of: url).

Or am I missing something?

Oh, I got it 🙂 Maybe add this as a comment just for clearance?

tomasstrba · 2021-05-11T11:56:43Z

Thanks @samsymons and @mallexxx for your suggestions! I’ll merge this right before the merge of suggestions to avoid a possible Core Data migration if something changes during the second part of my project

# By Tomas Strba (4) and others # Via GitHub * develop: Update the Chrome user agent (#122) TabViewModel released immediately from TabCollectionViewModel after its tab is closed (#118) User agent updated (#117) History Storage - Please do not merge (#108) use the temporary unprotected sites list and update the frequency to 30 minutes (#114) update email autofill (#113) Version 0.6.16 Force the content blocking rules publisher onto the main thread. (#112) Update to BrowserServicesKit 1.1.1 (#111) Zoom menu items; Menu Items validation (#105) check file headers programatically (#110) Fixed up filenames in file headers (#109) # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/Bookmarks/Services/Bookmark.xcdatamodeld/Bookmark.xcdatamodel/contents # Unit Tests/Bookmarks/Services/LocalBookmarkStoreTests.swift # Unit Tests/FileSystem/CoreDataEncryptionTests.swift

@samsymons

Task/Issue URL: https://app.asana.com/0/1203137811378537/1204667932873181/f CC: @samsymons Description: Follow-for to Restore the Developer ID script #103 Set Deploy location for Developer ID build to /Applications/DEBUG Added extra NetP Debug menu items to uninstall SysEx/Login Items Moved NetP debug menu items handling methods to AppDelegate to allow their usage when no windows are open

tomasstrba added 7 commits May 3, 2021 18:27

History storage and coordination.

564036a

Regular cleaning of history

f895963

Code cleaning

1126f06

Burning of history

40be7a2

Unit tests

7078191

Unit tests fixed for Bitrise

8bd7bdc

Time of website visit erased from history

7222bc2

tomasstrba commented May 5, 2021

View reviewed changes

DuckDuckGo/Fireproofing/Model/FireproofDomains.swift Show resolved Hide resolved

tomasstrba commented May 5, 2021

View reviewed changes

Unit Tests/Common/Extensions/NSPersistentContainerExtension.swift Show resolved Hide resolved

samsymons self-assigned this May 5, 2021

Fireproofing corrected

45c4cae

brindy reviewed May 6, 2021

View reviewed changes

DuckDuckGo/History/Model/History.swift Outdated Show resolved Hide resolved

Header fixed

70d556c

samsymons reviewed May 10, 2021

View reviewed changes

DuckDuckGo/BrowserTab/ViewModel/WebViewStateObserver.swift Outdated Show resolved Hide resolved

samsymons reviewed May 10, 2021

View reviewed changes

DuckDuckGo/History/Model/HistoryCoordinator.swift Outdated Show resolved Hide resolved

samsymons reviewed May 10, 2021

View reviewed changes

DuckDuckGo/History/Model/HistoryCoordinator.swift Outdated Show resolved Hide resolved

tomasstrba and others added 5 commits May 10, 2021 22:09

Storing of website visits avoided when user does back/forward navigat…

9c400b8

…ion through the history stack

Using Calendar API for date calculations

9d6c650

Co-authored-by: Sam Symons <sam@samsymons.com>

Using Calendar API for date calculations

74e66d7

Naming of the updateTitle() method enhanced

4c72ddf

Body length warning disabled for Tab.swift 🤷

53feeb6

samsymons approved these changes May 11, 2021

View reviewed changes

mallexxx reviewed May 11, 2021

View reviewed changes

Return added to the catch block

bc2d3ab

tomasstrba force-pushed the feature/tom/history-storage branch from 11e9d63 to 4060c15 Compare May 11, 2021 11:37

Date and TimeInterval extensions moved to standalone files

9edc9bb

tomasstrba force-pushed the feature/tom/history-storage branch from 4060c15 to 9edc9bb Compare May 11, 2021 11:38

tomasstrba changed the title ~~History Storage~~ History Storage - Please do not merge May 11, 2021

tomasstrba merged commit c694430 into develop May 24, 2021

tomasstrba deleted the feature/tom/history-storage branch May 24, 2021 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

History Storage - Please do not merge #108

History Storage - Please do not merge #108

tomasstrba commented May 5, 2021 •

edited

Loading

samsymons commented May 10, 2021

tomasstrba commented May 10, 2021

samsymons left a comment

mallexxx May 11, 2021

tomasstrba May 11, 2021

mallexxx May 11, 2021

tomasstrba May 11, 2021 •

edited

Loading

mallexxx May 11, 2021

tomasstrba May 11, 2021

mallexxx May 11, 2021

tomasstrba May 11, 2021

mallexxx May 11, 2021

tomasstrba May 11, 2021

tomasstrba commented May 11, 2021

History Storage - Please do not merge #108

History Storage - Please do not merge #108

Conversation

tomasstrba commented May 5, 2021 • edited Loading

Internal references:

samsymons commented May 10, 2021

tomasstrba commented May 10, 2021

samsymons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasstrba May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasstrba commented May 11, 2021

tomasstrba commented May 5, 2021 •

edited

Loading

tomasstrba May 11, 2021 •

edited

Loading