MD Settings: On Startup, extract dialog to its own file.

MD Settings: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while open.
 - Removing unnecessary usage of PromiseResolver.
 - Removing unused "prefs" property from <settings-startup-urls-page>.
 - Simplifying validation logic.
 - Adding missing tests.

This is in preparation of adding "edit startup page" functionality to the same
dialog.

BUG=601514
Committed: https://crrev.com/4c190f9618256c9175592abf6343834d96badb4e
Cr-Commit-Position: refs/heads/master@{#386269}

[dpapad, 2016-04-08 02:04:49]

Description was changed from

==========
MD Settigs: On Startup, extract dialog to its own file, WIP.

This is in preparation of adding "edit startup page" functionality to the
dialog.

BUG=601514
==========

to

==========
MD Settigs: On Startup, extract dialog to its own file, WIP.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Simplifying StartupUrlsBrowserProxy#validateStartupUrl

This is in preparation of adding "edit startup page" functionality to the
dialog.

BUG=601514
==========

[dpapad, 2016-04-08 02:06:53]

Description was changed from

==========
MD Settigs: On Startup, extract dialog to its own file, WIP.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Simplifying StartupUrlsBrowserProxy#validateStartupUrl

This is in preparation of adding "edit startup page" functionality to the
dialog.

BUG=601514
==========

to

==========
MD Settigs: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver from
   StartupUrlsPageBrowserProxy#validateStartupPage.

This is in preparation of adding "edit startup page" functionality to the
dialog.

BUG=601514
==========

[dpapad, 2016-04-08 17:20:16]

Description was changed from

==========
MD Settigs: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver from
   StartupUrlsPageBrowserProxy#validateStartupPage.

This is in preparation of adding "edit startup page" functionality to the
dialog.

BUG=601514
==========

to

==========
MD Settigs: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver from
   StartupUrlsPageBrowserProxy#validateStartupPage and starup_url_page_test.js.

This is in preparation of adding "edit startup page" functionality to the
dialog.

BUG=601514
==========

[dpapad, 2016-04-08 18:16:44]

Description was changed from

==========
MD Settigs: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver from
   StartupUrlsPageBrowserProxy#validateStartupPage and starup_url_page_test.js.

This is in preparation of adding "edit startup page" functionality to the
dialog.

BUG=601514
==========

to

==========
MD Settigs: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver.
 - Adding missing tests.

This is in preparation of adding "edit startup page" functionality to the same
dialog.

BUG=601514
==========

[dpapad, 2016-04-08 18:19:03]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-04-08 18:19:08]

Patchset #1 (id:20001) has been deleted

[dpapad, 2016-04-08 18:19:15]

Patchset #1 (id:40001) has been deleted

[dpapad, 2016-04-08 18:19:20]

Patchset #1 (id:60001) has been deleted

[dpapad, 2016-04-08 18:32:53]

Description was changed from

==========
MD Settigs: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver.
 - Adding missing tests.

This is in preparation of adding "edit startup page" functionality to the same
dialog.

BUG=601514
==========

to

==========
MD Settigs: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver.
 - Removing unused "prefs" property from <settings-startup-urls-page>.
 - Adding missing tests.

This is in preparation of adding "edit startup page" functionality to the same
dialog.

BUG=601514
==========

[dpapad, 2016-04-08 18:46:41]

dpapad@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-04-08 19:13:59]

how does your new code handle multiple validation Promises in-flight in
parallel?

https://codereview.chromium.org/1849543006/#msg11

[Dan Beam, 2016-04-08 19:20:04]

i'm not really sure why the resulting code you're proposing is better, +2 files,
+1 shadow DOM layer

https://codereview.chromium.org/1869883003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.html
(right):

https://codereview.chromium.org/1869883003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.html:38:
<template is="dom-if" if="[[showStartupUrlDialog_]]" restamp>
you don't really need to move this just to wrap it in a dom-if...

https://codereview.chromium.org/1869883003/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/on_startup_page/startup_urls_page_browser_proxy.js
(left):

https://codereview.chromium.org/1869883003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/on_startup_page/startup_urls_page_browser_proxy.js:49:
resolver.promise = url.trim().length == 0 ? Promise.resolve(false) :
I didn't really understand why this was bad at the time.

I'll send a CL that does something like this now:

if (url.trim().length == 0)
  resolver.resolve(false);
else
  cr.sendWithPromise('validateStartupPage', url).then(resolver.resolve);

[dpapad, 2016-04-08 19:32:21]

On 2016/04/08 at 19:13:59, dbeam wrote:
> how does your new code handle multiple validation Promises in-flight in
parallel?
> 
> https://codereview.chromium.org/1849543006/#msg11

I spent quite a bit of time thinking about this. My initial approach was to
purposefully not handle it, because I could not trigger the hypothetical race
condition. Also note that the same approach (of not handling) is followed at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....

If you do think that it needs to be handled (meaning that you do think that it
can actually be reproduced by a user), I am happy to address it, either by
restoring the previous behavior, or by a different approach I have in mind,
which I think is simpler and just as effective. Specifically.

  /** @private */
  onAddTap_: function() {
    // Extract the URL such that it is frozen (not bound to the UI anymore).
    var url = this.url_;
    // Re-run validation on that URL.
    this.browserProxy_.validateStartupPage(url).then(function(isValid) {
      if (isValid) {
        // Send the frozen URL, which guarantees that it is the one that got
        // validated to the browser.
        this.browserProxy_.addStartupPage(url);
      } else {
        // Nothing to do here, if it is invalid, the button becomes disabled
once the validation
        // triggered by the 'input' event returns.
      }
      
    });
  });
                                                                                
                                                                                
     
    this.$.dialog.close();
  },  

Will respond to the remaining comments shortly.

[dpapad, 2016-04-08 20:36:38]

On 2016/04/08 at 19:20:04, dbeam wrote:
> i'm not really sure why the resulting code you're proposing is better, +2
files, +1 shadow DOM layer
Here are my thoughts on why separate file is better.

1) Separation of concerns. By moving the dialog logic to its own custom element,
   there is a better IMO separation of concerns. The parent of the dialog, does
   not care at all about what the dialog is actually doing. The parent is
   concerned only about bringing the dialog up once the user requests it.
2) This makes it much easier to test various different codepaths in the dialog,
   since one can test the dialog in isolation (without the parent page).
3) By now, we have established a pattern of moving dialogs to their own files,
   for example
  passwords_and_forms_page/password_edit_dialog.js
  people_page/easy_unlock_turn_off_dialog.js
  search_engines_page/search_engine_dialog.js
  certificate_manager_page/certificate_password_decryption_dialog.js
  certificate_manager_page/certificates_error_dialog.js
  certificate_manager_page/certificate_delete_confirmation_dialog.js
  certificate_manager_page/ca_trust_edit_dialog.js
  certificate_manager_page/certificate_password_encryption_dialog.js
  reset_page/powerwash_dialog.js
  reset_page/reset_profile_dialog.js
  clear_browsing_data_dialog/clear_browsing_data_dialog.js

  In fact this dialog seems to be the only case where we don't follow that
  pattern so far.
4) This allows dom-ifing the dialog in an easier way than if it was not
   on its own file.
5) As mentioned in the CL description, this is in preparation of adding more
   logic to that dialog, to accomodate the case where an existing URL is edited.
   Creating a combined add/edit dialog is a problem I have already solved for
   search engines and the solution seems to be working pretty well.

[Dan Beam, 2016-04-08 21:17:15]

On 2016/04/08 20:36:38, dpapad wrote:
> On 2016/04/08 at 19:20:04, dbeam wrote:
> > i'm not really sure why the resulting code you're proposing is better, +2
> files, +1 shadow DOM layer
> Here are my thoughts on why separate file is better.
> 
> 1) Separation of concerns. By moving the dialog logic to its own custom
element,
>    there is a better IMO separation of concerns. The parent of the dialog,
does
>    not care at all about what the dialog is actually doing. The parent is
>    concerned only about bringing the dialog up once the user requests it.
> 2) This makes it much easier to test various different codepaths in the
dialog,
>    since one can test the dialog in isolation (without the parent page).
> 3) By now, we have established a pattern of moving dialogs to their own files,
>    for example
>   passwords_and_forms_page/password_edit_dialog.js
>   people_page/easy_unlock_turn_off_dialog.js
>   search_engines_page/search_engine_dialog.js
>   certificate_manager_page/certificate_password_decryption_dialog.js
>   certificate_manager_page/certificates_error_dialog.js
>   certificate_manager_page/certificate_delete_confirmation_dialog.js
>   certificate_manager_page/ca_trust_edit_dialog.js
>   certificate_manager_page/certificate_password_encryption_dialog.js
>   reset_page/powerwash_dialog.js
>   reset_page/reset_profile_dialog.js
>   clear_browsing_data_dialog/clear_browsing_data_dialog.js
> 
>   In fact this dialog seems to be the only case where we don't follow that
>   pattern so far.
> 4) This allows dom-ifing the dialog in an easier way than if it was not
>    on its own file.
> 5) As mentioned in the CL description, this is in preparation of adding more
>    logic to that dialog, to accomodate the case where an existing URL is
edited.
>    Creating a combined add/edit dialog is a problem I have already solved for
>    search engines and the solution seems to be working pretty well.

ok, sgtm

[dpapad, 2016-04-08 21:42:40]

Patchset #3 (id:120001) has been deleted

[dpapad, 2016-04-08 22:15:39]

Patchset #4 (id:160001) has been deleted

[dpapad, 2016-04-08 22:17:49]

Modified validation logic per offline discussion with @dbeam, and added
additional test to cover the behavior for the case where the user somehow
manages to click "Add" when the URL is invalid.

[dpapad, 2016-04-08 22:19:27]

Description was changed from

==========
MD Settigs: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver.
 - Removing unused "prefs" property from <settings-startup-urls-page>.
 - Adding missing tests.

This is in preparation of adding "edit startup page" functionality to the same
dialog.

BUG=601514
==========

to

==========
MD Settigs: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver.
 - Removing unused "prefs" property from <settings-startup-urls-page>.
 - Simplifying validation logic.
 - Adding missing tests.

This is in preparation of adding "edit startup page" functionality to the same
dialog.

BUG=601514
==========

[Dan Beam, 2016-04-08 23:24:43]

lgtm, awesome new tests!

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/resourc...
File chrome/browser/resources/settings/on_startup_page/startup_url_dialog.html
(right):

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/resourc...
chrome/browser/resources/settings/on_startup_page/startup_url_dialog.html:23:
<paper-button id="add" class="action-button" on-tap="onAddTap_">
i.e. disabled>

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/resourc...
File chrome/browser/resources/settings/on_startup_page/startup_url_dialog.js
(right):

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/resourc...
chrome/browser/resources/settings/on_startup_page/startup_url_dialog.js:23:
this.$.add.disabled = true;
why not just use the HTML instead?

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_resources.grd:107: flattenhtml="true"
flattenhtml="true" is only need for <if> or <include>, don't use it otherwise

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc (right):

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:84:
CHECK_EQ(2U, args->GetSize());
nit: \n

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:86:
CHECK(args->Get(0, &callback_id));
nit: \n

https://codereview.chromium.org/1869883003/diff/190001/chrome/test/data/webui...
File chrome/test/data/webui/settings/startup_urls_page_test.js (right):

https://codereview.chromium.org/1869883003/diff/190001/chrome/test/data/webui...
chrome/test/data/webui/settings/startup_urls_page_test.js:16:
'validateStartupPage',
nit: you may want to alphabetize the @interface as well, then

https://codereview.chromium.org/1869883003/diff/190001/chrome/test/data/webui...
chrome/test/data/webui/settings/startup_urls_page_test.js:27: setUrlValidity:
function(isValid) {
this isn't really functionally different than what I had before, just more code,
but if it makes you happy!

https://codereview.chromium.org/1869883003/diff/190001/chrome/test/data/webui...
chrome/test/data/webui/settings/startup_urls_page_test.js:65: function
triggerInputEvent(element) {
nit: pressSpace

[Dan Beam, 2016-04-08 23:43:36]

Description was changed from

==========
MD Settigs: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver.
 - Removing unused "prefs" property from <settings-startup-urls-page>.
 - Simplifying validation logic.
 - Adding missing tests.

This is in preparation of adding "edit startup page" functionality to the same
dialog.

BUG=601514
==========

to

==========
MD Settings: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver.
 - Removing unused "prefs" property from <settings-startup-urls-page>.
 - Simplifying validation logic.
 - Adding missing tests.

This is in preparation of adding "edit startup page" functionality to the same
dialog.

BUG=601514
==========

[dpapad, 2016-04-09 00:02:19]

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/resourc...
File chrome/browser/resources/settings/on_startup_page/startup_url_dialog.html
(right):

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/resourc...
chrome/browser/resources/settings/on_startup_page/startup_url_dialog.html:23:
<paper-button id="add" class="action-button" on-tap="onAddTap_">
On 2016/04/08 at 23:24:42, Dan Beam wrote:
> i.e. disabled>

Done.

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/resourc...
File chrome/browser/resources/settings/on_startup_page/startup_url_dialog.js
(right):

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/resourc...
chrome/browser/resources/settings/on_startup_page/startup_url_dialog.js:23:
this.$.add.disabled = true;
On 2016/04/08 at 23:24:42, Dan Beam wrote:
> why not just use the HTML instead?

Done.

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_resources.grd:107: flattenhtml="true"
On 2016/04/08 at 23:24:42, Dan Beam wrote:
> flattenhtml="true" is only need for <if> or <include>, don't use it otherwise

Done.

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc (right):

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:84:
CHECK_EQ(2U, args->GetSize());
On 2016/04/08 at 23:24:42, Dan Beam wrote:
> nit: \n

Done.

https://codereview.chromium.org/1869883003/diff/190001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:86:
CHECK(args->Get(0, &callback_id));
On 2016/04/08 at 23:24:42, Dan Beam wrote:
> nit: \n

Done.

https://codereview.chromium.org/1869883003/diff/190001/chrome/test/data/webui...
File chrome/test/data/webui/settings/startup_urls_page_test.js (right):

https://codereview.chromium.org/1869883003/diff/190001/chrome/test/data/webui...
chrome/test/data/webui/settings/startup_urls_page_test.js:16:
'validateStartupPage',
On 2016/04/08 at 23:24:42, Dan Beam wrote:
> nit: you may want to alphabetize the @interface as well, then

Done.

https://codereview.chromium.org/1869883003/diff/190001/chrome/test/data/webui...
chrome/test/data/webui/settings/startup_urls_page_test.js:27: setUrlValidity:
function(isValid) {
On 2016/04/08 at 23:24:42, Dan Beam wrote:
> this isn't really functionally different than what I had before, just more
code, but if it makes you happy!

Agreed, functionally is (almost) the same. Almost, because it was on the
prototype and now is in the constructor.
Regarding calling a setFoo() VS assigning a variable to public foo, I have a
very slight preference of calling the method over assigning the var, but only
very slightly.

https://codereview.chromium.org/1869883003/diff/190001/chrome/test/data/webui...
chrome/test/data/webui/settings/startup_urls_page_test.js:65: function
triggerInputEvent(element) {
On 2016/04/08 at 23:24:42, Dan Beam wrote:
> nit: pressSpace

If you are referring to
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/polyme...,
then it does not work for this case (tried it). The reason is that validation is
triggered on 'input' event, whereas pressSpace() triggers 'keyDown' and 'keyUp'.

[Dan Beam, 2016-04-09 00:03:40]

still lgtm

https://codereview.chromium.org/1869883003/diff/190001/chrome/test/data/webui...
File chrome/test/data/webui/settings/startup_urls_page_test.js (right):

https://codereview.chromium.org/1869883003/diff/190001/chrome/test/data/webui...
chrome/test/data/webui/settings/startup_urls_page_test.js:65: function
triggerInputEvent(element) {
On 2016/04/09 00:02:18, dpapad wrote:
> On 2016/04/08 at 23:24:42, Dan Beam wrote:
> > nit: pressSpace
> 
> If you are referring to
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/polyme...,
> then it does not work for this case (tried it). The reason is that validation
is
> triggered on 'input' event, whereas pressSpace() triggers 'keyDown' and
'keyUp'.

hah! no, I just meant "can we rename triggerInputEvent to pressSpace?".

[dpapad, 2016-04-09 00:19:57]

https://codereview.chromium.org/1869883003/diff/190001/chrome/test/data/webui...
File chrome/test/data/webui/settings/startup_urls_page_test.js (right):

https://codereview.chromium.org/1869883003/diff/190001/chrome/test/data/webui...
chrome/test/data/webui/settings/startup_urls_page_test.js:65: function
triggerInputEvent(element) {
On 2016/04/09 at 00:03:40, Dan Beam wrote:
> On 2016/04/09 00:02:18, dpapad wrote:
> > On 2016/04/08 at 23:24:42, Dan Beam wrote:
> > > nit: pressSpace
> > 
> > If you are referring to
> >
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/polyme...,
> > then it does not work for this case (tried it). The reason is that
validation is
> > triggered on 'input' event, whereas pressSpace() triggers 'keyDown' and
'keyUp'.
> 
> hah! no, I just meant "can we rename triggerInputEvent to pressSpace?".

Done.

[dpapad, 2016-04-09 00:21:19]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-04-09 00:21:19]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1869883003/#ps230001
(title: "Nit.")

[commit-bot: I haz the power, 2016-04-09 00:21:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1869883003/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1869883003/230001

[commit-bot: I haz the power, 2016-04-09 01:41:51]

Description was changed from

==========
MD Settings: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver.
 - Removing unused "prefs" property from <settings-startup-urls-page>.
 - Simplifying validation logic.
 - Adding missing tests.

This is in preparation of adding "edit startup page" functionality to the same
dialog.

BUG=601514
==========

to

==========
MD Settings: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver.
 - Removing unused "prefs" property from <settings-startup-urls-page>.
 - Simplifying validation logic.
 - Adding missing tests.

This is in preparation of adding "edit startup page" functionality to the same
dialog.

BUG=601514
==========

[commit-bot: I haz the power, 2016-04-09 01:41:52]

Committed patchset #7 (id:230001)

[commit-bot: I haz the power, 2016-04-09 01:43:11]

Description was changed from

==========
MD Settings: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver.
 - Removing unused "prefs" property from <settings-startup-urls-page>.
 - Simplifying validation logic.
 - Adding missing tests.

This is in preparation of adding "edit startup page" functionality to the same
dialog.

BUG=601514
==========

to

==========
MD Settings: On Startup, extract dialog to its own file.

Also:
 - Wrapping the dialog with dom-if such that it only exists in the DOM while
open.
 - Removing unnecessary usage of PromiseResolver.
 - Removing unused "prefs" property from <settings-startup-urls-page>.
 - Simplifying validation logic.
 - Adding missing tests.

This is in preparation of adding "edit startup page" functionality to the same
dialog.

BUG=601514

Committed: https://crrev.com/4c190f9618256c9175592abf6343834d96badb4e
Cr-Commit-Position: refs/heads/master@{#386269}
==========

[commit-bot: I haz the power, 2016-04-09 01:43:12]

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