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

Remove navigatorCredentials #392

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

jonathanKingston
Copy link
Collaborator

@jonathanKingston jonathanKingston commented Jan 7, 2022

Task/Issue URL: https://app.asana.com/0/1163321984198618/1201071474741984/f and https://app.asana.com/0/1200437802575119/1201288740667760/f
Tech Design URL:
CC:

Description:

Removes navigatorCredentials.js and moves it to: duckduckgo/content-scope-scripts#5

This is a follow up from: #389 (merged)

Steps to test this PR:

  1. Open imgur.com and see if it loads
  2. In the web inspector ensure navigator.credentials.get() exists

Testing checklist:

  • Test with Release configuration

Internal references:

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

Base automatically changed from jkt/content-scope to develop January 7, 2022 17:46
@jonathanKingston jonathanKingston force-pushed the jkt/remove-navigator-credentials branch from 52a57f3 to fe9a05c Compare January 8, 2022 00:19
@jonathanKingston
Copy link
Collaborator Author

@bwaresiak this is ready to land, I just need to update the branch reference once the BrowserServicesKit change has landed.

@tomasstrba tomasstrba self-assigned this Jan 18, 2022
@tomasstrba tomasstrba self-requested a review January 18, 2022 12:45
@tomasstrba
Copy link
Contributor

@jonathanKingston, navigator.credentials seems to be undefined. Am I missing something?

Note: I needed to merge develop into this branch in order to compile successfully. Reference to BSK remained yours. Also, it is a good practice to reference a commit directly. Once you push another commit into the branch Xcode won't recognize it 🤷‍♂️ We spent some time figuring this out :D

Screen Shot 2022-01-18 at 14 05 00

@jonathanKingston
Copy link
Collaborator Author

navigator.credentials seems to be undefined. Am I missing something?

Yeah, did you change the submodule in BSK perhaps. I see the code in the other PR: https://github.com/duckduckgo/BrowserServicesKit/pull/47/files (src/features/navigator-interface.js is the new file and it's code should be in the built output build/apple/contentScope.js)

Also, it is a good practice to reference a commit directly. Once you push another commit into the branch Xcode won't recognize it

I'm not sure I follow, by pointing to a feature branch in the PR, if I push to BSK in XCode I can just press update where as with a fixed commit I have to update both PRs. (The intention isn't to keep it as the feature branch at merge)

I needed to merge develop into this branch in order to compile successfully.

I'll rebase :)

@jonathanKingston jonathanKingston force-pushed the jkt/remove-navigator-credentials branch from fe9a05c to bd22a51 Compare January 18, 2022 13:49
Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 Thanks for the rebase!

In my previous comment, I was talking about referencing BSK by its commit instead of a feature branch. Once you push a new commit into the feature branch, it may not be loaded by Xcode
Screen Shot 2022-01-19 at 12 51 15

@jonathanKingston jonathanKingston force-pushed the jkt/remove-navigator-credentials branch 2 times, most recently from bc03815 to 356d3f4 Compare January 19, 2022 14:21
@jonathanKingston
Copy link
Collaborator Author

jonathanKingston commented Jan 19, 2022

In my previous comment, I was talking about referencing BSK by its commit instead of a feature branch. Once you push a new commit into the feature branch, it may not be loaded by Xcode

I wonder if not checking the resolved changes in would help here? I repro'd what you meant with XCode and a clean checkout. That way I can force push and your XCode would resolve correctly?

Anyway pointed to the merged BSK now; please note I removed all package and eslint code as we no longer have JS in the codebase (that's not a repo in it's own right) 🥳 . ESLint was failing because it was matching no files.

@jonathanKingston jonathanKingston force-pushed the jkt/remove-navigator-credentials branch from 356d3f4 to 3714470 Compare January 19, 2022 14:25
@jonathanKingston
Copy link
Collaborator Author

jonathanKingston commented Jan 19, 2022

This is also failing Bitrise because of the removed npm step... can't win. So if you're able to remove that, that should get it passing.

Update: I added this back in but made the test always pass. We could remove this in a follow up commit.

@jonathanKingston jonathanKingston force-pushed the jkt/remove-navigator-credentials branch from 3714470 to a73abe8 Compare January 19, 2022 14:52
@jonathanKingston jonathanKingston changed the title WIP Remove navigatorCredentials Remove navigatorCredentials Jan 19, 2022
@jonathanKingston jonathanKingston merged commit 9868df9 into develop Jan 19, 2022
@jonathanKingston jonathanKingston deleted the jkt/remove-navigator-credentials branch January 19, 2022 15:31
samsymons added a commit that referenced this pull request Feb 18, 2022
# By Alexey Martemyanov (20) and others
# Via Tomas Strba (2) and others
* develop: (63 commits)
  Tweaks of suggestions and autocomplete (#403)
  Bump privacy dashboard to latest version (#409)
  Point to the latest BrowserServicesKit branch. (#414)
  Move embedded TDS from BSK to platform repo (#412)
  Image of shield with dot replaced (#410)
  Expand Fireproofing to include Local Storage and IndexedDB (#408)
  Version 0.18.5
  support privacy config for clickToLoad (#407)
  Automatically select available login (#405)
  initial FB Click to Load (WIP) (#329)
  onboarding updates (#398)
  Version 0.18.4
  Configuration of Sparkle - Setting SUAllowsAutomaticUpdates to NO (#404)
  Hide downloads button if the popover is opened/closed manually (#397)
  Textfield of the homepage is empty and unfocused right after switching to the homepage (#400)
  Remove navigatorCredentials (#392)
  Remove GPC header if it exists when not needed (#366)
  Version 0.18.3
  Fireproofing encrypted storage (#332)
  Fix Lock Screen UI issues (#399)
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo/Crash Reports/Model/CrashReportSender.swift
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.

2 participants