Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a farbling-determined voice name to getVoices #10618

Merged
merged 1 commit into from
Nov 5, 2021
Merged

Add a farbling-determined voice name to getVoices #10618

merged 1 commit into from
Nov 5, 2021

Conversation

pilgrim-brave
Copy link
Contributor

@pilgrim-brave pilgrim-brave commented Oct 20, 2021 •
edited
Loading

Resolves brave/brave-browser#18062

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@pilgrim-brave pilgrim-brave requested a review from a team as a code owner October 20, 2021 16:39
@pilgrim-brave pilgrim-brave marked this pull request as draft October 20, 2021 16:40
@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_farble_voices branch 2 times, most recently from 10960ab to 2abb1ff Compare November 2, 2021 19:36
@pilgrim-brave pilgrim-brave marked this pull request as ready for review November 2, 2021 19:36
@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_farble_voices branch from 2abb1ff to 031c8c8 Compare November 2, 2021 19:40
@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_farble_voices branch from 031c8c8 to 1c5b49f Compare November 4, 2021 18:51
@pilgrim-brave pilgrim-brave requested a review from a team as a code owner November 4, 2021 18:56
@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_farble_voices branch 3 times, most recently from e202abf to 4e25a01 Compare November 5, 2021 04:04
goodov
goodov reviewed Nov 5, 2021
View reviewed changes
browser/farbling/brave_speech_synthesis_farbling_browsertest.cc Show resolved Hide resolved
browser/farbling/brave_speech_synthesis_farbling_browsertest.cc Outdated
EXPECT_EQ(off_voices_b, off_voices_z);

// On platforms without any voices, the rest of this test is invalid.
if (off_voices_b == "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could hide the real issues. Can we do this check with platform-specific ifdefs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the API itself is so flaky (requires setting a timeout to decide that there are no voices) that the test sometimes fails on Windows CI where there are definitely voices. This was the compromise solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, but maybe it's the title-observing approach flaky in its nature? I was wondering why you decided to go with it instead of directly calling async JS via something like content::EvalJS.

chromium_src/third_party/blink/renderer/modules/speech/speech_synthesis.cc Outdated Show resolved Hide resolved
chromium_src/third_party/blink/renderer/modules/speech/speech_synthesis.cc Outdated Show resolved Hide resolved
@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_farble_voices branch from 4e25a01 to 0b094e8 Compare November 5, 2021 13:10
@pilgrim-brave
Add a farbling-determined voice name to getVoices
8a6e84d 
call upstream function if farbling is off
add DEPS
exit early if no voices
extend test timeout
move exit condition earlier
feedback
@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_farble_voices branch from 0b094e8 to 8a6e84d Compare November 5, 2021 14:44
@pilgrim-brave pilgrim-brave merged commit 6e72cdc into master Nov 5, 2021
@pilgrim-brave pilgrim-brave deleted the mpilgrim_farble_voices branch November 5, 2021 16:54
goodov
goodov reviewed Nov 8, 2021
View reviewed changes
chromium_src/third_party/blink/renderer/modules/speech/speech_synthesis.cc
"Hubert", "Vernon", "Rudolph", "Clayton", "Irving",
"Wilson", "Alva", "Harley", "Beauregard", "Cleveland",
"Cecil", "Reuben", "Sylvester", "Jasper"};
const int kFakeNamesCount = 14;
Copy link
Member

@goodov goodov Nov 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base::size to the rescue! feel free to use it whenever you have a good ol' array.
https://github.com/chromium/chromium/blob/97.0.4690.7/base/cxx17_backports.h#L29-L32

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logged brave/brave-browser#19316 for followup

This was referenced Nov 9, 2021
Closed
Closed
@pilgrim-brave pilgrim-brave added this to the 1.35.x - Release milestone Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goodov goodov goodov left review comments

@bridiver bridiver bridiver approved these changes

Assignees

@pilgrim-brave pilgrim-brave

Labels
None yet
Projects
None yet
Milestone
1.35.x - Release
Development

Successfully merging this pull request may close these issues.

Fingerprinting 3.0: SpeechSynthesis API
3 participants
@pilgrim-brave @bridiver @goodov