Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Added JS event listener for tab web-state. #5181

Merged
merged 2 commits into from
Mar 31, 2022

Conversation

Brandon-T
Copy link
Collaborator

@Brandon-T Brandon-T commented Mar 30, 2022

Security Review

Summary of Changes

  • Fix tab screenshots in TabTray
  • Solidify Session Restore of dynamic pages

This pull request fixes #5180

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue and pull request is assigned to a milestone (should happen at merge time).

@Brandon-T Brandon-T added this to the 1.38 milestone Mar 30, 2022
@Brandon-T Brandon-T self-assigned this Mar 30, 2022
@Brandon-T Brandon-T force-pushed the bugfix/ScreenshotsAndDynamicURLs branch from 12e054d to 0de8e2e Compare March 30, 2022 18:16
Copy link
Collaborator

@pes10k pes10k left a comment

Choose a reason for hiding this comment

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

looks good to me privacy-wise, though i made a few small suggestions for cleanup and consistency

Copy link
Contributor

@iccub iccub left a comment

Choose a reason for hiding this comment

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

Good job, i ran this code for a bit and seems to work for all cases and websites

@Brandon-T Brandon-T requested a review from diracdeltas March 31, 2022 16:45
@Brandon-T Brandon-T merged commit f339def into development Mar 31, 2022
@Brandon-T Brandon-T deleted the bugfix/ScreenshotsAndDynamicURLs branch March 31, 2022 18:26
@diracdeltas
Copy link
Member

@Brandon-T i was not done reviewing this yet - in the future please wait til sec review is closed

@Brandon-T
Copy link
Collaborator Author

@Brandon-T i was not done reviewing this yet - in the future please wait til sec review is closed

Ahh sorry. I saw the sgtm (and I read it as seems good to merge). Sorry again. I should have checked.
I can revert it and then reopen this PR.

@diracdeltas
Copy link
Member

I can revert it and then reopen this PR.

@Brandon-T thanks, no need in this case since my comments are mostly questions. but for future reference sgtm = sounds good to me and the sec review ticket should be the source of truth.

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.

[Bug] - Websites with Dynamic Loading not storing screenshots & state properly
4 participants