Using temporarySiteSettings to store private tab preferences #4469
Conversation
| } | ||
| } | ||
| hostSettingsToContentSettings(appState.get('siteSettings').toJS(), contentSettings) | ||
| hostSettingsToContentSettings(appState.get('temporarySiteSettings').toJS(), contentSettings) |
There was a problem hiding this comment.
this is going to send the private settings to the non-private tabs. We can't sync all the sessions, we have to send the temp + regular to the private session and just the regular to the regular session. Probably use setUserPref with the incognito flag instead of registerUserPrefs
| } | ||
| } | ||
| hostSettingsToContentSettings(appState.get('siteSettings').toJS(), contentSettings) | ||
| hostSettingsToContentSettings(appState.get('temporarySiteSettings').toJS(), contentSettings) |
There was a problem hiding this comment.
shouldn't temporarySiteSettings get added only if the content settings are being sent to a private frame?
There was a problem hiding this comment.
nvm @bridiver made the same comment above
| const activeFrame = FrameStateUtil.getActiveFrame(this.props.windowState) | ||
| if (activeFrame && activeFrame.get('isPrivate')) { | ||
| return this.props.appState.get('temporarySiteSettings') | ||
| } |
There was a problem hiding this comment.
need to be careful here that the allSiteSettings frame property is consistent with what is being sent in js/state/contentSettings.js and also registerPermissionHandler in app/filtering.js. the expected behavior is that private context inheirit settings from regular contexts but not vice-versa, so I think what you want here is a merge of temporarySiteSettings and siteSettings for private frames.
2. Inherit settings from regular session on private session
| const regularSettings = hostSettingsToContentSettings(appState.get('siteSettings').toJS(), contentSettings) | ||
| if (isPrivate) { | ||
| const privateSettings = | ||
| hostSettingsToContentSettings(appState.get('siteSettings').merge(appState.get('temporarySiteSettings')).toJS(), |
There was a problem hiding this comment.
sorry I missed this earlier, but the deep merge is not required or desirable. The overlay pref store already handles this correctly if you set the regular settings for the regular profile and only the private settings for the private profile
There was a problem hiding this comment.
bool OverlayUserPrefStore::GetValue(const std::string& key,
const base::Value** result) const {
// If the |key| shall NOT be stored in the overlay store, there must not
// be an entry.
DCHECK(ShallBeStoredInOverlay(key) || !overlay_.GetValue(key, NULL));
if (overlay_.GetValue(key, result))
return true;
return underlay_->GetValue(GetUnderlayKey(key), result);
}
|
thanks a ton, this is mostly working! i did find one bug:
i am pretty sure this is because you need to edit also, please add a private tab test to |
|
@darkdh going to merge this in so we can get some testing, but please do the last comments in a follow up bug. |
|
will do |
It was added to this list: Auditors: @bbondy
git rebase -ito squash commits (if needed).fix #4468
Auditors: @diracdeltas , @bridiver
Test Plan:
Test bravery panel, site permissions, mixed content, flash in private tabs and the preferences should not write to disk and affect regular tabs
There is a better way to achieve this but require architectural change.
We plan to supersede siteSettings and temporarySiteSettings by reading and writing user prefs directly. Private sessions will keep in-memory overlaid user prefs, but still be able to read the normal session prefs that haven’t been changed in private tabs.