Skip to content
This repository has been archived by the owner. It is now read-only.

/ browser-laptop Archived

Looking up site using tab details now considers provisionalLocation #8588

Merged
merged 1 commit into from May 3, 2017
Merged

Looking up site using tab details now considers provisionalLocation #8588

merged 1 commit into from May 3, 2017

Conversation

@bsclifton
Copy link
Member

bsclifton commented Apr 30, 2017 •
edited

Test Plan

folder case

  1. Visit https://brave.com?abc
  2. Add a bookmark and then put this bookmark inside a folder
  3. While you are still at this URL, pin the site
  4. Quit Brave and relaunch
  5. Confirm pinned tab is there
  6. Unpin the tab pinned in step 3
  7. Quit Brave and relaunch
  8. Confirm pinned tab is NOT there

redirect case

  1. Visit a site that you own the hosting for
  2. Pin the site
  3. Quit Brave and relaunch
  4. Confirm pinned tab is there
  5. Quit Brave
  6. Create/update the .htaccess file (or similar for nginx, etc) and put a redirect in to a different URL than step 1
  7. Relaunch Brave
  8. Unpin the tab pinned in step 2
  9. Quit Brave and relaunch
  10. Confirm pinned tab is NOT there

Description

This PR fixes two bugs with pinned tabs

  • if you pin a tab which is bookmarked and inside a folder, you can't unpin it
  • if you pin a tab which later redirects the original URL, you can't unpin it

Looking up site using tab details now considers provisionalLocation
Result returned now includes parentFolderId

Fixes #8477

Auditors: @bbondy, @NejcZdovc

cc: @darkdh

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
@bsclifton bsclifton added this to the 0.15.2 milestone Apr 30, 2017
@bsclifton bsclifton self-assigned this Apr 30, 2017
@bsclifton bsclifton requested review from bbondy, NejcZdovc and darkdh Apr 30, 2017
@bsclifton bsclifton mentioned this pull request Apr 30, 2017
Closed
@darkdh
darkdh reviewed May 1, 2017
View changes
js/state/siteUtil.js Outdated
@@ -444,14 +444,69 @@ module.exports.getDetailFromFrame = function (frame, tag) {
})
}

module.exports.getDetailFromTab = function (tab, tag) {
const getSitesLikeKey = (sites, siteKey, tag) => {
if (!sites || !siteKey) {

This comment has been minimized.

Sign in to view
Copy link
@darkdh

darkdh May 1, 2017

Member

getSitesLikeKey seems not very instinctive to me. Maybe getSiteBySubKey? And we can extend this function to support more sub key if needed

This comment has been minimized.

Sign in to view
Copy link
@bsclifton

bsclifton May 2, 2017

Author Member

@darkdh I like that! 👍

This comment has been minimized.

Sign in to view
Copy link
@darkdh

darkdh May 2, 2017

Member

sorry my bad for typo. I think it should be getSitesBySubkey the plural 😛

This comment has been minimized.

Sign in to view
Copy link
@bsclifton

bsclifton May 2, 2017

Author Member

Updated 😄
js/state/siteUtil.js
let location = tab.get('url')
const partitionNumber = tab.get('partitionNumber') !== undefined
? tab.get('partitionNumber')
: undefined

This comment has been minimized.

Sign in to view
Copy link
@darkdh

darkdh May 1, 2017

Member

Is it supposed to be 0?

This comment has been minimized.

Sign in to view
Copy link
@bsclifton

bsclifton May 2, 2017

Author Member

I left as undefined so that it would skip the below code, which only sets if the value is truthy:

if (partitionNumber) {
  siteDetail.partitionNumber = partitionNumber
@bsclifton
Copy link
Member Author

bsclifton commented May 2, 2017 •
edited

@darkdh updated per feedback 👍

(reviewers: please don't merge without squashing!)
@bsclifton
Looking up site using tab details now considers provisionalLocation
Unverified
This user has not uploaded their public key yet.
GPG key ID: 37FF5F19022427F2 Learn about signing commits
Loading status checks…
902debd 
Result returned now includes parentFolderId

Fixes #8477

Auditors: @bbondy, @NejcZdovc

cc: @darkdh

Includes:
Fixes per feedback per @darkdh
@bsclifton bsclifton merged commit 728ebd8 into brave:master May 3, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@bsclifton bsclifton deleted the bsclifton:fix-pinned-tab-not-removing branch May 3, 2017
@srirambv
Copy link
Collaborator

srirambv commented May 8, 2017

Verified folder test case on Windows 10 x64
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@darkdh darkdh

@bbondy bbondy

@NejcZdovc NejcZdovc

Assignees

@bsclifton bsclifton

Labels
QA/test-plan-specified feature/pinned-tabs release-notes/include
Projects
None yet
Milestone
  0.15.300 (Frozen,...
Linked issues

Successfully merging this pull request may close these issues.

Pinned tabs reappear next load after unpinning
3 participants
@bsclifton @srirambv @darkdh
You can’t perform that action at this time.