Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upSync siteSettings with browser data #6721
Conversation
ayumi
force-pushed the
feature/syncing-settings
branch
2 times, most recently
from
59e0379
to
b928fb6
| let existingObject = this.getObjectById(objectId, category) | ||
| if (!existingObject) { | ||
| applySetting('objectId', objectId) | ||
| existingObject = this.getObjectById(objectId, category) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ayumi
Jan 19, 2017
Author
Contributor
I think Deletes are only sent when objectId is set, however it's probably good to check and skip it if it's a Delete.
This comment has been minimized.
This comment has been minimized.
| } else if (key === 'cookieControl') { | ||
| value = cookieControlEnum[value] | ||
| } | ||
| if (existingObjectData.get(key) === value) { continue } |
This comment has been minimized.
This comment has been minimized.
diracdeltas
Jan 19, 2017
Member
i think this needs to call applySetting anyway (instead of continue) when the action is DELETE. null may be a valid siteSetting
| @@ -620,11 +628,15 @@ const handleAppAction = (action) => { | |||
| let propertyName = action.temporary ? 'temporarySiteSettings' : 'siteSettings' | |||
| let newSiteSettings = new Immutable.Map() | |||
| appState.get(propertyName).map((entry, hostPattern) => { | |||
| // const value = entry.get(action.key) | |||
This comment has been minimized.
This comment has been minimized.
|
i tried the test plan and it was mostly glorious. however there is an issue where some site settings are not being removed. repro
|
|
when i go to |
|
@diracdeltas i made the changes! For the last thing: Currently clearing Synced data categories clears it locally and only prevents future records from being synced; it doesn't clear categories on devices which have already synced the data. |
Auditors: @diracdeltas Test Plan: Prep: 0. Update sync lib to `brave/sync #fix/resolve-delete-nonexistant-props`. 1. Prepare 2 instances (pyramids) of Brave with Sync enabled. - Enable Sync and close Brave. - Copy `{userData}/brave-development` to `{userData}/brave-development-2`. - Edit `brave-development-2/session-store-1` `deviceId` to `1`. - To `browser-laptop` `package.json` add `"start2": "node ./tools/start.js --user-data-dir=brave-development-2 --debug=5859 --enable-logging --v=0 --enable-extension-activity-logging --enable-sandbox-logging --enable-dcheck",` 2. In `appConfig.js` `sync.fetchInterval` reduce to 5 seconds. Play: 3. Open both pyramid 1 and pyramid 2. 4. In pyramid 1 visit a webpage and open up the bravery panel. 5. In pyramid 2 visit the same page and open up the bravery panel. 6. In pyramid 2 toggle each available siteSetting. Observe it appears in pyramid 1 after 1–5s. 7. Try toggling multiple settings at once, and toggling different settings simulatenously on both pyramids. 8. In both go to Preferences #Shields. Clear siteSettings with the Clear links and the red X's. Observe they sync over.
sgtm, can you open an issue in brave/sync for tracking? |
|
|
ayumi commentedJan 19, 2017
Auditors: @diracdeltas
Test Plan:
Prep:
brave/sync #fix/resolve-delete-nonexistant-props.a. Enable Sync and close Brave.
b. Copy
{userData}/brave-developmentto{userData}/brave-development-2.c. Edit
brave-development-2/session-store-1deviceIdto1.d. To
browser-laptoppackage.jsonadd"start2": "node ./tools/start.js --user-data-dir=brave-development-2 --debug=5859 --enable-logging --v=0 --enable-extension-activity-logging --enable-sandbox-logging --enable-dcheck",appConfig.jssync.fetchIntervalreduce to 5 seconds.Play: