Adding JS and C++ handlers for events on new Welcome page.

Adding JS and C++ handlers for events on new Welcome page.

BUG=618454
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/7f0818b0f710e64cb3fa7fab222850013a5438b0
Cr-Commit-Position: refs/heads/master@{#421385}

[tmartino, 2016-09-16 15:07:54]

Description was changed from

==========
Adding JS and C++ handlers for events on new Welcome page.

BUG=618454
==========

to

==========
Adding JS and C++ handlers for events on new Welcome page.

BUG=618454
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tmartino, 2016-09-16 15:12:00]

tmartino@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[anthonyvd, 2016-09-19 17:12:16]

Sorry this took a few days, here are some initial comments!

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/resources/we...
File chrome/browser/resources/welcome/welcome.html (right):

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/resources/we...
chrome/browser/resources/welcome/welcome.html:36: <paper-button class="action"
id="accept_button">$i18n{acceptText}</paper-button>
nit: I *think* you should have dashes instead of underscores for id values.

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/resources/we...
File chrome/browser/resources/welcome/welcome.js (right):

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/resources/we...
chrome/browser/resources/welcome/welcome.js:5:
document.addEventListener('DOMContentLoaded', function() {
You can take a look at
https://cs.chromium.org/chromium/src/chrome/browser/resources/signin/sync_con...
for the canonical way of doing js setup (using cr.define and all that jazz).

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
File chrome/browser/ui/webui/welcome_ui.cc (right):

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_ui.cc:39: const char* kEverywhereVariantPath =
"/1";
Why not a descriptive GET style param?

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_ui.cc:41: const char* kHandleActivateSignIn =
"handleActivateSignIn";
Message name strings tend to be just inlined when registered so you don't need
to define them as constants.

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_ui.cc:44: class WelcomeHandler : public
content::WebUIMessageHandler,
Your handler should be in a separate file for consistency with other handlers.

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_ui.cc:48: profile_ = Profile::FromWebUI(web_ui);
Prefer initializer lists over initializing in the ctor body.

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_ui.cc:51: if (oauth2_token_service_)
The Service Factories initialize services in GetForProfile if they don't exist
already so that if() is likely superfluous.

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_ui.cc:68: if (user_has_clicked_signin_) {
What happens if the user has this page up and signs in through the User Menu?
Shouldn't this always go to the NTP when the user signs in regardless of source?

[tmartino, 2016-09-19 22:37:05]

Thanks! Fixed all the comments except where I've responded.

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
File chrome/browser/ui/webui/welcome_ui.cc (right):

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_ui.cc:39: const char* kEverywhereVariantPath =
"/1";
On 2016/09/19 at 17:12:16, anthonyvd wrote:
> Why not a descriptive GET style param?

If you don't mind me asking, what would the gain be? This way I can do a simple
equality check. Using a GET param would involve parsing (to separate out the
key/value/delimiter) which seems to add a lot of complexity for little benefit
when we only have one variant that's either on or off.

I suppose we could do a literal string check against the whole parameter string
(e.g., if (url.query() == "variant=everywhere")) but then it's logically
equivalent to this anyway.

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_ui.cc:68: if (user_has_clicked_signin_) {
On 2016/09/19 at 17:12:15, anthonyvd wrote:
> What happens if the user has this page up and signs in through the User Menu?
Shouldn't this always go to the NTP when the user signs in regardless of source?

I was torn on this. Either way it's kind of jarring for the user. Either:

(1) Page navigation happens unexpectedly, or
(2) The page doesn't disappear, but the "sign in" button acts as a no-op
navigation button

I opted for the latter. WDYT?

[anthonyvd, 2016-09-21 16:17:56]

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
File chrome/browser/ui/webui/welcome_ui.cc (right):

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_ui.cc:39: const char* kEverywhereVariantPath =
"/1";
On 2016/09/19 22:37:04, tmartino wrote:
> On 2016/09/19 at 17:12:16, anthonyvd wrote:
> > Why not a descriptive GET style param?
> 
> If you don't mind me asking, what would the gain be? This way I can do a
simple
> equality check. Using a GET param would involve parsing (to separate out the
> key/value/delimiter) which seems to add a lot of complexity for little benefit
> when we only have one variant that's either on or off.
> 
> I suppose we could do a literal string check against the whole parameter
string
> (e.g., if (url.query() == "variant=everywhere")) but then it's logically
> equivalent to this anyway.

It's just more self-documenting that way. You can also use
net::GetValueForKeyInQuery for easy parsing. Something like:

std::string value;
if (net::GetValueForKeyInQuery(url, "take_everywhere", &value) && value == "1")
{
  // Show the "Take Everywhere" variant
}

alternately, if we expect to have more variants at some point:

std::string value;
if (net::GetValueForKeyInQuery(url, "variant", &value) && value ==
"take_everywhere") {
  // Show the "Take Everywhere" variant
}

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_ui.cc:68: if (user_has_clicked_signin_) {
On 2016/09/19 22:37:04, tmartino wrote:
> On 2016/09/19 at 17:12:15, anthonyvd wrote:
> > What happens if the user has this page up and signs in through the User
Menu?
> Shouldn't this always go to the NTP when the user signs in regardless of
source?
> 
> I was torn on this. Either way it's kind of jarring for the user. Either:
> 
> (1) Page navigation happens unexpectedly, or
> (2) The page doesn't disappear, but the "sign in" button acts as a no-op
> navigation button
> 
> I opted for the latter. WDYT?

Good point, I personally prefer (1) but I could see it either way. That's
something Design/PM should be able to settle.

[tmartino, 2016-09-22 20:32:09]

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
File chrome/browser/ui/webui/welcome_ui.cc (right):

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_ui.cc:39: const char* kEverywhereVariantPath =
"/1";
On 2016/09/21 at 16:17:56, anthonyvd wrote:
> On 2016/09/19 22:37:04, tmartino wrote:
> > On 2016/09/19 at 17:12:16, anthonyvd wrote:
> > > Why not a descriptive GET style param?
> > 
> > If you don't mind me asking, what would the gain be? This way I can do a
simple
> > equality check. Using a GET param would involve parsing (to separate out the
> > key/value/delimiter) which seems to add a lot of complexity for little
benefit
> > when we only have one variant that's either on or off.
> > 
> > I suppose we could do a literal string check against the whole parameter
string
> > (e.g., if (url.query() == "variant=everywhere")) but then it's logically
> > equivalent to this anyway.
> 
> It's just more self-documenting that way. You can also use
net::GetValueForKeyInQuery for easy parsing. Something like:
> 
> std::string value;
> if (net::GetValueForKeyInQuery(url, "take_everywhere", &value) && value ==
"1") {
>   // Show the "Take Everywhere" variant
> }
> 
> alternately, if we expect to have more variants at some point:
> 
> std::string value;
> if (net::GetValueForKeyInQuery(url, "variant", &value) && value ==
"take_everywhere") {
>   // Show the "Take Everywhere" variant
> }

Opted for the latter.

https://codereview.chromium.org/2338213007/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_ui.cc:68: if (user_has_clicked_signin_) {
On 2016/09/21 at 16:17:56, anthonyvd wrote:
> On 2016/09/19 22:37:04, tmartino wrote:
> > On 2016/09/19 at 17:12:15, anthonyvd wrote:
> > > What happens if the user has this page up and signs in through the User
Menu?
> > Shouldn't this always go to the NTP when the user signs in regardless of
source?
> > 
> > I was torn on this. Either way it's kind of jarring for the user. Either:
> > 
> > (1) Page navigation happens unexpectedly, or
> > (2) The page doesn't disappear, but the "sign in" button acts as a no-op
> > navigation button
> > 
> > I opted for the latter. WDYT?
> 
> Good point, I personally prefer (1) but I could see it either way. That's
something Design/PM should be able to settle.

Brought this up and the consensus was to go with (1). Changed.

[anthonyvd, 2016-09-23 16:55:52]

lgtm, you should be good to loop in owners :)

[tmartino, 2016-09-23 17:04:48]

tmartino@chromium.org changed reviewers:
+ michaelpg@chromium.org

[tmartino, 2016-09-23 17:04:49]

+michaelpg for OWNERS. This is the dramatic conclusion of the UI from the last
CL you reviewed for me.

[michaelpg, 2016-09-23 18:46:36]

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/resource...
File chrome/browser/resources/welcome/welcome.html (right):

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:13: <script
src="chrome://resources/js/cr.js"></script>
I suggest changing these to <link rel="import" ...> for the equivalent HTML
files. HTML imports make dependency management a lot easier, prevent duplicate
imports, etc.

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:14: <script
src="chrome://resources/js/load_time_data.js"></script>
unused?

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/resource...
File chrome/browser/resources/welcome/welcome.js (right):

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.js:25: 
nit: remove extra blank line

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/welcome_handler.cc (right):

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.cc:23:
oauth2_token_service_->AddObserver(this);
According to its comments, this service will be nullptr in incognito mode.
Should we prevent this UI from opening in incognito contexts (similar to how
chrome://settings in an Incognito window opens it in a new on-the-record tab)?

What about Guest mode? (top-right profile icon => switch person => browse as
guest)

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/welcome_handler.h (right):

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.h:8: #include
"chrome/browser/profiles/profile.h"
forward declare?

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.h:9: #include
"chrome/browser/ui/browser.h"
forward declare?

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.h:10: #include
"components/signin/core/browser/profile_oauth2_token_service.h"
forward declare?

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.h:14: enum WelcomeResult {
Can we scope this? e.g., inside WelcomeHandler

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.h:26: void OnRefreshTokenAvailable(const
std::string& account_id) override;
indicate which type's functions this overrides, in a separate
(blank-line-separated) section

// OAuth2TokenService::Observer:

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.h:27: void HandleActivateSignIn(const
base::ListValue* args);
private

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.h:28: void HandleUserDecline(const
base::ListValue* args);
private

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.h:29: void RegisterMessages() override;
// content::WebUIMessageHandler:

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.h:30: void GoToNewTabPage();
private

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.h:37: };
consider adding DISALLOW_COPY_AND_ASSIGN

https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/disall...

[tmartino, 2016-09-26 23:10:15]

Thanks, Michael! Acted on all feedback except the forward declarations,
commented below.

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/welcome_handler.cc (right):

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.cc:23:
oauth2_token_service_->AddObserver(this);
On 2016/09/23 at 18:46:35, michaelpg wrote:
> According to its comments, this service will be nullptr in incognito mode.
Should we prevent this UI from opening in incognito contexts (similar to how
chrome://settings in an Incognito window opens it in a new on-the-record tab)?
> 
> What about Guest mode? (top-right profile icon => switch person => browse as
guest)

Done. Added an early exit to welcome_ui.cc constructor. The result is simply
showing a "This page can't be reached" page.

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/welcome_handler.h (right):

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.h:8: #include
"chrome/browser/profiles/profile.h"
On 2016/09/23 at 18:46:36, michaelpg wrote:
> forward declare?

Forward Declarations are discouraged:
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[tmartino, 2016-09-27 15:43:24]

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/welcome_handler.h (right):

https://codereview.chromium.org/2338213007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_handler.h:8: #include
"chrome/browser/profiles/profile.h"
On 2016/09/26 at 23:10:15, tmartino wrote:
> On 2016/09/23 at 18:46:36, michaelpg wrote:
> > forward declare?
> 
> Forward Declarations are discouraged:
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Whoops, I just saw that Chromium style contradicts Google C++ on this point.
Changed.

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[michaelpg, 2016-09-27 21:14:12]

lgtm

https://codereview.chromium.org/2338213007/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/welcome_ui.cc (right):

https://codereview.chromium.org/2338213007/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.cc:23: // here, we return, causing a 404-like
page.
Cool, didn't know that. It doesn't trip a DCHECK or anything in Debug builds?

[tmartino, 2016-09-27 22:26:53]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 22:27:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tmartino, 2016-09-27 22:27:27]

On 2016/09/27 at 21:14:12, michaelpg wrote:
> lgtm
> 
>
https://codereview.chromium.org/2338213007/diff/100001/chrome/browser/ui/webu...
> File chrome/browser/ui/webui/welcome_ui.cc (right):
> 
>
https://codereview.chromium.org/2338213007/diff/100001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/welcome_ui.cc:23: // here, we return, causing a
404-like page.
> Cool, didn't know that. It doesn't trip a DCHECK or anything in Debug builds?

Nope! Logging is quiet as well.

[commit-bot: I haz the power, 2016-09-27 23:33:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 23:33:07]

Dry run: This issue passed the CQ dry run.

[tmartino, 2016-09-27 23:33:44]

The CQ bit was checked by tmartino@chromium.org

[tmartino, 2016-09-27 23:33:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2338213007/#ps100001
(title: "Forward Declarations")

[commit-bot: I haz the power, 2016-09-27 23:34:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 23:39:39]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-27 23:41:35]

Description was changed from

==========
Adding JS and C++ handlers for events on new Welcome page.

BUG=618454
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Adding JS and C++ handlers for events on new Welcome page.

BUG=618454
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/7f0818b0f710e64cb3fa7fab222850013a5438b0
Cr-Commit-Position: refs/heads/master@{#421385}
==========

[commit-bot: I haz the power, 2016-09-27 23:41:36]

Patchset 6 (id:??) landed as
https://crrev.com/7f0818b0f710e64cb3fa7fab222850013a5438b0
Cr-Commit-Position: refs/heads/master@{#421385}