[WIP] Disable Brave Shields + extension on navigation error #9746
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Wanted to run the approach by folks first.)
We added a fingerprinting strict protection where we disable dark mode. We shouldn't apply that on 'internal' pages (for e.g. brave://settings, error pages). The
brave://
URL case is taken care of by checking for Brave Shields being up, but turns out that on error pages (no wifi, no DNS) shields is still up, which breaks dark mode on error pages.In this PR, my current (faulty) approach to fix this is:
onErrorOccurred
event fromwebNavigation
error
on the Tab.onCommitted
fires, check if the current tab has an error. If yes, clear the error, check the old value of shields set on the Tab and set the old value of Shields on the current originThis is flawed, because Brave Shields being up or down is an origin concept, while the browser action is a tab concept. So we need to store the old (correct) value of Brave Shields for that origin somewhere else (not on the Tab). I could create a new map of
{origin => braveShieldValues}
but I think that would lead to a problem where the user goes to https://twitter.com, has their wifi off, the Shields is set to down and{'twitter.com' : 'up'}
is stored in memory, but the user now quits the browser. Next time they relaunch and go to https://twitter.com, there would be no in-memory map and they would see the Shields being down even though their wifi is now working.In general, this approach seems pretty complicated. Am I missing something, is there an easier way to do this?
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: