Move Singleton and related structs to namespace base

Move Singleton and related structs to namespace base

Public APIs from base should live inside base:: so moved Singleton class
and structs to base{} and fixed consumers.

also fixed:

** Presubmit ERRORS **
Found Singleton<T> in the following header files.
Please move them to an appropriate source file so that the template
gets instantiated in a single compilation unit.
  chrome/browser/plugins/plugin_finder.h \
  chromecast/media/base/media_message_loop.h \
  content/browser/media/android/media_drm_credential_manager.h

Presubmit warnings:
src/chrome/browser/extensions/warning_badge_service_factory.h:5:
  #ifndef header guard has wrong style, please use:
  CHROME_BROWSER_EXTENSIONS_WARNING_BADGE_SERVICE_FACTORY_H_
  [build/header_guard] [5]
src/chrome/browser/extensions/warning_badge_service_factory.h:39:
  #endif line should be "#endif  //
  CHROME_BROWSER_EXTENSIONS_WARNING_BADGE_SERVICE_FACTORY_H_"
  [build/header_guard] [5]

TBR=jam@chromium.org

Committed: https://crrev.com/36aa8be4eb53b3ea75b84a9646e0204725f3bc43
Cr-Commit-Position: refs/heads/master@{#348136}

[Olli Raula, 2015-08-21 13:58:40]

olli.raula@intel.com changed reviewers:
+ rvargas@chromium.org

[Olli Raula, 2015-08-21 14:09:21]

Hi, I did this what I asked previously. There is still errors, probably mostly
just missing edits but that Presubmit error is the biggest problem. Now
presubmit errors are done only by removing those friend declarations but that of
course make the build fail. Is there some way to skip that check because it is
not related to this any real way. I can maybe try to fix them later. For now I
added only you because I like to get some feedback before bothering others.
Should I ask reviewer for every sub-directory(or use some whole chromium
reviewer)?

[rvargas (doing something else), 2015-08-21 19:43:05]

Just some small nits.

https://codereview.chromium.org/1308823002/diff/40001/base/memory/singleton_u...
File base/memory/singleton_unittest.cc (left):

https://codereview.chromium.org/1308823002/diff/40001/base/memory/singleton_u...
base/memory/singleton_unittest.cc:8: 
Don't remove this line

https://codereview.chromium.org/1308823002/diff/40001/base/memory/singleton_u...
base/memory/singleton_unittest.cc:150: }  // namespace
nit: Keep the tests outside of the anon namespace?

https://codereview.chromium.org/1308823002/diff/40001/base/memory/singleton_u...
File base/memory/singleton_unittest.cc (right):

https://codereview.chromium.org/1308823002/diff/40001/base/memory/singleton_u...
base/memory/singleton_unittest.cc:8: namespace base {
I don't like to place tests inside the namespace of the code under test.

When someone is going through a random test to figure out why it is failing,
being as explicit as possible about where the code comes from is a good thing.
It may not be much of an issue in this specific case, but code may come from the
test file's anon namespace, or from the code under test, or from the OS, or from
any other file/library included here (more of an issue for tests outside of
base, but still).

It may be harder to write, but it is easier on the reader.

https://codereview.chromium.org/1308823002/diff/40001/base/threading/thread_i...
File base/threading/thread_id_name_manager.h (right):

https://codereview.chromium.org/1308823002/diff/40001/base/threading/thread_i...
base/threading/thread_id_name_manager.h:19: struct DefaultSingletonTraits;
keep in a single line.

https://codereview.chromium.org/1308823002/diff/40001/base/trace_event/trace_...
File base/trace_event/trace_log.cc (right):

https://codereview.chromium.org/1308823002/diff/40001/base/trace_event/trace_...
base/trace_event/trace_log.cc:43: class DeleteTraceLogForTesting {
move to line 57

https://codereview.chromium.org/1308823002/diff/40001/base/trace_event/trace_...
base/trace_event/trace_log.cc:46: base::Singleton<
no base::

[sdefresne, 2015-08-24 10:03:29]

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

[sdefresne, 2015-08-24 10:03:33]

https://codereview.chromium.org/1308823002/diff/40001/ios/chrome/browser/favi...
File ios/chrome/browser/favicon/favicon_service_factory.h (right):

https://codereview.chromium.org/1308823002/diff/40001/ios/chrome/browser/favi...
ios/chrome/browser/favicon/favicon_service_factory.h:13: struct
base::DefaultSingletonTraits;
Are you sure this work? Shouldn't this instead be (here and everywhere else):

namespace base {
template <typename T>
struct DefaultSingletonTraits;
}

[Olli Raula, 2015-08-24 10:37:33]

On 2015/08/24 10:03:33, sdefresne wrote:
> Are you sure this work? Shouldn't this instead be (here and everywhere else):


No, ;) . As I said that there is still some like that errors, I was just asking
some guidelines not to make unnecessary work. I will check them asap. Those
Presubmit ERRORS are much harder part and maybe take some time to get fixed if I
cannot skip it somehow.

[Olli Raula, 2015-08-24 15:40:20]

olli.raula@intel.com changed reviewers:
+ bauerb@chromium.org, gunsch@chromium.org, qinmin@chromium.org

[Olli Raula, 2015-08-27 06:30:47]

olli.raula@intel.com changed reviewers:
- bauerb@chromium.org, gunsch@chromium.org, qinmin@chromium.org

[Olli Raula, 2015-08-27 11:52:24]

olli.raula@intel.com changed reviewers:
+ jam@chromium.org

[Olli Raula, 2015-08-27 12:31:37]

Could you review,

sdefresne: components/*  ios/chrome/*
jam: the rest, but I can split it if you want.

I made what rvargas suggested but this:

https://codereview.chromium.org/1308823002/diff/40001/base/threading/thread_i...
base/threading/thread_id_name_manager.h:19: struct DefaultSingletonTraits;
keep in a single line.

is still in two lines because git cl format likes it more. If it is reasonable I
can force it to one line.

[sdefresne, 2015-08-27 15:25:12]

components/ and ios/chrome/ lgtm

Could you make sure to keep an empty line between bracket closing the namespace
and the rest of the file (and be consistent with the comment at the end of the
namespace, my understanding being that "// namespace base" is not necessary for
a namespace with only two lines)?

https://codereview.chromium.org/1308823002/diff/330001/components/keyed_servi...
File components/keyed_service/content/browser_context_dependency_manager.h
(right):

https://codereview.chromium.org/1308823002/diff/330001/components/keyed_servi...
components/keyed_service/content/browser_context_dependency_manager.h:18: }  //
namespace base
nit, optional: "// namespace base" not really necessary for just really short
namespace (I see that you're sometimes using "// namespace base" and sometimes
not using it, can you be consistent)

https://codereview.chromium.org/1308823002/diff/330001/ios/chrome/browser/boo...
File ios/chrome/browser/bookmarks/bookmark_model_factory.h (right):

https://codereview.chromium.org/1308823002/diff/330001/ios/chrome/browser/boo...
ios/chrome/browser/bookmarks/bookmark_model_factory.h:16: namespace bookmarks {
nit: can you live a blank line?

https://codereview.chromium.org/1308823002/diff/330001/ios/chrome/browser/his...
File ios/chrome/browser/history/web_history_service_factory.h (right):

https://codereview.chromium.org/1308823002/diff/330001/ios/chrome/browser/his...
ios/chrome/browser/history/web_history_service_factory.h:16: namespace history {
nit: can you leave a blank line here too

https://codereview.chromium.org/1308823002/diff/330001/ios/chrome/browser/sea...
File ios/chrome/browser/search_engines/template_url_service_factory.h (right):

https://codereview.chromium.org/1308823002/diff/330001/ios/chrome/browser/sea...
ios/chrome/browser/search_engines/template_url_service_factory.h:16: class
TemplateURLService;
ditto

https://codereview.chromium.org/1308823002/diff/330001/ios/chrome/browser/sig...
File ios/chrome/browser/signin/account_tracker_service_factory.h (right):

https://codereview.chromium.org/1308823002/diff/330001/ios/chrome/browser/sig...
ios/chrome/browser/signin/account_tracker_service_factory.h:18: namespace ios {
ditto

https://codereview.chromium.org/1308823002/diff/330001/ios/chrome/browser/sig...
File ios/chrome/browser/signin/signin_client_factory.h (right):

https://codereview.chromium.org/1308823002/diff/330001/ios/chrome/browser/sig...
ios/chrome/browser/signin/signin_client_factory.h:16: class SigninClient;
ditto

[jam, 2015-08-27 18:01:23]

On 2015/08/27 12:31:37, olli.raula wrote:
> Could you review,
> 
> sdefresne: components/*  ios/chrome/*
> jam: the rest, but I can split it if you want.

no need for me to review. look at
https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review...

[Olli Raula, 2015-08-28 07:55:37]

olli.raula@intel.com changed reviewers:
+ bauerb@chromium.org, danakj@chromium.org, halliwell@chromium.org,
qinmin@google.com, rockot@chromium.org

[Olli Raula, 2015-08-28 07:58:17]

Add more reviewers, could you review. base/* as it is the main part of this
change, others because 
there is something else than basic search and replace(check commit message).
Everything else I 
will approve in dev-list like jam suggested


danakj  base/*
bauerb  chrome/browser/plugins/plugin_finder.h
halliwell chromecast/media/base/media_message_loop.h \
qinmin content/browser/media/android/media_drm_credential_manager.h
rockot src/chrome/browser/extensions/warning_badge_service_factory.h

[halliwell, 2015-08-28 14:52:17]

https://codereview.chromium.org/1308823002/diff/350001/base/memory/singleton.h
File base/memory/singleton.h (right):

https://codereview.chromium.org/1308823002/diff/350001/base/memory/singleton....
base/memory/singleton.h:41: // TODO(joth): Move more of this file into namespace
base
remove TODO?

https://codereview.chromium.org/1308823002/diff/350001/chromecast/media/base/...
File chromecast/media/base/media_message_loop.h (right):

https://codereview.chromium.org/1308823002/diff/350001/chromecast/media/base/...
chromecast/media/base/media_message_loop.h:24: static MediaMessageLoop*
GetInstance();
why did you make this public?

[danakj, 2015-08-28 17:25:09]

On Fri, Aug 28, 2015 at 12:58 AM, <olli.raula@intel.com> wrote:

> Add more reviewers, could you review. base/* as it is the main part of this
> change, others because
> there is something else than basic search and replace(check commit
> message).
> Everything else I
> will approve in dev-list like jam suggested
>
>
> danakj  base/*
>

Chrome/rietveld freezes when I try to load this CL. It's too big for me to
physically review.


> bauerb  chrome/browser/plugins/plugin_finder.h
> halliwell chromecast/media/base/media_message_loop.h \
> qinmin content/browser/media/android/media_drm_credential_manager.h
> rockot src/chrome/browser/extensions/warning_badge_service_factory.h
>
> https://codereview.chromium.org/1308823002/
>

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

[Olli Raula, 2015-08-31 06:12:29]

olli.raula@intel.com changed reviewers:
+ mark@chromium.org

[Olli Raula, 2015-08-31 06:18:22]

> Chrome/rietveld freezes when I try to load this CL. It's too big for me to
> physically review.

I added mark if you can review the base/* or atleast the singleton classes. I
will drop danakj from this Rvargas can do it but he is ooo and I have 2 weeks
before vacation. 



halliwell: todo removed, I made GetInstance public because I had  to remove the
friend declaration because that presubmit error and keeping GetInstance public
is the way that is handled in other classes. I you have better way to do it, I
am welcome on ideas.

[Olli Raula, 2015-08-31 06:22:05]

olli.raula@intel.com changed reviewers:
- danakj@chromium.org

[Mark Mentovai, 2015-08-31 14:00:52]

LGTM otherwise in base. I only looked at base.

https://codereview.chromium.org/1308823002/diff/370001/base/memory/singleton.h
File base/memory/singleton.h (right):

https://codereview.chromium.org/1308823002/diff/370001/base/memory/singleton....
base/memory/singleton.h:278: }  // namespace base
Blank line after this.

https://codereview.chromium.org/1308823002/diff/370001/base/memory/singleton_...
File base/memory/singleton_unittest.cc (right):

https://codereview.chromium.org/1308823002/diff/370001/base/memory/singleton_...
base/memory/singleton_unittest.cc:11:
COMPILE_ASSERT(base::DefaultSingletonTraits<int>::kRegisterAtExit == true, a);
Putting these in base::<unnamed namespace> also would have been OK.

https://codereview.chromium.org/1308823002/diff/370001/base/trace_event/trace...
File base/trace_event/trace_log.cc (right):

https://codereview.chromium.org/1308823002/diff/370001/base/trace_event/trace...
base/trace_event/trace_log.cc:47: class DeleteTraceLogForTesting {
Should be in the unnamed namespace (base::trace_event::<unnamed> is OK).

[halliwell, 2015-08-31 20:51:37]

On 2015/08/31 14:00:52, Mark Mentovai - August is over wrote:
> LGTM otherwise in base. I only looked at base.
> 
> https://codereview.chromium.org/1308823002/diff/370001/base/memory/singleton.h
> File base/memory/singleton.h (right):
> 
>
https://codereview.chromium.org/1308823002/diff/370001/base/memory/singleton....
> base/memory/singleton.h:278: }  // namespace base
> Blank line after this.
> 
>
https://codereview.chromium.org/1308823002/diff/370001/base/memory/singleton_...
> File base/memory/singleton_unittest.cc (right):
> 
>
https://codereview.chromium.org/1308823002/diff/370001/base/memory/singleton_...
> base/memory/singleton_unittest.cc:11:
> COMPILE_ASSERT(base::DefaultSingletonTraits<int>::kRegisterAtExit == true, a);
> Putting these in base::<unnamed namespace> also would have been OK.
> 
>
https://codereview.chromium.org/1308823002/diff/370001/base/trace_event/trace...
> File base/trace_event/trace_log.cc (right):
> 
>
https://codereview.chromium.org/1308823002/diff/370001/base/trace_event/trace...
> base/trace_event/trace_log.cc:47: class DeleteTraceLogForTesting {
> Should be in the unnamed namespace (base::trace_event::<unnamed> is OK).

chromecast/ lgtm

[Ken Rockot(use gerrit already), 2015-08-31 23:14:09]

rs lgtm

[Bernhard Bauer, 2015-09-01 08:22:06]

c/b/plugins LGTM, thanks for the additional cleanup!

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

[Bernhard Bauer, 2015-09-01 08:23:52]

On Fri, Aug 28, 2015 at 6:25 PM Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Aug 28, 2015 at 12:58 AM, <olli.raula@intel.com> wrote:
>
>> Add more reviewers, could you review. base/* as it is the main part of
>> this
>> change, others because
>> there is something else than basic search and replace(check commit
>> message).
>> Everything else I
>> will approve in dev-list like jam suggested
>>
>>
>> danakj  base/*
>>
>
> Chrome/rietveld freezes when I try to load this CL. It's too big for me to
> physically review.
>

I had the same issue, but opening the CL in incognito (i.e. with extensions
disabled) worked, so disabling various Rietveld extensions might help in
your case as well.


> https://codereview.chromium.org/1308823002/
>>
>

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

[Bernhard Bauer, 2015-09-01 08:26:38]

On 2015/09/01 08:22:06, Bernhard Bauer wrote:
> c/b/plugins LGTM, thanks for the additional cleanup!

And of course I can't LGTM over email, but here goes 😃

[Olli Raula, 2015-09-01 12:23:17]

mark: I added the missing \n. I tried the "base/trace_event/trace_log.cc:47:"
but I realized that that cannot be in unnamed namespace because then singleton.h
cannot declare it as a friend. Or do you have some better information. I can do
it if you give some tip?


All: I added a topic to chromium-dev:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/FWlMKYOllog

[Mark Mentovai, 2015-09-01 14:11:00]

Then DeleteTraceLogForTesting should be in base::internal. We shouldn’t leave
something for testing only that’s specific to a single .cc file hanging out
exposed in the base namespace.

[danakj, 2015-09-01 20:55:07]

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

[danakj, 2015-09-01 20:55:14]

Cool, I was able to load the CL. base/ generally LGTM, a couple comments.

But why do you want to add base:: to Singleton at uses that are inside the base
namespace?

https://codereview.chromium.org/1308823002/diff/410001/base/logging_win.h
File base/logging_win.h (right):

https://codereview.chromium.org/1308823002/diff/410001/base/logging_win.h#new...
base/logging_win.h:18: }  // namespace base
nit: you don't need the closing namespace comment when it's just for forward
decls and such a small set lines in the braces. the linter lets you skip it
here, and i think it's just noisy in this case.

https://codereview.chromium.org/1308823002/diff/410001/base/memory/singleton_...
File base/memory/singleton_unittest.cc (right):

https://codereview.chromium.org/1308823002/diff/410001/base/memory/singleton_...
base/memory/singleton_unittest.cc:9: namespace {
you could wrap this file in the base namespace and then you don't have to add
base:: everywhere. like:

namespace base {
namespace {

... the rest of this file ...

}  // namespace
}  // namespace base

https://codereview.chromium.org/1308823002/diff/410001/base/trace_event/trace...
File base/trace_event/trace_event_etw_export_win.h (right):

https://codereview.chromium.org/1308823002/diff/410001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.h:15: // Fwd.
put this comment on the forward decl, or remove it. (dont put it on the
namespace base)

https://codereview.chromium.org/1308823002/diff/410001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.h:16: namespace base {
whitespace after this line

https://codereview.chromium.org/1308823002/diff/410001/base/trace_event/trace...
File base/trace_event/trace_event_win.h (right):

https://codereview.chromium.org/1308823002/diff/410001/base/trace_event/trace...
base/trace_event/trace_event_win.h:15: // Fwd.
put this comment on the forward decl or remove it

https://codereview.chromium.org/1308823002/diff/410001/base/trace_event/trace...
File base/trace_event/trace_log.h (right):

https://codereview.chromium.org/1308823002/diff/410001/base/trace_event/trace...
base/trace_event/trace_log.h:32: 
generally no whitespace between forward decls

[Olli Raula, 2015-09-03 14:26:01]

Thanks for feedback, danakj and mark comments are now done. Will commit it on
Monday if nothing special happens.

[danakj, 2015-09-03 17:37:44]

https://codereview.chromium.org/1308823002/diff/510001/base/memory/singleton.h
File base/memory/singleton.h (right):

https://codereview.chromium.org/1308823002/diff/510001/base/memory/singleton....
base/memory/singleton.h:39: class DeleteTraceLogForTesting;
Why expose this in the header?

[Olli Raula, 2015-09-07 07:32:55]

On 2015/09/03 17:37:44, danakj wrote:
> Why expose this in the header?

Because without it "../../base/memory/singleton.h:222:26: error:
‘DeleteTraceLogForTesting’ in namespace ‘base::internal’ does not name a type"
It is not needed if that function is not in internal::

[Olli Raula, 2015-09-08 07:34:14]

olli.raula@intel.com changed reviewers:
+ jochen@chromium.org

[Olli Raula, 2015-09-08 07:35:28]

Jochen: Could you check the PRESUBMIT_test.py , otherwice this should be ready
for landing

[Olli Raula, 2015-09-08 07:37:53]

Jochen: Could you check the PRESUBMIT_test.py , otherwice this should be ready
for landing

[danakj, 2015-09-08 18:34:09]

On Mon, Sep 7, 2015 at 12:32 AM, <olli.raula@intel.com> wrote:

> On 2015/09/03 17:37:44, danakj wrote:
>
>> Why expose this in the header?
>>
>
> Because without it "../../base/memory/singleton.h:222:26: error:
> ‘DeleteTraceLogForTesting’ in namespace ‘base::internal’ does not name a
> type"
> It is not needed if that function is not in internal::
>

OK but why'd you move it into internal? Why not anonymous namespace? It's
defined and used in a single .cc file, no?

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

[Olli Raula, 2015-09-09 06:21:36]

> OK but why'd you move it into internal? Why not anonymous namespace? It's
> defined and used in a single .cc file, no?

Danakj: That's marks advice after I find out that I cannot make it friend if it
is in anonymous namespace. Check previous messages(20,26,27) .

Jochen: I like to have you check fast because I am going to vacation this
friday, thanks.

[jochen (gone - plz use gerrit), 2015-09-09 14:21:10]

lgtm

[Olli Raula, 2015-09-10 05:33:27]

The CQ bit was checked by olli.raula@intel.com

[Olli Raula, 2015-09-10 05:33:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, bauerb@chromium.org, rockot@chromium.org,
mark@chromium.org, halliwell@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1308823002/#ps530001
(title: "ToT")

[commit-bot: I haz the power, 2015-09-10 05:33:40]

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

[commit-bot: I haz the power, 2015-09-10 05:41:40]

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

[commit-bot: I haz the power, 2015-09-10 05:41:47]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Olli Raula, 2015-09-10 07:07:33]

The CQ bit was checked by olli.raula@intel.com

[Olli Raula, 2015-09-10 07:07:35]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rockot@chromium.org, jochen@chromium.org, danakj@chromium.org,
mark@chromium.org, halliwell@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1308823002/#ps550001
(title: "ToT")

[commit-bot: I haz the power, 2015-09-10 07:07:47]

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

[commit-bot: I haz the power, 2015-09-10 09:50:21]

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

[commit-bot: I haz the power, 2015-09-10 09:50:25]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Olli Raula, 2015-09-10 10:16:29]

The CQ bit was checked by olli.raula@intel.com

[commit-bot: I haz the power, 2015-09-10 10:16:40]

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

[commit-bot: I haz the power, 2015-09-10 11:14:41]

Committed patchset #29 (id:550001)

[commit-bot: I haz the power, 2015-09-10 11:15:34]

Patchset 29 (id:??) landed as
https://crrev.com/36aa8be4eb53b3ea75b84a9646e0204725f3bc43
Cr-Commit-Position: refs/heads/master@{#348136}

[commit-bot: I haz the power, 2015-09-23 12:09:43]

Patchset 29 (id:??) landed as
https://crrev.com/36aa8be4eb53b3ea75b84a9646e0204725f3bc43
Cr-Commit-Position: refs/heads/master@{#348136}