leveldb::MojoEnv should not inherit from ChromiumEnv.

leveldb::MojoEnv should not inherit from ChromiumEnv.

This changes leveldb::MojoEnv to no longer inherit from ChromiumEnv
since it overrides nearly every method of that class anyway. Also
rather than having the one-background-thread-per-env that ChromiumEnv
has this uses base::PostTask to post background tasks to the global task
scheduler.

Also got rid of being able to specify a custom environment name
for MojoEnv. MojoEnv doesn't (and hasn't) actually emit any of
the ChromiumEnv histograms. In a follow-up CL I'll add histograms
to MojoEnv similar to the ones that ChromiumEnv emits, but for now
it doesn't make sense to have these histograms defined.

BUG=697612
Review-Url: https://codereview.chromium.org/2822503002
Cr-Commit-Position: refs/heads/master@{#464553}
Committed: https://chromium.googlesource.com/chromium/src/+/6bfb96f0eb4350010c8d9c29fb40408212536bcc

[Marijn Kruisselbrink, 2017-04-13 17:35:45]

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

[commit-bot: I haz the power, 2017-04-13 17:36:21]

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

[Marijn Kruisselbrink, 2017-04-13 17:51:15]

Description was changed from

==========
WIP Don't inherit from ChromiumEnv in MojoEnv.

BUG=
==========

to

==========
leveldb::MojoEnv should not inherit from ChromiumEnv.

This changes leveldb::MojoEnv to no longer inherit from ChromiumEnv
since it overrides nearly every method of that class anyway. Also
rather than having the one-background-thread-per-env that ChromiumEnv
has this uses base::PostTask to post background tasks to the global task
scheduler.

BUG=697612
==========

[Marijn Kruisselbrink, 2017-04-13 17:57:52]

Description was changed from

==========
leveldb::MojoEnv should not inherit from ChromiumEnv.

This changes leveldb::MojoEnv to no longer inherit from ChromiumEnv
since it overrides nearly every method of that class anyway. Also
rather than having the one-background-thread-per-env that ChromiumEnv
has this uses base::PostTask to post background tasks to the global task
scheduler.

BUG=697612
==========

to

==========
leveldb::MojoEnv should not inherit from ChromiumEnv.

This changes leveldb::MojoEnv to no longer inherit from ChromiumEnv
since it overrides nearly every method of that class anyway. Also
rather than having the one-background-thread-per-env that ChromiumEnv
has this uses base::PostTask to post background tasks to the global task
scheduler.

Also got rid of being able to specify a custom environment name
for MojoEnv. MojoEnv doesn't (and hasn't) actually emit any of
the ChromiumEnv histograms. In a follow-up CL I'll add histograms
to MojoEnv similar to the ones that ChromiumEnv emits, but for now
it doesn't make sense to have these histograms defined.

BUG=697612
==========

[Marijn Kruisselbrink, 2017-04-13 18:01:28]

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

[commit-bot: I haz the power, 2017-04-13 18:01:49]

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

[Marijn Kruisselbrink, 2017-04-13 18:03:17]

mek@chromium.org changed reviewers:
+ cmumford@chromium.org, erg@chromium.org, michaeln@chromium.org

[Marijn Kruisselbrink, 2017-04-13 18:03:17]

Probably a nicer approach than in https://codereview.chromium.org/2722293002 to
address the lifetime issues around MojoEnv/ChromiumEnv: just don't use
ChromiumEnv at all.

[commit-bot: I haz the power, 2017-04-13 19:28:33]

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

[commit-bot: I haz the power, 2017-04-13 19:28:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2017-04-13 19:38:33]

lgtm

[cmumford, 2017-04-13 19:53:22]

nice! lgtm.

[michaeln, 2017-04-13 20:39:56]

lgtm 2, that's a lot nicer :)

[Marijn Kruisselbrink, 2017-04-13 20:41:35]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2017-04-13 20:42:22]

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

[commit-bot: I haz the power, 2017-04-13 20:54:20]

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

[commit-bot: I haz the power, 2017-04-13 20:54:22]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Marijn Kruisselbrink, 2017-04-13 20:56:41]

mek@chromium.org changed reviewers:
+ isherman@chromium.org, tsepez@chromium.org

[Marijn Kruisselbrink, 2017-04-13 20:56:42]

+isherman for histograms OWNERS
+tsepez for mojom OWNERS

[Tom Sepez, 2017-04-13 21:00:13]

RS LGTM on deleting method from mojom.

[Ilya Sherman, 2017-04-13 21:03:03]

https://codereview.chromium.org/2822503002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2822503002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:119852: -  <affected-histogram
name="LevelDBEnv.LocalStorage.TimeUntilSuccessFor"/>
Should these histograms be marked as <obsolete> instead?

https://codereview.chromium.org/2822503002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:119873: -      label="Restricted to
LocalStorage LevelDB environments"/>
Please mark this suffix as obsolete rather than removing it.  This allows
historical data to continue to be viewed.

[Marijn Kruisselbrink, 2017-04-13 21:23:41]

On 2017/04/13 at 21:03:03, isherman wrote:
>
https://codereview.chromium.org/2822503002/diff/20001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (left):
> 
>
https://codereview.chromium.org/2822503002/diff/20001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:119852: -  <affected-histogram
name="LevelDBEnv.LocalStorage.TimeUntilSuccessFor"/>
> Should these histograms be marked as <obsolete> instead?
> 
>
https://codereview.chromium.org/2822503002/diff/20001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:119873: -      label="Restricted to
LocalStorage LevelDB environments"/>
> Please mark this suffix as obsolete rather than removing it.  This allows
historical data to continue to be viewed.

There is no such thing as historical data for these histograms. There never was
code that emitted these histograms.

[Ilya Sherman, 2017-04-13 21:25:24]

On 2017/04/13 21:23:41, Marijn Kruisselbrink wrote:
> On 2017/04/13 at 21:03:03, isherman wrote:
> >
>
https://codereview.chromium.org/2822503002/diff/20001/tools/metrics/histogram...
> > File tools/metrics/histograms/histograms.xml (left):
> > 
> >
>
https://codereview.chromium.org/2822503002/diff/20001/tools/metrics/histogram...
> > tools/metrics/histograms/histograms.xml:119852: -  <affected-histogram
> name="LevelDBEnv.LocalStorage.TimeUntilSuccessFor"/>
> > Should these histograms be marked as <obsolete> instead?
> > 
> >
>
https://codereview.chromium.org/2822503002/diff/20001/tools/metrics/histogram...
> > tools/metrics/histograms/histograms.xml:119873: -      label="Restricted to
> LocalStorage LevelDB environments"/>
> > Please mark this suffix as obsolete rather than removing it.  This allows
> historical data to continue to be viewed.
> 
> There is no such thing as historical data for these histograms. There never
was
> code that emitted these histograms.

I see.  If all of the histograms being removed have never been emitted to, then
LGTM.  Thanks.

[Marijn Kruisselbrink, 2017-04-13 21:28:11]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2017-04-13 21:29:17]

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

[commit-bot: I haz the power, 2017-04-13 21:36:19]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492118891185760,
"parent_rev": "4f4530109b8b68643c6a3afbb239e21580d46a8a", "commit_rev":
"6bfb96f0eb4350010c8d9c29fb40408212536bcc"}

[commit-bot: I haz the power, 2017-04-13 21:37:10]

Description was changed from

==========
leveldb::MojoEnv should not inherit from ChromiumEnv.

This changes leveldb::MojoEnv to no longer inherit from ChromiumEnv
since it overrides nearly every method of that class anyway. Also
rather than having the one-background-thread-per-env that ChromiumEnv
has this uses base::PostTask to post background tasks to the global task
scheduler.

Also got rid of being able to specify a custom environment name
for MojoEnv. MojoEnv doesn't (and hasn't) actually emit any of
the ChromiumEnv histograms. In a follow-up CL I'll add histograms
to MojoEnv similar to the ones that ChromiumEnv emits, but for now
it doesn't make sense to have these histograms defined.

BUG=697612
==========

to

==========
leveldb::MojoEnv should not inherit from ChromiumEnv.

This changes leveldb::MojoEnv to no longer inherit from ChromiumEnv
since it overrides nearly every method of that class anyway. Also
rather than having the one-background-thread-per-env that ChromiumEnv
has this uses base::PostTask to post background tasks to the global task
scheduler.

Also got rid of being able to specify a custom environment name
for MojoEnv. MojoEnv doesn't (and hasn't) actually emit any of
the ChromiumEnv histograms. In a follow-up CL I'll add histograms
to MojoEnv similar to the ones that ChromiumEnv emits, but for now
it doesn't make sense to have these histograms defined.

BUG=697612

Review-Url: https://codereview.chromium.org/2822503002
Cr-Commit-Position: refs/heads/master@{#464553}
Committed:
https://chromium.googlesource.com/chromium/src/+/6bfb96f0eb4350010c8d9c29fb40...
==========

[commit-bot: I haz the power, 2017-04-13 21:37:11]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/6bfb96f0eb4350010c8d9c29fb40...