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

Bugfix FXIOS-5304 [v109] Fixes pinned sites no longer work from add to shortcuts #12579

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

tarikeshaq
Copy link
Contributor

@tarikeshaq tarikeshaq commented Dec 4, 2022

Bugfix for #12461

Removes the extra query to the browser.db when adding a pinned site through the Add to Shortcuts button.

  • In 107 we removed favicons from the SQL layer
  • The query joins with the favicons table, which there is a really good chance doesn't have the site
  • The query returns an empty site, then the actual add to pinned sites won't run

This PR removes that extra query and instead writes directly to the pinned sites. Note that since we no longer run the query, we now don't have the site's guid. but that's okay, it's nullable and pinned sites don't really need to have site's history guid.

I put this as 108 since it was a regression in 107. But please feel free to change it up to whatever the team sees fit!

@lmarceau
Copy link
Contributor

lmarceau commented Dec 5, 2022

v108 is now on hard freeze, so I'll put this in as v109

@lmarceau lmarceau changed the title Bugfix FXIOS-5304 [v108] Fixes pinned sites no longer work from add to shortcuts Bugfix FXIOS-5304 [v109] Fixes pinned sites no longer work from add to shortcuts Dec 5, 2022
@lmarceau lmarceau self-requested a review December 5, 2022 18:48
@lmarceau
Copy link
Contributor

lmarceau commented Dec 5, 2022

Build was green here

@lmarceau lmarceau requested a review from nbhasin2 December 5, 2022 18:59
Copy link
Contributor

@nbhasin2 nbhasin2 left a comment

Choose a reason for hiding this comment

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

Sync'd with @tarikeshaq and this does not support backwards compatibility so v109 change once happens will require additional work if we ever want to downgrade.

@tarikeshaq Do you think its worth to support backward compatibility?

@mergify
Copy link
Contributor

mergify bot commented Dec 8, 2022

This pull request has conflicts when rebasing. Could you fix it @tarikeshaq?

@mergify
Copy link
Contributor

mergify bot commented Dec 8, 2022

This pull request has conflicts when rebasing. Could you fix it @tarikeshaq?

@tarikeshaq
Copy link
Contributor Author

@tarikeshaq Do you think its worth to support backward compatibility?

Because it's close to the 109 cut off, and removing backward compatibility meant crashing if we downgrade, I simplified the fix to only skip the joining with the favicon table - which is a safer approach, but we keep the extra query

cc @nbhasin2 does that make sense? if so we can probably land this so it hits 109

@nbhasin2
Copy link
Contributor

nbhasin2 commented Dec 8, 2022

This is much simpler @tarikeshaq r+

@lmarceau
Copy link
Contributor

lmarceau commented Dec 9, 2022

Build is green here

@lmarceau lmarceau merged commit 6551207 into mozilla-mobile:main Dec 9, 2022
@tarikeshaq tarikeshaq deleted the fixes-pinned-sites branch December 10, 2022 03:18
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.

3 participants