SRT Bubble

Offer to download the Software removal tool when the sw_reporter component found something that it can handle.

For chrome/app/chrome_command_ids.h
TBR=cpu@chromium.org

BUG=416582
Committed: https://crrev.com/2b3b655281789477955499659b5b06eba5faacfc
Cr-Commit-Position: refs/heads/master@{#296594}

[MAD, 2014-09-24 04:29:13]

Patchset #2 (id:20001) has been deleted

[MAD, 2014-09-24 04:31:44]

mad@chromium.org changed reviewers:
+ gab@chromium.org, robertshield@google.com

[MAD, 2014-09-24 04:31:45]

gab@, robertshield@, can you take a first look before I go to ONWERs?

battre@, & engedy@, you are welcome to comment, but I CC'd you mainly to keep
you posted on my progress, and see if you think this could be merged/modified to
use the JTL interpreter...

Thanks!

BYE
MAD

[engedy, 2014-09-24 10:14:44]

engedy@chromium.org changed reviewers:
+ engedy@chromium.org

[engedy, 2014-09-24 10:14:45]

Just a few comments.

https://codereview.chromium.org/599653002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/srt_global_error.cc (right):

https://codereview.chromium.org/599653002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:19: const char
kSRTDownloadURL[] = "http://www.google.com/chrome/srt/";
We should explicitly use HTTPS here, as www.google.com does not seem to be on
the HSTS preload list.

https://codereview.chromium.org/599653002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:117:
global_error_service_->RemoveGlobalError(this);
I am unsure about the details of the GlobalErrorBubbleView implementation or how
much animation is playing during closing the bubble.

However, theoretically, this *might* end up calling OnBubbleViewDidClose()
synchronously, in which case we would leak the SRTGlobalError instance. We
should double-check that this is not the case, or, even better, commit suicide
right after this line (instead of line 97). This should be fine as the
BubbleView only has a weak reference to the GlobalError.

https://codereview.chromium.org/599653002/diff/40001/components/component_upd...
File components/component_updater/pref_names.cc (right):

https://codereview.chromium.org/599653002/diff/40001/components/component_upd...
components/component_updater/pref_names.cc:17: extern const char
kSwReporterPromptState[] = "software_reporter.prompt_state";
nit: Remove extern.

[MAD, 2014-09-24 11:12:33]

Thanks! All addressed... Anything else?

BYE
MAD

https://codereview.chromium.org/599653002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/srt_global_error.cc (right):

https://codereview.chromium.org/599653002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:19: const char
kSRTDownloadURL[] = "http://www.google.com/chrome/srt/";
On 2014/09/24 10:14:45, engedy wrote:
> We should explicitly use HTTPS here, as http://www.google.com does not seem to
be on
> the HSTS preload list.

Done.

https://codereview.chromium.org/599653002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:117:
global_error_service_->RemoveGlobalError(this);
Actually, the call to Close the Widget is synchronous, but the closing of the
widget itself, hence the calling of OnBubbleViewDidClose is asynchronous.

But still, you are right, this is fragile, and we should benefit from the fact
that the GlobalError is a weak pointer... so...

Done! ;-)

On 2014/09/24 10:14:45, engedy wrote:
> I am unsure about the details of the GlobalErrorBubbleView implementation or
how
> much animation is playing during closing the bubble.
> 
> However, theoretically, this *might* end up calling OnBubbleViewDidClose()
> synchronously, in which case we would leak the SRTGlobalError instance. We
> should double-check that this is not the case, or, even better, commit suicide
> right after this line (instead of line 97). This should be fine as the
> BubbleView only has a weak reference to the GlobalError.

https://codereview.chromium.org/599653002/diff/40001/components/component_upd...
File components/component_updater/pref_names.cc (right):

https://codereview.chromium.org/599653002/diff/40001/components/component_upd...
components/component_updater/pref_names.cc:17: extern const char
kSwReporterPromptState[] = "software_reporter.prompt_state";
On 2014/09/24 10:14:45, engedy wrote:
> nit: Remove extern.

Done.

[robertshield, 2014-09-24 13:02:11]

robertshield@chromium.org changed reviewers:
+ robertshield@chromium.org

[robertshield, 2014-09-24 13:02:13]

Just a couple of nits from me, LGTM

https://codereview.chromium.org/599653002/diff/60001/chrome/app/chromium_stri...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/599653002/diff/60001/chrome/app/chromium_stri...
chrome/app/chromium_strings.grd:910: <message name="IDS_SRT_MENU_ITEM" desc="In
Title Case: Text for the Chrome menu option replacing Update required.">
This is the same desc text as for IDS_UPGRADE_BUBBLE_MENU_ITEM, is that
intentional?

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:122: if
((*it)->GetBubbleView())
Just to double check: GetBubbleView() should return NULL for the |global_error|
just added line 114 since it hasn't been shown yet, right?

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/srt_global_error.cc (right):

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:19: const char
kSRTDownloadURL[] = "https://www.google.com/chrome/srt/";
should some of this file be #if defined(GOOGLE_CHROME_BUILD)? Not sure we should
have google.com URLs in Chromium.

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:117: // bubble actually gets
closed by RemoveGlobalError, we can safely suicide
totally ignorable grammar nit: s/suicide/commit suicide/

https://codereview.chromium.org/599653002/diff/60001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/599653002/diff/60001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:1056: 'browser/safe_browsing/srt_global_error.h',
alphabetic ordering: srt goes after safe

[engedy, 2014-09-24 13:37:15]

LGTM module one more comment.

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/srt_global_error.cc (right):

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:69: ShowBubbleView(browser);
Showing more than one instance of the bubble is unusual for a GlobalError.
However, by looking at the code, I could not find anything that could go
terribly wrong here. Just wanted to make sure that you are aware of this and
double-checked.

[gab, 2014-09-24 16:22:37]

lg, comments below

https://codereview.chromium.org/599653002/diff/60001/chrome/app/google_chrome...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/599653002/diff/60001/chrome/app/google_chrome...
chrome/app/google_chrome_strings.grd:832: <!-- SRT bubble messages -->
I think those should be wrapped in 
<if expr="is_win">

https://codereview.chromium.org/599653002/diff/60001/chrome/app/google_chrome...
chrome/app/google_chrome_strings.grd:835: Chrome Behaving Strangely?
Do we typically use "Chrome" in other places or do we always use "Google
Chrome"?

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:97: void
ReportAndClearExitCode(int exit_code, const std::string& version) {
Augment method comment with |version|.

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:102:
chrome::FindLastActiveWithHostDesktopType(chrome::GetActiveDesktop());
I don't think this is what you want. This may be NULL in numerous scenarios (see
method comment) and can also be a non-tabbed browser.

The comment @
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
hints that you want to use a ScopedTabbedBrowserDisplayer for this use case
(which will even take care of creating one for you if none exist).

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:110:
profile->GetPrefs()->SetString(prefs::kSwReporterPromptState, version);
Only set this pref if we actually end up showing the bubble below.

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:114:
global_error_service->AddGlobalError(global_error);
This gives ownership of |global_error|, make this explicit by putting it in a
scoped_ptr above and releasing it here.

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:126:
global_error->ShowBubbleView(browser);
This is not safe as you gave up ownership of |global_error| above and thus
depend on the internal implementation of GlobalErrorService to have kept it
alive (the scoped_ptr syntax suggested above specifically prevents this type of
mistake).

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/srt_global_error.cc (right):

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:27: SRT_PROMPT_MAX = 3,
Do not explicitly label SRT_PROMPT_MAX (let it be bumped up naturally as things
are inserted before it)

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:47: bool
SRTGlobalError::IsSupportedOnPlatform() {
Are there any callers of this method? Shouldn't this entire file be compiled out
on non-win (or at least stubbed-out, e.g.,
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
)?

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:120: // synchronous, we'll
still be OK.
This comment should not be necessary. All your code ever does is "create the
error and give to the service" (the service's API then claims to grab ownership
of the error); and here it calls RemoveGlobalError() which's API claims it's
giving ownership back to you.

How the service handles that in between is none of this code's business, if the
service claims it gives ownership back to us, it's up to it to make sure it
didn't give a strong dependency to anybody; thus you can just delete this
pointer without having to justify (justifying it here only binds you to
implementation details of the service which this code should have nothing to do
with).

https://codereview.chromium.org/599653002/diff/60001/components/component_upd...
File components/component_updater/pref_names.cc (right):

https://codereview.chromium.org/599653002/diff/60001/components/component_upd...
components/component_updater/pref_names.cc:17: const char
kSwReporterPromptState[] = "software_reporter.prompt_state";
Explicitly split profile prefs from Local State prefs here.

[MAD, 2014-09-24 20:40:36]

Thanks all. Addressed/Answered all comments.

Will now reach out to OWNERS...

Let me know if you any more comments.

BYE
MAD

https://codereview.chromium.org/599653002/diff/60001/chrome/app/chromium_stri...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/599653002/diff/60001/chrome/app/chromium_stri...
chrome/app/chromium_strings.grd:910: <message name="IDS_SRT_MENU_ITEM" desc="In
Title Case: Text for the Chrome menu option replacing Update required.">
On 2014/09/24 13:02:12, robertshield wrote:
> This is the same desc text as for IDS_UPGRADE_BUBBLE_MENU_ITEM, is that
> intentional?

Yes, keeping them separate in case we would like to make them different at one
point. E.g., via Finch.

https://codereview.chromium.org/599653002/diff/60001/chrome/app/google_chrome...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/599653002/diff/60001/chrome/app/google_chrome...
chrome/app/google_chrome_strings.grd:832: <!-- SRT bubble messages -->
On 2014/09/24 16:22:36, gab wrote:
> I think those should be wrapped in 
> <if expr="is_win">

Done.

https://codereview.chromium.org/599653002/diff/60001/chrome/app/google_chrome...
chrome/app/google_chrome_strings.grd:835: Chrome Behaving Strangely?
On 2014/09/24 16:22:36, gab wrote:
> Do we typically use "Chrome" in other places or do we always use "Google
> Chrome"?

The text is the one chosen by UX.

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:97: void
ReportAndClearExitCode(int exit_code, const std::string& version) {
On 2014/09/24 16:22:36, gab wrote:
> Augment method comment with |version|.

Done.

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:102:
chrome::FindLastActiveWithHostDesktopType(chrome::GetActiveDesktop());
The problem is that I don't have a profile here, so I can't use
ScopedTabbedBrowserDisplayer. I think it's OK if in some edge cases we don't get
a browser, we will simply try again when we do have one. But your comment about
the potential of getting a non tabbed browser is interesting, indeed we see a
weird bubble if we launch with a --app= switch... Thanks! Fixed...

On 2014/09/24 16:22:36, gab wrote:
> I don't think this is what you want. This may be NULL in numerous scenarios
(see
> method comment) and can also be a non-tabbed browser.
> 
> The comment @
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> hints that you want to use a ScopedTabbedBrowserDisplayer for this use case
> (which will even take care of creating one for you if none exist).

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:110:
profile->GetPrefs()->SetString(prefs::kSwReporterPromptState, version);
On 2014/09/24 16:22:36, gab wrote:
> Only set this pref if we actually end up showing the bubble below.

No, because we still create the global error which makes the bubble available
via the menu, and will show up as a bubble as soon as the other bubbles get
dismissed.

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:114:
global_error_service->AddGlobalError(global_error);
On 2014/09/24 16:22:36, gab wrote:
> This gives ownership of |global_error|, make this explicit by putting it in a
> scoped_ptr above and releasing it here.

If I release it here, I won't be able to use it below...

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:122: if
((*it)->GetBubbleView())
On 2014/09/24 13:02:12, robertshield wrote:
> Just to double check: GetBubbleView() should return NULL for the
|global_error|
> just added line 114 since it hasn't been shown yet, right?

Yes.

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:126:
global_error->ShowBubbleView(browser);
On 2014/09/24 16:22:36, gab wrote:
> This is not safe as you gave up ownership of |global_error| above and thus
> depend on the internal implementation of GlobalErrorService to have kept it
> alive (the scoped_ptr syntax suggested above specifically prevents this type
of
> mistake).

But then if we release it above, we still can't use it here... :-/
Added comments to clarify the lifespan and ownership...

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/srt_global_error.cc (right):

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:19: const char
kSRTDownloadURL[] = "https://www.google.com/chrome/srt/";
On 2014/09/24 13:02:12, robertshield wrote:
> should some of this file be #if defined(GOOGLE_CHROME_BUILD)? Not sure we
should
> have http://google.com URLs in Chromium.

Hum, but then, which value do we put here? Actually, it's the whole bubble that
should only show up in official builds... I'll put a TODO for now...

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:27: SRT_PROMPT_MAX = 3,
On 2014/09/24 16:22:37, gab wrote:
> Do not explicitly label SRT_PROMPT_MAX (let it be bumped up naturally as
things
> are inserted before it)

Done.

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:47: bool
SRTGlobalError::IsSupportedOnPlatform() {
On 2014/09/24 16:22:37, gab wrote:
> Are there any callers of this method? Shouldn't this entire file be compiled
out
> on non-win (or at least stubbed-out, e.g.,
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
> )?

Good point, this was a leftover from the profile reset thingy...

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:69: ShowBubbleView(browser);
On 2014/09/24 13:37:15, engedy wrote:
> Showing more than one instance of the bubble is unusual for a GlobalError.
> However, by looking at the code, I could not find anything that could go
> terribly wrong here. Just wanted to make sure that you are aware of this and
> double-checked.

Since the bubble is closed when it loses focus, this shouldn't happen. Added a
DCHECK.

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:117: // bubble actually gets
closed by RemoveGlobalError, we can safely suicide
On 2014/09/24 13:02:12, robertshield wrote:
> totally ignorable grammar nit: s/suicide/commit suicide/

Done.

https://codereview.chromium.org/599653002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/srt_global_error.cc:120: // synchronous, we'll
still be OK.
Actually, this comment was added to explain the state of the code to another
code reviews, but since the code now changed to not rely on the
global_error_service_ being NULL or not, the comment isn't necessary anymore...
Thanks! :-)
On 2014/09/24 16:22:36, gab wrote:
> This comment should not be necessary. All your code ever does is "create the
> error and give to the service" (the service's API then claims to grab
ownership
> of the error); and here it calls RemoveGlobalError() which's API claims it's
> giving ownership back to you.
> 
> How the service handles that in between is none of this code's business, if
the
> service claims it gives ownership back to us, it's up to it to make sure it
> didn't give a strong dependency to anybody; thus you can just delete this
> pointer without having to justify (justifying it here only binds you to
> implementation details of the service which this code should have nothing to
do
> with).

https://codereview.chromium.org/599653002/diff/60001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/599653002/diff/60001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:1056: 'browser/safe_browsing/srt_global_error.h',
On 2014/09/24 13:02:12, robertshield wrote:
> alphabetic ordering: srt goes after safe

Done.

https://codereview.chromium.org/599653002/diff/60001/components/component_upd...
File components/component_updater/pref_names.cc (right):

https://codereview.chromium.org/599653002/diff/60001/components/component_upd...
components/component_updater/pref_names.cc:17: const char
kSwReporterPromptState[] = "software_reporter.prompt_state";
On 2014/09/24 16:22:37, gab wrote:
> Explicitly split profile prefs from Local State prefs here.

Done.

[MAD, 2014-09-24 20:48:55]

mad@chromium.org changed reviewers:
+ cpu@chromium.org

[MAD, 2014-09-24 20:49:50]

mad@chromium.org changed reviewers:
+ noelutz@chromium.org, waffles@chromium.org
- cpu@chromium.org

[MAD, 2014-09-24 20:49:50]

OWNERS review:

waffles@:
chrome/browser/component_updater/sw_reporter_installer_win.*
components/component_updater/pref_names.*

noelutz@:
chrome/browser/safe_browsing/srt_global_error.*

Thanks!

BYE
MAD

[waffles, 2014-09-24 21:33:53]

lgtm for */component_updater/*

[noé, 2014-09-24 23:13:28]

lgtm

[MAD, 2014-09-24 23:18:57]

Excellent! Thanks all...

Landing as soon as the tree re-opens...


On Wed, Sep 24, 2014 at 7:13 PM, <noelutz@chromium.org> wrote:

> lgtm
>
>
>
> https://codereview.chromium.org/599653002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[MAD, 2014-09-24 23:39:16]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2014-09-24 23:40:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/599653002/100001

[commit-bot: I haz the power, 2014-09-25 00:44:58]

Committed patchset #5 (id:100001) as 97e84b2fb4f1f8158612f5365dc0312a921b0eba

[commit-bot: I haz the power, 2014-09-25 00:45:31]

Patchset 5 (id:??) landed as
https://crrev.com/2b3b655281789477955499659b5b06eba5faacfc
Cr-Commit-Position: refs/heads/master@{#296594}

[gab, 2014-09-25 16:26:30]

A few comments post-commit.

https://codereview.chromium.org/599653002/diff/80001/chrome/browser/component...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/599653002/diff/80001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:143: browser = new
Browser(Browser::CreateParams(profile, desktop_type));
Are you sure you want to create a browser for this? Your initial comment in this
method suggests that you're okay with trying again later, why not in this case?

https://codereview.chromium.org/599653002/diff/80001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:148: if
(prompt_version.empty()) {
This check should go above the TYPE_TABBED check or you may end up creating a
browser and not use it...

[MAD, 2014-09-25 16:41:42]

Thanks, addressed in  https://codereview.chromium.org/607573002

https://codereview.chromium.org/599653002/diff/80001/chrome/browser/component...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/599653002/diff/80001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:143: browser = new
Browser(Browser::CreateParams(profile, desktop_type));
On 2014/09/25 16:26:29, gab wrote:
> Are you sure you want to create a browser for this? Your initial comment in
this
> method suggests that you're okay with trying again later, why not in this
case?

Well, I thought the suggestion of using the scope tab browser to create tabbed
browser when none exist was cool, so I used the same idea here. :-)

https://codereview.chromium.org/599653002/diff/80001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:148: if
(prompt_version.empty()) {
On 2014/09/25 16:26:29, gab wrote:
> This check should go above the TYPE_TABBED check or you may end up creating a
> browser and not use it...

Ho! Good point... Damn!

[gab, 2014-09-25 16:56:23]

On 2014/09/25 16:41:42, MAD wrote:
> Thanks, addressed in  https://codereview.chromium.org/607573002

Woot! With this, lgtm :-)!