[Linux] Implement First Run EULA dialog [chromium/src : main]
Thomas Anderson (Gerrit)
Thomas Anderson voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 12 comments![]( "Open in Gerrit")
Adds a views-based EULA dialog for Linux that appears during the firstDoes Windows do this as well? If so, are there any opportunities to share code?
UI demo:Out of curiosity, does it need to mention Chrome OS?
https://g-issues.chromium.org/issues/470084686#comment2No prefix.
- Adds HTML-to-text conversion for displaying the EULA in a... because the cross-platform EULA is HTML?
Just curious, is spinning up a renderer to displaying HTML not a thing that Views can do?
I'm also somewhat worried that the HTML parsing is fragile.
#if BUILDFLAG(IS_LINUX)Does it make sense to integrate this with ChromeBrowserMainParts::ApplyFirstRunPrefs()?
if (!first_run::MaybeShowEulaDialog()) {Combine with previous line?
EulaDialog(base::OnceClosure on_accept, base::OnceClosure on_cancel)Why not just 1 Callback that takes a bool?
// Ensure we have a task runner for the loop."there exists"
base::CreateDirectory(sentinel.DirName());The parent dir is `chrome::DIR_USER_DATA`. Does that need to be created?
base::PathService::Get(chrome::DIR_USER_DATA, &sentinel);In tests, use the checked version.
sources += [ "../browser/first_run/first_run_internal_linux_unittest.cc" ]sources first.
Related details
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson voted and added 12 comments![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Auto-Submit | +1 |
| Commit-Queue | +1 |
12 comments
Adds a views-based EULA dialog for Linux that appears during the firstDoes Windows do this as well? If so, are there any opportunities to share code?
On Windows, the EULA dialog runs in a subprocess (setup.exe), so code cannot be shared with the current design. Windows could theoretically switch to the views impl in the future, but I'm not sure why the subprocess design was made to say if it actually should.
UI demo:Out of curiosity, does it need to mention Chrome OS?
The title was copied from the EULA html's <title> tag, and that string also appears several times in the actual EULA. I'm not a lawyer, so I'm too scared to change it.
Thomas AndersonNo prefix.
Done
- Adds HTML-to-text conversion for displaying the EULA in a... because the cross-platform EULA is HTML?
Just curious, is spinning up a renderer to displaying HTML not a thing that Views can do?
I'm also somewhat worried that the HTML parsing is fragile.
the dialog gets shown too early during init to render web contents.
You can view how the EULA is supposed to look at chrome://terms. It's just plaintext except the title and "AVC" are bold. We can possibly just get away with changing it to plaintext in the repo.
For now, I've added DCHECKs in ConvertHtmlEulaToText to ensure all nodes are of a recognized type.
#if BUILDFLAG(IS_LINUX)Does it make sense to integrate this with ChromeBrowserMainParts::ApplyFirstRunPrefs()?
It needs to run later because UI is not available during ApplyFirstRunPrefs. I chose here because this function already has several first_run:: calls, and this also runs before profile init, which is required since otherwise the first_run codepath wouldn't get hit.
if (!first_run::MaybeShowEulaDialog()) {Thomas AndersonCombine with previous line?
Done
EulaDialog(base::OnceClosure on_accept, base::OnceClosure on_cancel)Why not just 1 Callback that takes a bool?
Done
// Ensure we have a task runner for the loop.Thomas Anderson"there exists"
Done
if (!sentinel.empty()) {Thomas AndersonAlways evaluates to true.
Done
base::CreateDirectory(sentinel.DirName());The parent dir is `chrome::DIR_USER_DATA`. Does that need to be created?
It's already created before this gets called (though it doesn't yet have a profile). Changed this to a check that it exists.
base::PathService::Get(chrome::DIR_USER_DATA, &sentinel);In tests, use the checked version.
Done
sources += [ "../browser/first_run/first_run_internal_linux_unittest.cc" ]- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
Adds a views-based EULA dialog for Linux that appears during the firstThomas AndersonDoes Windows do this as well? If so, are there any opportunities to share code?
On Windows, the EULA dialog runs in a subprocess (setup.exe), so code cannot be shared with the current design. Windows could theoretically switch to the views impl in the future, but I'm not sure why the subprocess design was made to say if it actually should.
May be good to get chrome/browser/first_run/OWNERS to review as well.
Related details
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
return std::any_of(std::begin(kBlockCloseTags), std::end(kBlockCloseTags),std::ranges::any_of()
constexpr const char html_content[] = R"(<!DOCTYPE html>Should the test case just parse `IDS_TERMS_HTML` instead of test data?
Related details
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
return std::any_of(std::begin(kBlockCloseTags), std::end(kBlockCloseTags),Thomas Andersonstd::ranges::any_of()
Done
Should the test case just parse `IDS_TERMS_HTML` instead of test data?
I've added a test that parses IDS_TERMS_HTML (just to verify no DCHECKs are hit), but it doesn't actually check the output since the terms get updated sometimes.
Related details
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 3 comments![]( "Open in Gerrit")
Adds a views-based EULA dialog for Linux that appears during the firstThomas AndersonDoes Windows do this as well? If so, are there any opportunities to share code?
Lei ZhangOn Windows, the EULA dialog runs in a subprocess (setup.exe), so code cannot be shared with the current design. Windows could theoretically switch to the views impl in the future, but I'm not sure why the subprocess design was made to say if it actually should.
May be good to get chrome/browser/first_run/OWNERS to review as well.
+grt@
constexpr const char html_content[] = R"(<!DOCTYPE html>Thomas AndersonShould the test case just parse `IDS_TERMS_HTML` instead of test data?
I've added a test that parses IDS_TERMS_HTML (just to verify no DCHECKs are hit), but it doesn't actually check the output since the terms get updated sometimes.
Maybe check for a few key phrases that are lilely to be in the EULA?
if (ui::ResourceBundle::HasSharedInstance()) {Not sure if this evaluates to true without additional work, like calling ui::ResourceBundle::InitSharedInstanceWithLocale(). This and line 135 should be deterministic in a test.
Related details
- Greg Thompson
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
constexpr const char html_content[] = R"(<!DOCTYPE html>Thomas AndersonShould the test case just parse `IDS_TERMS_HTML` instead of test data?
Lei ZhangI've added a test that parses IDS_TERMS_HTML (just to verify no DCHECKs are hit), but it doesn't actually check the output since the terms get updated sometimes.
Maybe check for a few key phrases that are lilely to be in the EULA?
In unbranded builds (which is where the tests would normally run), the contents of the EULA are:
```
This Space Intentionally Blank
In official builds this space will show the terms of service.
```
(It also has a typo, it should say branded builds, not official builds)
So there aren't really any strings in common (between branded and non branded) to test against. I've instead added a check that the html and text representations are non-empty.
Not sure if this evaluates to true without additional work, like calling ui::ResourceBundle::InitSharedInstanceWithLocale(). This and line 135 should be deterministic in a test.
- Greg Thompson
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
constexpr const char html_content[] = R"(<!DOCTYPE html>Thomas AndersonShould the test case just parse `IDS_TERMS_HTML` instead of test data?
Lei ZhangI've added a test that parses IDS_TERMS_HTML (just to verify no DCHECKs are hit), but it doesn't actually check the output since the terms get updated sometimes.
Thomas AndersonMaybe check for a few key phrases that are lilely to be in the EULA?
In unbranded builds (which is where the tests would normally run), the contents of the EULA are:
```
This Space Intentionally Blank
In official builds this space will show the terms of service.
```(It also has a typo, it should say branded builds, not official builds)
So there aren't really any strings in common (between branded and non branded) to test against. I've instead added a check that the html and text representations are non-empty.
- Greg Thompson
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 2 comments![]( "Open in Gerrit")
Adds a views-based EULA dialog for Linux that appears during the firstThomas AndersonDoes Windows do this as well? If so, are there any opportunities to share code?
Lei ZhangOn Windows, the EULA dialog runs in a subprocess (setup.exe), so code cannot be shared with the current design. Windows could theoretically switch to the views impl in the future, but I'm not sure why the subprocess design was made to say if it actually should.
Lei ZhangMay be good to get chrome/browser/first_run/OWNERS to review as well.
+grt@
I wasn't part of the design back then, but my understanding has always been that we use the installer to show the EULA on Windows so that we don't have to worry about accidentally starting the browser (or anything related to it) before the user has had a chance to see the ToS.
if (!headless::IsHeadlessMode() && !first_run::MaybeShowEulaDialog()) {is there any way we can do this before the metrics service is started (the EULA is shown during `PreEarlyInitialization` on Windows)? i think we need to take care to not record or report any metrics before the ToS are accepted.
Related details
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson added 2 comments![]( "Open in Gerrit")
Adds a views-based EULA dialog for Linux that appears during the firstThomas AndersonDoes Windows do this as well? If so, are there any opportunities to share code?
Lei ZhangOn Windows, the EULA dialog runs in a subprocess (setup.exe), so code cannot be shared with the current design. Windows could theoretically switch to the views impl in the future, but I'm not sure why the subprocess design was made to say if it actually should.
Lei ZhangMay be good to get chrome/browser/first_run/OWNERS to review as well.
Greg Thompson+grt@
I wasn't part of the design back then, but my understanding has always been that we use the installer to show the EULA on Windows so that we don't have to worry about accidentally starting the browser (or anything related to it) before the user has had a chance to see the ToS.
Acknowledged
if (!headless::IsHeadlessMode() && !first_run::MaybeShowEulaDialog()) {is there any way we can do this before the metrics service is started (the EULA is shown during `PreEarlyInitialization` on Windows)? i think we need to take care to not record or report any metrics before the ToS are accepted.
Yes, that is done in this CL because this EULA dialog will show **before** the first-run dialog with a checkbox for "Automatically send usage statistics and crash reports to Google".
Related details
- Greg Thompson
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 12 comments![]( "Open in Gerrit")
if (!headless::IsHeadlessMode() && !first_run::MaybeShowEulaDialog()) {Thomas Andersonis there any way we can do this before the metrics service is started (the EULA is shown during `PreEarlyInitialization` on Windows)? i think we need to take care to not record or report any metrics before the ToS are accepted.
Yes, that is done in this CL because this EULA dialog will show **before** the first-run dialog with a checkbox for "Automatically send usage statistics and crash reports to Google".
I was referring to line 1620 above. Can that happen after the dialog is accepted or refused?
This might be an odd combination, but what happens if an install that requires the ToS is started on a machine that's enterprise-managed and has the "MetricsReportingEnabled" policy enabled? I think we still need to honor showing the ToS before collecting any metrics. Are you certain that not only reporting but also recording will be off at this point in that case?
In any case, I think we should have a comment here explaining what the guarantee is so that future refactors are less likely to accidentally move this later. Even better would be some tests for cases like this to prevent regressions.
bool MaybeShowEulaDialog();long ago, i was asked to replace "Maybe" with something more explicit in functions like this. WDYT about `ShowEulaDialogIfRequired`, with the comment explaining the condition(s) under which it's required?
on second thought, see my comment in first_run.cc. i think we can avoid "Maybe" and "IfRequired" altogether.
// or not required. Returns false if the EULA has not been accepted.maybe add to the comment that the process must exit promptly in this case? this is an important details for callers of this function.
!internal::ShowPostInstallEULAIfNeeded(initial_prefs.get())) {rather than rely on setting a global in this fn that is later used to actually show the eula, i think it's more clean to have two eula-launching paths: windows will do it here as always. linux can continue to ignore the call to `ShowPostInstallEULAIfNeeded`. add a new `bool eula_required;` for linux in the `MasterPrefs` struct that is populated in `SetupInitialPrefsFromInstallPrefs` and then change `ChromeBrowserMainParts::PreMainMessageLoopRunImpl` to something like:
```
#if BUILDFLAG(IS_LINUX)
// On Linux, the EULA dialog requires Views, so it is shown here rather than
// when applying the first-run prefs.
if (first_run::IsChromeFirstRun() && master_prefs_->eula_required &&
!headless::IsHeadlessMode() !first_run::ShowEulaDialog()) {
return CHROME_RESULT_CODE_EULA_REFUSED;
}
#endif
```
this also removes the need for "Maybe" on the function name and "SetRequireEulaForTesting".
// Just continue. The EULA is only used on Windows.... and Linux
*path = user_data_dir.AppendASCII("EulaSentinel");could we not use `installer::kEulaSentinelFile` for this?
class EulaDialog : public views::DialogDelegate {could/should this live in //chrome/browser/ui/views somewhere along with a browser test based on TestBrowserDialog? i'm not a ui/views OWNER, so i'm not the best person to review this.
// Ensure there exists a task runner for the loop.
std::optional<base::SingleThreadTaskExecutor> executor;
if (!base::SingleThreadTaskRunner::GetCurrentDefault()) {
executor.emplace(base::MessagePumpType::UI);
}why is this necessary? hasn't //content already created the UI thread's runner at this point in startup?
CHECK(base::DirectoryExists(sentinel.DirName()));not needed -- `PathService` guarantees that the directory exists.
base::PathService::OverrideAndCreateIfNeeded(chrome::DIR_USER_DATA,please use `base::ScopedPathOverride` from base/test/scoped_path_override.h
.AppendASCII("EulaSentinel");installer::kEulaSentinelFile?
EXPECT_FALSE(converted.empty());wdyt about checking in the expected text as a data file and confirming a match? i'm a bit worried that this test as-is could easily pass after a change to the ToS, yet the code could yield something that isn't comprehensible to the user as a ToS.
it also only tests en-us. is it possible that a bug in the converter could yield incorrect results for non-ascii input? it would be ideal to run through all translations somehow.
Related details
- Lei Zhang
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson voted and added 12 comments![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Auto-Submit | +1 |
12 comments
if (!headless::IsHeadlessMode() && !first_run::MaybeShowEulaDialog()) {Thomas Andersonis there any way we can do this before the metrics service is started (the EULA is shown during `PreEarlyInitialization` on Windows)? i think we need to take care to not record or report any metrics before the ToS are accepted.
Greg ThompsonYes, that is done in this CL because this EULA dialog will show **before** the first-run dialog with a checkbox for "Automatically send usage statistics and crash reports to Google".
I was referring to line 1620 above. Can that happen after the dialog is accepted or refused?
This might be an odd combination, but what happens if an install that requires the ToS is started on a machine that's enterprise-managed and has the "MetricsReportingEnabled" policy enabled? I think we still need to honor showing the ToS before collecting any metrics. Are you certain that not only reporting but also recording will be off at this point in that case?
In any case, I think we should have a comment here explaining what the guarantee is so that future refactors are less likely to accidentally move this later. Even better would be some tests for cases like this to prevent regressions.
resolved by moving this above StartMetricsRecording, and added a comment
long ago, i was asked to replace "Maybe" with something more explicit in functions like this. WDYT about `ShowEulaDialogIfRequired`, with the comment explaining the condition(s) under which it's required?
on second thought, see my comment in first_run.cc. i think we can avoid "Maybe" and "IfRequired" altogether.
Done
// or not required. Returns false if the EULA has not been accepted.maybe add to the comment that the process must exit promptly in this case? this is an important details for callers of this function.
Done
!internal::ShowPostInstallEULAIfNeeded(initial_prefs.get())) {rather than rely on setting a global in this fn that is later used to actually show the eula, i think it's more clean to have two eula-launching paths: windows will do it here as always. linux can continue to ignore the call to `ShowPostInstallEULAIfNeeded`. add a new `bool eula_required;` for linux in the `MasterPrefs` struct that is populated in `SetupInitialPrefsFromInstallPrefs` and then change `ChromeBrowserMainParts::PreMainMessageLoopRunImpl` to something like:
```
#if BUILDFLAG(IS_LINUX)
// On Linux, the EULA dialog requires Views, so it is shown here rather than
// when applying the first-run prefs.
if (first_run::IsChromeFirstRun() && master_prefs_->eula_required &&
!headless::IsHeadlessMode() !first_run::ShowEulaDialog()) {
return CHROME_RESULT_CODE_EULA_REFUSED;
}
#endif
```
this also removes the need for "Maybe" on the function name and "SetRequireEulaForTesting".
Done
// Just continue. The EULA is only used on Windows.Thomas Anderson... and Linux
Done
could we not use `installer::kEulaSentinelFile` for this?
Done
could/should this live in //chrome/browser/ui/views somewhere along with a browser test based on TestBrowserDialog? i'm not a ui/views OWNER, so i'm not the best person to review this.
Done
// Ensure there exists a task runner for the loop.
std::optional<base::SingleThreadTaskExecutor> executor;
if (!base::SingleThreadTaskRunner::GetCurrentDefault()) {
executor.emplace(base::MessagePumpType::UI);
}why is this necessary? hasn't //content already created the UI thread's runner at this point in startup?
removed
not needed -- `PathService` guarantees that the directory exists.
Done
base::PathService::OverrideAndCreateIfNeeded(chrome::DIR_USER_DATA,please use `base::ScopedPathOverride` from base/test/scoped_path_override.h
Done
.AppendASCII("EulaSentinel");Thomas Andersoninstaller::kEulaSentinelFile?
Done
EXPECT_FALSE(converted.empty());wdyt about checking in the expected text as a data file and confirming a match? i'm a bit worried that this test as-is could easily pass after a change to the ToS, yet the code could yield something that isn't comprehensible to the user as a ToS.
it also only tests en-us. is it possible that a bug in the converter could yield incorrect results for non-ascii input? it would be ideal to run through all translations somehow.
I'm wondering if attempting to parse the html is the wrong approach. wdyt about adding txt versions of the EULA alongside the html versions?
Related details
- Greg Thompson
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 4 comments![]( "Open in Gerrit")
#include <string>
#include <string_view>unused?
if (GetEulaSentinelFilePath(&sentinel) && base::PathExists(sentinel)) {if this returns false, we'll continue to show the eula below, but then we'll have an empty `sentinel` path when we reach line 75. i think we should early exit here if we can't determine the path (this means we don't have a usable user data directory).
EXPECT_FALSE(converted.empty());Thomas Andersonwdyt about checking in the expected text as a data file and confirming a match? i'm a bit worried that this test as-is could easily pass after a change to the ToS, yet the code could yield something that isn't comprehensible to the user as a ToS.
it also only tests en-us. is it possible that a bug in the converter could yield incorrect results for non-ascii input? it would be ideal to run through all translations somehow.
I'm wondering if attempting to parse the html is the wrong approach. wdyt about adding txt versions of the EULA alongside the html versions?
that would add a little bit of work whenever the ToS changes, but it would greatly simplify things here. i think that's a totally valid tradeoff. maybe check with benmason@ to see what they think?
"util_constants.h",doh! i didn't realize that this file wasn't already part of the linux build. now i'm rethinking my request that you use the existing constant since doing so pulls a lot of windows-specific stuff into the linux build.
i think it's better to not do this right now. my apologies. could you undo this part of the change, but make sure that the same actual name ("EULA Accepted") is used for the file on both platforms?
Related details
- Lei Zhang
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson voted and added 3 comments![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Auto-Submit | +1 |
3 comments
#include <string>
#include <string_view>Thomas Andersonunused?
Done
if (GetEulaSentinelFilePath(&sentinel) && base::PathExists(sentinel)) {if this returns false, we'll continue to show the eula below, but then we'll have an empty `sentinel` path when we reach line 75. i think we should early exit here if we can't determine the path (this means we don't have a usable user data directory).
Done
doh! i didn't realize that this file wasn't already part of the linux build. now i'm rethinking my request that you use the existing constant since doing so pulls a lot of windows-specific stuff into the linux build.
i think it's better to not do this right now. my apologies. could you undo this part of the change, but make sure that the same actual name ("EULA Accepted") is used for the file on both platforms?
- Greg Thompson
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Auto-Submit | +0 |
1 comment
EXPECT_FALSE(converted.empty());Thomas Andersonwdyt about checking in the expected text as a data file and confirming a match? i'm a bit worried that this test as-is could easily pass after a change to the ToS, yet the code could yield something that isn't comprehensible to the user as a ToS.
it also only tests en-us. is it possible that a bug in the converter could yield incorrect results for non-ascii input? it would be ideal to run through all translations somehow.
Greg ThompsonI'm wondering if attempting to parse the html is the wrong approach. wdyt about adding txt versions of the EULA alongside the html versions?
that would add a little bit of work whenever the ToS changes, but it would greatly simplify things here. i think that's a totally valid tradeoff. maybe check with benmason@ to see what they think?
- Ben Mason
- Greg Thompson
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 1 comment![]( "Open in Gerrit")
everything lgtm % the result of the html vs. text discussion. thanks.
Related details
- Ben Mason
- Lei Zhang
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson added 1 comment![]( "Open in Gerrit")
pinging benmason@
Related details
- Ben Mason
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 4 comments![]( "Open in Gerrit")
// This must match kEulaSentinelFile in chrome/installer/util/util_constants.ccUse IFTTT to help enforce this? `// LINT.IfChange(...)`
// Exposed for testing.1. Should have a ForTest(ing) suffix then?
2. Maybe make it a EulaDialog static method, instead of putting it in the global scope?
Maybe rename to eula_dialog_linux.h to make it obvious this is only used on Linux?
// This must match kEulaSentinelFile in chrome/installer/util/util_constants.ccI haven't used IFTTT enough to know if a 3 way check is possible.
Related details
- Ben Mason
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
new EulaDialog(std::move(callback)), nullptr, nullptr);What's the ownership model for this dialog? I don't see anyone deleting it.
constexpr const char kEulaSentinelFile[] = "EULA Accepted";`constexpr char` sufficient. More of this below.
Thomas Anderson (Gerrit)
Thomas Anderson voted and added 6 comments![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Commit-Queue | +1 |
6 comments
// This must match kEulaSentinelFile in chrome/installer/util/util_constants.ccUse IFTTT to help enforce this? `// LINT.IfChange(...)`
Done in chrome/installer/util/util_constants.cc
1. Should have a ForTest(ing) suffix then?
2. Maybe make it a EulaDialog static method, instead of putting it in the global scope?
Done
Maybe rename to eula_dialog_linux.h to make it obvious this is only used on Linux?
Done
What's the ownership model for this dialog? I don't see anyone deleting it.
Widgets are NATIVE_WIDGET_OWNS_WIDGET by default. So they are destroyed when the native window is destroyed.
// This must match kEulaSentinelFile in chrome/installer/util/util_constants.ccI haven't used IFTTT enough to know if a 3 way check is possible.
Done in chrome/installer/util/util_constants.cc
constexpr const char kEulaSentinelFile[] = "EULA Accepted";`constexpr char` sufficient. More of this below.
- Ben Mason
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 3 comments![]( "Open in Gerrit")
// This must match kEulaSentinelFile in chrome/installer/util/util_constants.ccThomas AndersonUse IFTTT to help enforce this? `// LINT.IfChange(...)`
Done in chrome/installer/util/util_constants.cc
I think the same comment needs to be here as well.
new EulaDialog(std::move(callback)), nullptr, nullptr);Thomas AndersonWhat's the ownership model for this dialog? I don't see anyone deleting it.
Widgets are NATIVE_WIDGET_OWNS_WIDGET by default. So they are destroyed when the native window is destroyed.
In ui/views/widget/widget.h, `NATIVE_WIDGET_OWNS_WIDGET` is marked as deprecated. I think this is done for backward compatibility purposes. The top of that header says "All widgets should use ownership = CLIENT_OWNS_WIDGET.".
textarea->SetText(EulaDialog::ConvertHtmlEulaToText(html_content));EulaDialog can call it without the EulaDialog:: prefix.
Related details
- Ben Mason
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson added 1 comment![]( "Open in Gerrit")
textarea->SetText(EulaDialog::ConvertHtmlEulaToText(html_content));EulaDialog can call it without the EulaDialog:: prefix.
Done
Related details
- Ben Mason
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 1 comment![]( "Open in Gerrit")
question from one who is not a ui/views OWNER:
should we have a browser test derived from TestBrowserDialog for this (see chrome/browser/ui/test/test_browser_dialog.h and chrome/browser/ui/test/test_browser_ui.h)? this not only ensures that there's no crash when the dialog is shown/dismissed, but also makes it easy for developers to see the dialog via the instructions here: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/test/test_browser_ui.h;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3;l=93.
Related details
- Ben Mason
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Commit-Queue | +1 |
2 comments
EXPECT_FALSE(converted.empty());Thomas Andersonwdyt about checking in the expected text as a data file and confirming a match? i'm a bit worried that this test as-is could easily pass after a change to the ToS, yet the code could yield something that isn't comprehensible to the user as a ToS.
it also only tests en-us. is it possible that a bug in the converter could yield incorrect results for non-ascii input? it would be ideal to run through all translations somehow.
Greg ThompsonI'm wondering if attempting to parse the html is the wrong approach. wdyt about adding txt versions of the EULA alongside the html versions?
Thomas Andersonthat would add a little bit of work whenever the ToS changes, but it would greatly simplify things here. i think that's a totally valid tradeoff. maybe check with benmason@ to see what they think?
+benmason@ for comment
I've added the converted txt files in the repo and removed ConvertHtmlEulaToText.
question from one who is not a ui/views OWNER:
should we have a browser test derived from TestBrowserDialog for this (see chrome/browser/ui/test/test_browser_dialog.h and chrome/browser/ui/test/test_browser_ui.h)? this not only ensures that there's no crash when the dialog is shown/dismissed, but also makes it easy for developers to see the dialog via the instructions here: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/test/test_browser_ui.h;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3;l=93.
- Ben Mason
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
<!-- The text for the about:terms page -->Maybe add this in a separate CL first?
<if expr="not is_android and not is_ios">Make this just Linux?
Related details
- Ben Mason
- Greg Thompson
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |