Expose way for sites to determine if browser is Brave

[bsclifton]

Fixes brave/brave-browser#8216

Submitter Checklist:

• 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).
• Verified that these changes build without errors on
  • Android
  • iOS
  • Linux
  • macOS
  • Windows
• Verified that these changes pass automated tests (unit, browser, security tests) on
  • iOS
  • Linux
  • macOS
  • Windows
• Verified that all lint errors/warnings are resolved (npm run lint)
• Ran git rebase master (if needed).
• Ran git rebase -i to squash commits (if needed).
• Tagged reviewers and labelled the pull request as needed.
• Request a security/privacy review as needed.
• Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone
• Public documentation has been updated as necessary. For instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protection-Mode
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-compatibility-issues-with-tracking-protection
  • https://github.com/brave/brave-browser/wiki/QA-Guide

Test Plan:

1. See Desktop :: Implement "Enabling Sites to Determine Brave" Spec brave-browser#8216
2. npm run test -- brave_browser_tests --filter=NavigatorGetBraveDetectedTest.*

Reviewer Checklist:

• New files have MPL-2.0 license header.
• Request a security/privacy review as needed.
• Adequate test coverage exists to prevent regressions
• Verify test plan is specified in PR before merging to source

After-merge Checklist:

• The associated issue milestone is set to the smallest version that the
  changes has landed on.
• All relevant documentation has been updated.

[bsclifton]

Note to self - this would require updating https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)

[jumde]

Two questions: I'm running a build to confirm

1. Are we enabling any Client Hints with this change?
2. Is Brave being appended to the user agent (navigator.userAgent).

[bsclifton]

@jumde answers:

1. No - client hints is not enabled. It remains off by default. We'd need to modify src/brave/app/brave_main_delegate.cc to add kUserAgentClientHint
2. Actual user agent is NOT modified (ex: navigator.userAgent)

[bsclifton]

Needs some patch cleanup and need to modify test plan (will do that later tonite)

[bsclifton]

Ready for review 😄 I'd love to further clean up patches, but need help in order to do so
cc: @bridiver @petemill @pes10k @jumde

[pes10k]

@bsclifton sure happy to help. What would be most helpful?

[bsclifton]

@pes10k mostly just looking at the code to see if the approach taken looks good 😄 I guess we went over it together, so you can share some notes and approve/deny based on that (just to have something formal on GitHub)

[pes10k]

@bsclifton made a tiny suggestion, hope its helpful. Otherwise, LGTM!

[bridiver]

maybe modules.gni ?

[bsclifton]

considered that- but it also includes core changes

[bridiver]

but its purpose is to make modules work. If nothing else config.gni would be a better choice, but it can be a follow-up

[bsclifton]

Ready for re-review 😄👍

[pes10k]

wunderbar, lgtm!

[pes10k]

lgtm!

[bsclifton]

Only CI failure was during Publish xUnit test result report - (1.5 sec in self) on macOS:
https://ci.brave.com/job/brave-browser-build-pr/job/bsc-detect-brave/15/execution/node/641/log/

All builds / tests / create_dists / etc worked great 😄👍

[iefremov]

EXPECT_TRUE(EvalJs(contents, "getBraveDetected()"));

[bsclifton]

I tried to do this when you had shared feedback before; it's causing a problem though:

../../third_party/googletest/src/googletest/include/gtest/gtest.h:299:9: error: no viable conversion from 'const content::EvalJsResult' to 'bool'

Not sure why comparison with EXPECT_EQ is handled differently than EXPECT_TRUE?

[zenparsing]

From browser_test_utils.h:

//      For boolean results, note that EXPECT_TRUE(..) and EXPECT_FALSE()
//      won't compile; use EXPECT_EQ(true, ...) instead. This is intentional,
//      since EXPECT_TRUE() could be read ambiguously as either "expect
//      successful execution", "expect truthy value of any type", or "expect
//      boolean value 'true'".

[bsclifton]

ah- thanks, @zenparsing! 😄

[kjozwiak]

Verification PASSED on macOS 10.15.3 x64 using the following build:

Brave | 1.7.47 Chromium: 80.0.3987.132 (Official Build) nightly (64-bit)
-- | --
Revision | fcea73228632975e052eb90fcf6cd1752d3b42b4-refs/branch-heads/3987@{#974}
OS | macOS Version 10.15.3 (Build 19D76)

• Verified the STR outlined via Desktop :: Implement "Enabling Sites to Determine Brave" Spec brave-browser#8216

[Nathan13888]

How could this be disabled?

[bridiver]

How could this be disabled?

We have discussed some options including that it may be disabled when shields are down and I would personally be in favor of a feature flag or pref. Cc @pes10k

[Nathan13888]

Agreed. A feature flag would be more modular and obvious to configure. Using a feature flag would also allow for the feature to be disabled by default too.