Show welcome page only at first run #130

simonhong · 2018-06-04T15:21:39Z

When welcome page is requested at the first run, store that this profile
has been shown the welcome page.

close brave/brave-browser#221
close brave/brave-browser#264

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).
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.

Test Plan:

--filter BraveWelcomeUIBrowserTest*

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions

simonhong · 2018-06-04T15:38:37Z

To add test case for this, brave/brave-browser#264 should be resolved first.

bbondy · 2018-06-04T16:29:49Z

Just curious if you know why code to remember no welcome page is needed, I'd think this would already be handled by chromium code somewhere.

You probably already know but you can test spanning restarts of the app with:

Tests Spanning Restarts
A test can span a restart of the browser process. This is useful in testing that something persisted, as an example. To do this, use the PRE prefix:

IN_PROC_BROWSER_TEST_F(InProcessBrowserTest, PRE_Foo) {
  // Do something.
}

IN_PROC_BROWSER_TEST_F(InProcessBrowserTest, Foo) {
  // Verify something was done.
}

from: https://www.chromium.org/developers/testing/running-tests

simonhong · 2018-06-05T00:26:15Z

Just curious if you know why code to remember no welcome page is needed, I'd think this would already be handled by chromium code somewhere.

You're right. chromium handles it in each webui controller(WelcomeUI and WelcomeWin10UI) for their URL. After we changed the chromium url to our welcome url from HandleURLRewrite() in our url handler(brave_content_browser_client.cc), that code has not been executed.
Because of that, I created our webui controller for kWelcomeRemoteURL.

You probably already know but you can test spanning restarts of the app with:

Yep, I tried to check the tab list state between first run and second run by using spanning test technique. I realized that our browser test should use BraveMainDelegate to test BraveContentBrowserClient features.

bbondy · 2018-06-05T01:39:30Z

That makes sense, thanks.

bbondy · 2018-06-05T02:14:20Z

browser/ui/webui/brave_welcome_ui.cc

+  profile->GetPrefs()->SetBoolean(prefs::kHasSeenWelcomePage, true);
+#if defined(OS_WIN)
+  g_brave_browser_process->local_state()->SetBoolean(
+      prefs::kHasSeenWin10PromoPage,


Do we need to set this pref even know we don't want a win10 promo page?

I checked that chrome shows this promo page at first run. Then, it shows welcome page at next run.
So, if we doesn't set, we will show our welcome ui at first run and second run.

bbondy · 2018-06-05T02:18:49Z

ok thanks, looks good. Looks like you need to rebase test/BUILD.gn, then feel free to merge.

simonhong · 2018-06-05T02:19:47Z

Thanks for fast review.
I'll merge after rebasing this on C67.

When welcome page is requested at the first run, store that this profile has been shown the welcome page.

So far, ChromeMainDelegate is used.

welcome ui only should be added at first run.

simonhong · 2018-06-05T09:50:50Z

@bbondy After rebasing on C68, your approving was disappeared :)

simonhong · 2018-06-05T11:54:04Z

@emerick , PTAL again. I think @bbondy couldn't check for some time(travelling).
I got approved but it disappeared when this is rebased on C67.

emerick

LGTM

simonhong · 2018-06-05T11:58:52Z

@emerick Thank you!

bridiver · 2018-06-05T15:11:12Z

browser/ui/webui/brave_web_ui_controller_factory.cc

@@ -47,6 +53,8 @@ WebUIFactoryFunction GetWebUIFactoryFunction(WebUI* web_ui,
  if (url.host_piece() == kPaymentsHost ||
      url.host_piece() ==  chrome::kChromeUINewTabHost) {
    return &NewWebUI<BasicUI>;
+  } else if (url.spec() == kWelcomeRemoteURL) {


@diracdeltas @bbondy - this isn't directly related to this PR, but I think a remote welcome url is a privacy issue. Is there any reason why it can't be an internal page?

We discussed changing to local welcome page but no one has had time to do that yet. Note that browser-laptop is remote too so we have at least baseline parity with that. But we did talk about wanting to move to a local one.

seems like it should be pretty easy to do now that we have a webui for it. Is there a ticket or should I create one?

I thought there was but I couldn't find one, so I posted it here:
brave/brave-browser#276

here's the original issue by @flamsmark brave/browser-laptop#12691. i also feel strongly that this should be baked into brave for privacy reasons.

This seems the regression from #130. In that CL, BraveWelcomeUI WebUIController is introduced. Because of this, current welcome ui page treated as webui. This CL could be reverted when our local welcome ui is implemented.

Favicon URL for youtube publishers should now be retrieved properly.

…stays white

simonhong force-pushed the fix_welcome_ui branch from d6d86fe to d64853c Compare June 5, 2018 01:43

simonhong requested a review from bbondy June 5, 2018 02:11

simonhong changed the title ~~WIP: Show welcome page only at first run~~ Show welcome page only at first run Jun 5, 2018

bbondy reviewed Jun 5, 2018

View reviewed changes

simonhong force-pushed the fix_welcome_ui branch from fd153e3 to 777dba9 Compare June 5, 2018 02:14

bbondy previously approved these changes Jun 5, 2018

View reviewed changes

simonhong added 3 commits June 5, 2018 18:24

Show welcome page only at first run

d2e3b5a

When welcome page is requested at the first run, store that this profile has been shown the welcome page.

Use BraveMainDelegate for brave_browser_tests

6ed2120

So far, ChromeMainDelegate is used.

Add test case for startup url at first run

36faccf

welcome ui only should be added at first run.

simonhong dismissed bbondy’s stale review via 36faccf June 5, 2018 09:40

simonhong force-pushed the fix_welcome_ui branch from 777dba9 to 36faccf Compare June 5, 2018 09:40

simonhong self-assigned this Jun 5, 2018

simonhong requested a review from emerick June 5, 2018 11:53

emerick approved these changes Jun 5, 2018

View reviewed changes

simonhong merged commit 1dc4273 into master Jun 5, 2018

simonhong deleted the fix_welcome_ui branch June 5, 2018 11:59

bridiver reviewed Jun 5, 2018

View reviewed changes

srirambv mentioned this pull request Jun 5, 2018

No welcome page shown for offline installation brave/browser-laptop#10523

Closed

simonhong mentioned this pull request Jun 11, 2018

Crash from welcome ui when first run on debug build brave/brave-browser#315

Closed

simonhong mentioned this pull request Jun 11, 2018

Fix first run startup crash due to welcome ui #153

Merged

9 tasks

NejcZdovc added a commit that referenced this pull request Dec 10, 2018

Merge pull request #130 from brave-intl/youtube_favicon

fa06a4b

Favicon URL for youtube publishers should now be retrieved properly.

NejcZdovc pushed a commit that referenced this pull request Sep 19, 2019

Rewards: Imported from brave-ui: Fixes #130, recovery key background …

e53120e

…stays white

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show welcome page only at first run #130

Show welcome page only at first run #130

simonhong commented Jun 4, 2018 •

edited

Loading

simonhong commented Jun 4, 2018 •

edited

Loading

bbondy commented Jun 4, 2018

simonhong commented Jun 5, 2018

bbondy commented Jun 5, 2018

bbondy Jun 5, 2018

simonhong Jun 5, 2018

bbondy commented Jun 5, 2018

simonhong commented Jun 5, 2018

simonhong commented Jun 5, 2018

simonhong commented Jun 5, 2018 •

edited

Loading

emerick left a comment

simonhong commented Jun 5, 2018

bridiver Jun 5, 2018

bbondy Jun 5, 2018

bridiver Jun 5, 2018 •

edited

Loading

bbondy Jun 5, 2018

bridiver Jun 5, 2018

diracdeltas Jun 5, 2018

Show welcome page only at first run #130

Show welcome page only at first run #130

Conversation

simonhong commented Jun 4, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

simonhong commented Jun 4, 2018 • edited Loading

bbondy commented Jun 4, 2018

simonhong commented Jun 5, 2018

bbondy commented Jun 5, 2018

bbondy Jun 5, 2018

Choose a reason for hiding this comment

simonhong Jun 5, 2018

Choose a reason for hiding this comment

bbondy commented Jun 5, 2018

simonhong commented Jun 5, 2018

simonhong commented Jun 5, 2018

simonhong commented Jun 5, 2018 • edited Loading

emerick left a comment

Choose a reason for hiding this comment

simonhong commented Jun 5, 2018

bridiver Jun 5, 2018

Choose a reason for hiding this comment

bbondy Jun 5, 2018

Choose a reason for hiding this comment

bridiver Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

bbondy Jun 5, 2018

Choose a reason for hiding this comment

bridiver Jun 5, 2018

Choose a reason for hiding this comment

diracdeltas Jun 5, 2018

Choose a reason for hiding this comment

simonhong commented Jun 4, 2018 •

edited

Loading

simonhong commented Jun 4, 2018 •

edited

Loading

simonhong commented Jun 5, 2018 •

edited

Loading

bridiver Jun 5, 2018 •

edited

Loading