Settings People Revamp: Implement parts of ChromeOS Change Picture

Settings People Revamp: Implement parts of ChromeOS Change Picture

Implements:
 1. Selecting from default images.
 2. Selecting the 'old' image (from a file, old camera shot, deprecated default image).
 3. Selecting the Google profile photo.

Missing from this implmentation:
 1. Larger preview image on the side.
 2. Ability to take new pictures with the camera, and flip it.
 3. Ability to select an image from the file.
 4. Lots of polish.

BUG=563721
Committed: https://crrev.com/1d4f8c7571693dfc346a2d3f12beda2481a291ea
Cr-Commit-Position: refs/heads/master@{#369308}

[tommycli, 2016-01-09 00:58:09]

Description was changed from

==========
Settings People Revamp: Implement ChromeOS Change Picture

BUG=563721
==========

to

==========
Settings People Revamp: Implement parts of ChromeOS Change Picture

Implements:
 1. Selecting from default images.
 2. Selecting the 'old' image (from a file, old camera shot, deprecated default
image).
 3. Selecting the Google profile photo.

Missing from this implmentation:
 1. Larger preview image on the side.
 2. Ability to take new pictures with the camera, and flip it.
 3. Ability to select an image from the file.
 4. Lots of polish.

BUG=563721
==========

[tommycli, 2016-01-11 18:33:01]

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

[tommycli, 2016-01-11 18:33:02]

dpapad: PTAL, thanks!

[dpapad, 2016-01-11 19:25:50]

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.html (right):

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.html:30:
on-tap="onDefaultImageTap_" data-image-url$="[[item.url]]">
data-image-url$="[[item.url]]

Have not seen this syntax before. Is the "$" a typo, or intentional?

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.js:44: * The
default user image images. Populated by ChangePicturePrivateApi.
Maybe change to "The default user images"?

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.js:115: var
imageUrl;
Nit: Avoid implicit boolean conversions when possible.

var imageUrl = null;
...
...

if (imageUrl != null) {
 ...
}

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.js:116: if
(element.nodeName == 'IMG')
Should we change this to
if (element.nodeName.toUpperCase() == 'IMG')

to make it a bit more robust?

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.js:153: * @param
{!string} imageUrl
No need to put "!" before "string". It is implied, see
https://developers.google.com/closure/compiler/docs/js-for-compiler?hl=en#non...,
specifically this sentence

"Functions and all value types (boolean, number, and string) are non-nullable by
default whether or not they are declared with the Non-nullable operator."

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture_private_api.js
(right):

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture_private_api.js:24:
function ChangePicturePrivateApi() {}
How about instantiating an object from this class, instead of making everything
static? I think this would make the code less verbose and more readable. For
example

/**
 * @constructor
 * @param ...
 * @param ...
 */
function ChangePicturePrivateApi(
    getDefaultImagesCallback,
    getSelectedImageUrlCallback,
    getOldImageUrlCallback,
    getProfileImageUrlCallback) {
  this.getDefaultImagesCallback_ = getDefaultImagesCallback;
  this.getSelectedImageUrlCallback_ = getSelectedImageUrlCallback;
  this.getOldImageUrlCallback_ = getOldImageUrlCallback;
  this.getProfileImageUrlCallback_ = getProfileImageUrlCallback;

  // No need for the static initialize() method anymore.
  chrome.send('onChangePicturePageInitialized')
}

ChangePicturePrivateApi.prototype.receiveDefaultImages = function() {
  // Maybe the if check is not even needed anymore, because constructor
guarantees
  // that getDefaultImagesCallback_ has a value.
  if (this.getDefaultImagesCallback_)
    this.getDefaultImagesCallback_(images);
};

...
...


Then from your client code (change_picture.js), you simply have to call the
constructor and create an instance, instead of using the static
ChangePicturePrivateApi.initialize method. Also to expose the methods of
ChangePicturePrivateApi, such that they can be called from C++, you can do same
trick as with
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....

This is just as suggestion (therefore optional), because I think less static
stuff makes the code more readable, but I understand how current implementation
works.

[michaelpg, 2016-01-11 20:38:05]

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.html (right):

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.html:30:
on-tap="onDefaultImageTap_" data-image-url$="[[item.url]]">
On 2016/01/11 19:25:50, dpapad wrote:
> data-image-url$="[[item.url]]
> 
> Have not seen this syntax before. Is the "$" a typo, or intentional?

https://www.polymer-project.org/1.0/docs/devguide/data-binding.html#attribute...

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.js:116: if
(element.nodeName == 'IMG')
On 2016/01/11 19:25:50, dpapad wrote:
> Should we change this to
> if (element.nodeName.toUpperCase() == 'IMG')
> 
> to make it a bit more robust?

element.nodeName is always element.tagName for Elements, which is always
uppercase.

and if the element isn't an Element, you have other problems.

[dpapad, 2016-01-13 01:35:52]

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.css (right):

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.css:6:
-webkit-margin-begin: 16px;
Is -webkit-margin-begin a valid property name?

I can't find any other reference to -webkit-margin-begin in the codebase. On the
other hand -webkit-margin-start has plenty of references.

[Dan Beam, 2016-01-13 18:04:37]

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.css (right):

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.css:6:
-webkit-margin-begin: 16px;
On 2016/01/13 01:35:52, dpapad wrote:
> Is -webkit-margin-begin a valid property name?
> 
> I can't find any other reference to -webkit-margin-begin in the codebase. On
the
> other hand -webkit-margin-start has plenty of references. 

no (start, end, before, after)

[tommycli, 2016-01-13 18:29:11]

dpapad: Got this working and compiling! Thanks!

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.css (right):

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.css:6:
-webkit-margin-begin: 16px;
On 2016/01/13 01:35:52, dpapad wrote:
> Is -webkit-margin-begin a valid property name?
> 
> I can't find any other reference to -webkit-margin-begin in the codebase. On
the
> other hand -webkit-margin-start has plenty of references. 

Done. Doh!

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.js:44: * The
default user image images. Populated by ChangePicturePrivateApi.
On 2016/01/11 19:25:50, dpapad wrote:
> Maybe change to "The default user images"?

Done.

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.js:115: var
imageUrl;
On 2016/01/11 19:25:50, dpapad wrote:
> Nit: Avoid implicit boolean conversions when possible.
> 
> var imageUrl = null;
> ...
> ...
> 
> if (imageUrl != null) {
>  ...
> }

Done.

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.js:153: * @param
{!string} imageUrl
On 2016/01/11 19:25:50, dpapad wrote:
> No need to put "!" before "string". It is implied, see
>
https://developers.google.com/closure/compiler/docs/js-for-compiler?hl=en#non...,
> specifically this sentence
> 
> "Functions and all value types (boolean, number, and string) are non-nullable
by
> default whether or not they are declared with the Non-nullable operator."

Done.

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture_private_api.js
(right):

https://codereview.chromium.org/1575543003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture_private_api.js:24:
function ChangePicturePrivateApi() {}
On 2016/01/11 19:25:50, dpapad wrote:
> How about instantiating an object from this class, instead of making
everything
> static? I think this would make the code less verbose and more readable. For
> example
> 
> /**
>  * @constructor
>  * @param ...
>  * @param ...
>  */
> function ChangePicturePrivateApi(
>     getDefaultImagesCallback,
>     getSelectedImageUrlCallback,
>     getOldImageUrlCallback,
>     getProfileImageUrlCallback) {
>   this.getDefaultImagesCallback_ = getDefaultImagesCallback;
>   this.getSelectedImageUrlCallback_ = getSelectedImageUrlCallback;
>   this.getOldImageUrlCallback_ = getOldImageUrlCallback;
>   this.getProfileImageUrlCallback_ = getProfileImageUrlCallback;
> 
>   // No need for the static initialize() method anymore.
>   chrome.send('onChangePicturePageInitialized')
> }
> 
> ChangePicturePrivateApi.prototype.receiveDefaultImages = function() {
>   // Maybe the if check is not even needed anymore, because constructor
> guarantees
>   // that getDefaultImagesCallback_ has a value.
>   if (this.getDefaultImagesCallback_)
>     this.getDefaultImagesCallback_(images);
> };
> 
> ...
> ...
> 
> 
> Then from your client code (change_picture.js), you simply have to call the
> constructor and create an instance, instead of using the static
> ChangePicturePrivateApi.initialize method. Also to expose the methods of
> ChangePicturePrivateApi, such that they can be called from C++, you can do
same
> trick as with
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....
> 
> This is just as suggestion (therefore optional), because I think less static
> stuff makes the code more readable, but I understand how current
implementation
> works.

Done. Based on your examples, I was able to eliminate ~80 lines of code!

[dpapad, 2016-01-13 19:00:08]

lgtm

LGTM with nits.

I like the shortened version of ChangePicturePrivateApi!

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.js:106: var
ChangePicturePage = nativeInterface;
Nit: Does the compiler complain if you do directly the following

cr.define('settings', function() {
  return {
    ChangePicturePage: nativeInterface,
  };
});

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture_private_api.js
(right):

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture_private_api.js:38:
* profile image, etc.
It would be useful to mention in the comment which functions on
ChangePicturePage are called as callbacks.

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:497:
html_source->AddLocalizedString("changePictureTitle",
Nit: By styleguide (I think) arguments need to start on the same column, 

html_source->AddLocalizedString("changePictureTitle",
                                IDS_SETTINGS_CHANGE_PICTURE_DIALOG_TITLE);

or 

html_source->AddLocalizedString(
    "changePictureTitle",
    IDS_SETTINGS_CHANGE_PICTURE_DIALOG_TITLE);

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_ui.cc:56:
AddSettingsPageUIHandler(new chromeos::settings::ChangePictureHandler);
Nit: Is it allowed to omit the () for no-arg constructors? We should at least be
consistent within the same file, see line 49.

[tommycli, 2016-01-13 19:35:18]

dpapad: thanks!!

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.js:106: var
ChangePicturePage = nativeInterface;
On 2016/01/13 19:00:07, dpapad wrote:
> Nit: Does the compiler complain if you do directly the following
> 
> cr.define('settings', function() {
>   return {
>     ChangePicturePage: nativeInterface,
>   };
> });

Exactly. That's what I had initially, but I get

##
/mnt/src/chromium/src/chrome/browser/resources/settings/people_page/change_picture.js:107:
ERROR - Property ChangePicturePage never defined on settings
##         ChangePicturePage: nativeInterface,

I have no idea why.

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture_private_api.js
(right):

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture_private_api.js:38:
* profile image, etc.
On 2016/01/13 19:00:07, dpapad wrote:
> It would be useful to mention in the comment which functions on
> ChangePicturePage are called as callbacks.

Done.

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:497:
html_source->AddLocalizedString("changePictureTitle",
On 2016/01/13 19:00:07, dpapad wrote:
> Nit: By styleguide (I think) arguments need to start on the same column, 
> 
> html_source->AddLocalizedString("changePictureTitle",
>                                 IDS_SETTINGS_CHANGE_PICTURE_DIALOG_TITLE);
> 
> or 
> 
> html_source->AddLocalizedString(
>     "changePictureTitle",
>     IDS_SETTINGS_CHANGE_PICTURE_DIALOG_TITLE);

Done. Ran git cl format on everything. (I didn't previously to make review
easier)

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/1575543003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_ui.cc:56:
AddSettingsPageUIHandler(new chromeos::settings::ChangePictureHandler);
On 2016/01/13 19:00:07, dpapad wrote:
> Nit: Is it allowed to omit the () for no-arg constructors? We should at least
be
> consistent within the same file, see line 49.

Done. You're right, I think 'new Foo()' is more standard in Chromium.

[tommycli, 2016-01-13 19:35:35]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-01-13 19:35:35]

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

[commit-bot: I haz the power, 2016-01-13 19:36:05]

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

[commit-bot: I haz the power, 2016-01-13 19:40:02]

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

[commit-bot: I haz the power, 2016-01-13 19:40:03]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-01-13 20:21:00]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-01-13 20:21:01]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/1575543003/#ps240001
(title: "Fix change_picture_handler to accomodate origin/master changes")

[commit-bot: I haz the power, 2016-01-13 20:22:26]

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

[commit-bot: I haz the power, 2016-01-13 23:28:40]

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

[commit-bot: I haz the power, 2016-01-13 23:28:41]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)

[tommycli, 2016-01-13 23:49:50]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-01-13 23:51:36]

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

[commit-bot: I haz the power, 2016-01-13 23:55:23]

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

[commit-bot: I haz the power, 2016-01-13 23:55:25]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[tommycli, 2016-01-13 23:56:16]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-01-14 00:00:40]

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

[commit-bot: I haz the power, 2016-01-14 02:11:52]

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

[commit-bot: I haz the power, 2016-01-14 02:11:53]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

[tommycli, 2016-01-14 02:27:17]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-01-14 02:28:56]

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

[commit-bot: I haz the power, 2016-01-14 03:38:15]

Description was changed from

==========
Settings People Revamp: Implement parts of ChromeOS Change Picture

Implements:
 1. Selecting from default images.
 2. Selecting the 'old' image (from a file, old camera shot, deprecated default
image).
 3. Selecting the Google profile photo.

Missing from this implmentation:
 1. Larger preview image on the side.
 2. Ability to take new pictures with the camera, and flip it.
 3. Ability to select an image from the file.
 4. Lots of polish.

BUG=563721
==========

to

==========
Settings People Revamp: Implement parts of ChromeOS Change Picture

Implements:
 1. Selecting from default images.
 2. Selecting the 'old' image (from a file, old camera shot, deprecated default
image).
 3. Selecting the Google profile photo.

Missing from this implmentation:
 1. Larger preview image on the side.
 2. Ability to take new pictures with the camera, and flip it.
 3. Ability to select an image from the file.
 4. Lots of polish.

BUG=563721
==========

[commit-bot: I haz the power, 2016-01-14 03:38:16]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-01-14 03:39:42]

Description was changed from

==========
Settings People Revamp: Implement parts of ChromeOS Change Picture

Implements:
 1. Selecting from default images.
 2. Selecting the 'old' image (from a file, old camera shot, deprecated default
image).
 3. Selecting the Google profile photo.

Missing from this implmentation:
 1. Larger preview image on the side.
 2. Ability to take new pictures with the camera, and flip it.
 3. Ability to select an image from the file.
 4. Lots of polish.

BUG=563721
==========

to

==========
Settings People Revamp: Implement parts of ChromeOS Change Picture

Implements:
 1. Selecting from default images.
 2. Selecting the 'old' image (from a file, old camera shot, deprecated default
image).
 3. Selecting the Google profile photo.

Missing from this implmentation:
 1. Larger preview image on the side.
 2. Ability to take new pictures with the camera, and flip it.
 3. Ability to select an image from the file.
 4. Lots of polish.

BUG=563721

Committed: https://crrev.com/1d4f8c7571693dfc346a2d3f12beda2481a291ea
Cr-Commit-Position: refs/heads/master@{#369308}
==========

[commit-bot: I haz the power, 2016-01-14 03:39:42]

Patchset 13 (id:??) landed as
https://crrev.com/1d4f8c7571693dfc346a2d3f12beda2481a291ea
Cr-Commit-Position: refs/heads/master@{#369308}