Skip to content
This repository was archived by the owner on Feb 24, 2025. It is now read-only.

Conversation

@tomasstrba
Copy link
Contributor

@tomasstrba tomasstrba commented Jul 16, 2023

Task/Issue URL: https://app.asana.com/0/72649045549333/1203956617610891/f
Tech Design URL: https://app.asana.com/0/481882893211075/1204384679803788/f

Description:
Making Fire Window available to public

Steps to test this PR:

  1. Make sure the UI of Fire Window follows the latest Figma design
  2. Verify Fire Window does not use or add to website state used by regular windows.
  3. Make sure Fire Window does not save website state beyond a single Fire Window session, which ends when closed.
  4. Verify Fire Window doesn't add anything to user's browsing history.
  5. Verify Fire Window does have access to existing autofill items but can't create new autofill items.

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@ayoy ayoy removed their assignment Jul 20, 2023
@tomasstrba
Copy link
Contributor Author

@ayoy, there is a slight change in functionality based on feedback we collected in the ship review. If you'd like to verify it here is the list of things to test:

  • Make sure each Fire Window has its own separate nonpersistant website data store
  • Verify you can't open new Fire Windows using context menus in the existing Fire Window
  • Make sure drag and drop to open a new tab doesn't work in Fire Window
  • Make sure drag and drop doesn't work between Fire Windows and between one regular window and one Fire Window

@tomasstrba tomasstrba requested a review from ayoy July 25, 2023 20:12
Copy link
Collaborator

@ayoy ayoy left a comment

Choose a reason for hiding this comment

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

@tomasstrba all works great now, but I'm leaving an API naming comment for you to take into consideration. Thanks!

enum BurnerStatus: Equatable {

case regular
case burner(websiteDataStore: WKWebsiteDataStore)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering about naming here – BurnerStatus can be pretty cryptic to people not familiar with the topic (especially with no documentation). The following example questions come to my mind:

  • what's Burner?
  • what is "status" really about?

How about calling the enum based on what it actually is, i.e. the type of Website Data Store? I think the following wouldn't even require docs as it's pretty obvious what's the purpose of it:

enum WebsiteDataStoreType: Equatable {
    case `default`
    case ephemeral(WKWebsiteDataStore)

    init(isEphemeral: Bool) {
        if isEphemeral {
            // Each Burner Window has it's own independent website data store that
            // stores website data in memory
            self = .ephemeral(.nonPersistent())
        } else {
            self = .default
        }
    }

    var isEphemeral: Bool {
        switch self {
        case .`default`: return false
        case .ephemeral: return true
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For future reference, Dominik and I discussed the naming on Zoom and agreed to go with BurnerMode.

The reason is, I prefer to keep data store logic encapsulated in Tab and keep the enum just for identification. Agree with Dominik that Status is slightly confusing as it doesn't change over time.

@tomasstrba tomasstrba merged commit 520562c into develop Jul 27, 2023
@tomasstrba tomasstrba deleted the tom/fire-window branch July 27, 2023 08:55
samsymons added a commit that referenced this pull request Jul 28, 2023
# By Dominik Kapusta (7) and others
# Via Sam Symons (2) and others
* develop:
  Modularize debug menu and login items logic #3 (#1397)
  Modularize debug menu and login items logic (#1378)
  Fixes of minor issues with Fire Button after the new logic release (#1395)
  Fire Window (#1357)
  Fix reordering favorites in the last row when it's not fully populated (#1401)
  Close the window when cmd+w is pressed on a pinned tab and there are no regular tabs (#1390)
  update sparkle to 2.4.2 (#1362)
  disable trailing_comma (#1394)
  Add support for per-model Sync Data Providers initialization (#1387)
  update favicon logic to fix gsuite icons (#1380)
  Update minor version by default in code_freeze lane (#1392)
  Upload dSYMs from release builds to S3 for Sentry (#1393)
  Set version to 1.48.2.
  Hide Cards UI on macOS Catalina (#1386)
  Hide Cards UI on macOS Catalina (#1386)
  update shield icon only after navigation ended and tab is selected (#1314)
  Add Debug menu option back in for resetting the Email Protection InContext prompt (#1373)
  Bump version to 1.49.0 (42)
  Update embedded files

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants