Add about:welcome, add about:adblock, refactor about:payments WebUI #186

cezaraugusto · 2018-06-22T01:11:02Z

adds basic UI handler for welcome page without usable UI while design finish the new spec for the page

Test Plan:

npm run test -- brave_browser_tests --filter=BraveWelcomeUIBrowserTest.*

simonhong · 2018-06-22T01:41:35Z

browser/ui/webui/brave_web_ui_controller_factory.cc

      url.host_piece() ==  chrome::kChromeUINewTabHost) {
    return &NewWebUI<BasicUI>;
-  } else if (url.spec() == kWelcomeRemoteURL) {
+  } else if (url.spec() == kBraveUIWelcomeURL) {


I think this if clauses isn't used anymore because of above url.host_piece() == kWelcomeHost.
Does BraveWelcomeUIBrowserTest.StartupURLTest pass?

~~I assume you didn't try to browser test yet.~~
~~You need to replace kWelcomeRemoteURL with kBraveUIWelcomeURL in the test code.~~
Sorry, you already changed it.

To pass the test, logic in BraveWelcomeUI::BraveWelcomeUI() should be used.
How about make BraveWelcomeUI inherit BasicUI and use it for url.host_piece() == kWelcomeHost?

I tested this PR on my local machine.
First, BraveWelcomeUIBrowserTest was failed in PRE_StartupURLTest because actual url isn't same askBraveUIWelcomeURL.
The reason is reverse handler(HandleURLReverseRewrite) isn't working properly.
If reverse handler doesn't return true, virtual url is not updated with our ones.
Because of this, virtual url of tab at 0 index in test browser was different.
To fix this, kBraveUIWelcomeURL should be chrome://welcome/ not chrome://welcome. (url.spec() is chrome://welcome/). Or checking like above newtab seems better instead of comparing whole url.

When run test again, test will be failed in StartupURLTest because prefs::kHasSeenWelcomePage and prefs::kHasSeenWin10PromoPage isn't set properly.
If these prefs are not updated, browser always create welcome ui tab at startup.
It was handled in ctor of BraveWelcomeUI.
So, I recommend that making BraveWelcomeUI as a subclass of BasicUI and use it.

simonhong · 2018-06-24T23:25:05Z

Also you can revert this PR - #153.

bbondy · 2018-06-25T14:24:50Z

Re-added WIP label and title based on last review comments, please remove and re-ping once it is ready. Thank you!

simonhong

I think most of comments are trivial.
Feel free to merge after addressing nits.

simonhong · 2018-07-05T05:05:25Z

browser/ui/BUILD.gn

    "webui/brave_web_ui_controller_factory.cc",
    "webui/brave_web_ui_controller_factory.h",
    "webui/brave_welcome_ui.cc",
    "webui/brave_welcome_ui.h",
-    "webui/new_tab_html_source.cc",
-    "webui/new_tab_html_source.h",
+    "webui/brave_webui_source.cc",


simonhong · 2018-07-05T05:15:10Z

browser/ui/webui/brave_new_tab_ui.cc

+}
+
+BraveNewTabUI::~BraveNewTabUI() {
+  pref_change_registrar_.reset();


You don't need to call unique_ptr's reset() explicitly here.
dtor will destroy it implicitly and then it will destroy its managed object.

simonhong · 2018-07-05T05:21:06Z

browser/ui/webui/brave_new_tab_ui.h

+  void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
+  void OnPreferenceChanged();
+
+  std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;


If the lifetime of object member is same with this class, do we have to use |unique_ptr|?

per above, same as for reasons of forward declaring only. Will leave it.

simonhong · 2018-07-05T05:23:36Z

browser/ui/webui/brave_webui_source.cc

+
+#include "brave/browser/ui/webui/brave_webui_source.h"
+
+#include <string>


can be removed because header file did it already.

simonhong · 2018-07-05T05:28:52Z

browser/ui/webui/brave_webui_source.h

@@ -2,11 +2,11 @@
 * License, v. 2.0. If a copy of the MPL was not distributed with this file,
 * You can obtain one at http://mozilla.org/MPL/2.0/. */

+#include <string>


Header guard is omitted.

simonhong · 2018-07-05T05:39:45Z

browser/ui/webui/brave_welcome_ui.h

 public:
-  explicit BraveWelcomeUI(content::WebUI* web_ui);
+  explicit BraveWelcomeUI(content::WebUI* web_ui, const std::string& host);


explicit keyword can be removed

simonhong · 2018-07-05T05:42:48Z

browser/ui/webui/brave_rewards_ui.h

+
+class BraveRewardsUI : public BasicUI {
+ public:
+  explicit BraveRewardsUI(content::WebUI* web_ui, const std::string& host);


We don't need to use explicit keyword for ctor that has more than one parameter.

oops left over from copying from somewhere. Fixed.

@simonhong although not needed here I agree and removed, but just noting that it can still be used with multiple params: https://stackoverflow.com/a/39122237

Oh.. I didn't think about initializer-list. Thanks for checking this!

simonhong · 2018-07-05T05:45:23Z

browser/ui/webui/brave_adblock_ui.h

+
+class BraveAdblockUI : public BasicUI {
+ public:
+  explicit BraveAdblockUI(content::WebUI* web_ui, const std::string& host);


simonhong · 2018-07-05T06:00:33Z

browser/ui/webui/brave_adblock_ui.h

+  void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
+  void OnPreferenceChanged();
+
+  std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;


I'm going to keep this as std::unique_ptr because then it can be forward declared. I see a lot of instances in chromium source in both ways. I will remove the destructor explicit reset though which I agree isn't needed.

simonhong · 2018-07-05T06:01:49Z

browser/ui/webui/brave_adblock_ui.cc

 }

 BraveAdblockUI::~BraveAdblockUI() {
+  pref_change_registrar_.reset();


- adds basic UI handler for welcome page without usable UI while design finish the new spec for the page - address brave/brave-browser#276 test plan: npm run test -- brave_browser_tests --filter=BraveWelcomeUIBrowserTest.*

BasicUI had some things only specific to the new-tab in it. This change makes it so the custom things for NewTabUI are in the NewTab page and makes it easier for future things to subclass BasicUI

Partially implements brave/brave-browser#267

Related to: brave/brave-browser#315

Fix brave/brave-browser#485

Fix brave/brave-browser#267

simonhong · 2018-07-05T19:33:36Z

browser/ui/webui/brave_new_tab_ui.h

+  void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
+  void OnPreferenceChanged();
+
+  std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;


Adding provider to ledger::PublisherBanner

…ader Add loader to welcome page

Add loader to welcome page

…ader Add loader to welcome page

cezaraugusto self-assigned this Jun 22, 2018

cezaraugusto requested a review from bbondy June 22, 2018 01:11

simonhong reviewed Jun 22, 2018

View reviewed changes

bbondy changed the title ~~add about:welcome and webui handler~~ WIP: add about:welcome and webui handler Jun 25, 2018

bbondy added the pr/work-in-progress label Jun 25, 2018

bbondy force-pushed the welcome/webui branch 2 times, most recently from ea75eea to a6ff0ed Compare July 4, 2018 17:05

bbondy changed the title ~~WIP: add about:welcome and webui handler~~ Add about:welcome, add about:adblock, refactor about:payments WebUI Jul 4, 2018

bbondy force-pushed the welcome/webui branch 2 times, most recently from f295afa to 071e7e3 Compare July 5, 2018 01:52

bbondy self-assigned this Jul 5, 2018

bbondy removed the pr/work-in-progress label Jul 5, 2018

simonhong previously approved these changes Jul 5, 2018

View reviewed changes

bbondy dismissed simonhong’s stale review via 102d447 July 5, 2018 13:32

bbondy force-pushed the welcome/webui branch from 071e7e3 to 102d447 Compare July 5, 2018 13:32

cezaraugusto and others added 9 commits July 5, 2018 10:15

add about:welcome and webui handler

76c6f39

- adds basic UI handler for welcome page without usable UI while design finish the new spec for the page - address brave/brave-browser#276 test plan: npm run test -- brave_browser_tests --filter=BraveWelcomeUIBrowserTest.*

Refactor new-tab WebUI

c860958

BasicUI had some things only specific to the new-tab in it. This change makes it so the custom things for NewTabUI are in the NewTab page and makes it easier for future things to subclass BasicUI

Subclass BasicUI for chrome://welcome page

76527c7

Refactor and rename Brave Payments

504adc4

Add chrome://adblock page

7d47f65

Add dep to grd to fix header changes not updating

038c4b6

Partially implements brave/brave-browser#267

Remove IsWebUIAllowedToMakeNetworkRequests exception for brave.com

8583bdb

Related to: brave/brave-browser#315

Add test to make sure custom WebUI pages load

59927ea

Harden regional ad block language matching

3fa9eb3

Fix brave/brave-browser#485

bbondy force-pushed the welcome/webui branch from 102d447 to c6ab8b9 Compare July 5, 2018 14:15

Setup about:adblock redux and populate with some basic data

7d27486

Fix brave/brave-browser#267

bbondy force-pushed the welcome/webui branch from c6ab8b9 to 7d27486 Compare July 5, 2018 18:56

simonhong approved these changes Jul 5, 2018

View reviewed changes

bbondy merged commit 4f6afa8 into master Jul 5, 2018

cezaraugusto deleted the welcome/webui branch July 9, 2018 13:47

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

Merge pull request #186 from ryanml/banner-provider

b4fb7f9

Adding provider to ledger::PublisherBanner

NejcZdovc pushed a commit that referenced this pull request Dec 11, 2018

Merge pull request #186 from ryanml/banner-provider

27f4c8b

Adding provider to ledger::PublisherBanner

cezaraugusto pushed a commit that referenced this pull request Jul 17, 2019

Imported from brave-ui: Merge pull request #186 from brave/welcome-lo…

2e3e379

…ader Add loader to welcome page

NejcZdovc added a commit that referenced this pull request Sep 18, 2019

Merge pull request #186 from brave/welcome-loader

c6ca5be

Add loader to welcome page

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

Imported from brave-ui: Merge pull request #186 from brave/welcome-lo…

05022c0

…ader Add loader to welcome page

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add about:welcome, add about:adblock, refactor about:payments WebUI #186

Add about:welcome, add about:adblock, refactor about:payments WebUI #186

cezaraugusto commented Jun 22, 2018 •

edited by bbondy

simonhong Jun 22, 2018

simonhong Jun 22, 2018 •

edited

simonhong Jun 23, 2018

simonhong commented Jun 24, 2018

bbondy commented Jun 25, 2018

simonhong left a comment

simonhong Jul 5, 2018

bbondy Jul 5, 2018

simonhong Jul 5, 2018

bbondy Jul 5, 2018 •

edited

simonhong Jul 5, 2018

bbondy Jul 5, 2018

simonhong Jul 5, 2018

simonhong Jul 5, 2018

bbondy Jul 5, 2018 •

edited

simonhong Jul 5, 2018

bbondy Jul 5, 2018

simonhong Jul 5, 2018

bbondy Jul 5, 2018

simonhong Jul 5, 2018

bbondy Jul 5, 2018

bbondy Jul 5, 2018

simonhong Jul 5, 2018

simonhong Jul 5, 2018

simonhong Jul 5, 2018

bbondy Jul 5, 2018

simonhong Jul 5, 2018

simonhong Jul 5, 2018

simonhong Jul 5, 2018


		#include "brave/browser/ui/webui/brave_webui_source.h"

		#include <string>

Add about:welcome, add about:adblock, refactor about:payments WebUI #186

Add about:welcome, add about:adblock, refactor about:payments WebUI #186

Conversation

cezaraugusto commented Jun 22, 2018 • edited by bbondy

Choose a reason for hiding this comment

simonhong Jun 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Jun 24, 2018

bbondy commented Jun 25, 2018

simonhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy Jul 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy Jul 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezaraugusto commented Jun 22, 2018 •

edited by bbondy

simonhong Jun 22, 2018 •

edited

bbondy Jul 5, 2018 •

edited

bbondy Jul 5, 2018 •

edited