Remove RuntimeAPIDelegate::GetPreviousVersion.

Remove RuntimeAPIDelegate::GetPreviousVersion.

We currently go to chrome/ to get the extension version, but we don't
need to. ExtensionRegistry in extensions/ has the info already, move
the function to ExtensionRegistry.

Also, the name GetPreviousVersion is misleading. It only gives us the
current version, so rename it to be simply GetVersion.

BUG=723754
Test=None, internal only change.

Review-Url: https://codereview.chromium.org/2892843002
Cr-Commit-Position: refs/heads/master@{#473332}
Committed: https://chromium.googlesource.com/chromium/src/+/640dc5161179cd9058dd3c97788880a8ac4c7f4d

[lazyboy, 2017-05-18 23:27:26]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2017-05-18 23:27:27]

Hi Devlin,
The chrome/ dependency is getting in my way to
https://codereview.chromium.org/2893693002/, so in order to keep that CL
simpler, extracting this to a separate CL. Thanks!

[lazyboy, 2017-05-18 23:27:39]

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

[commit-bot: I haz the power, 2017-05-18 23:29:20]

Dry run: CQ is trying da patch.

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

[Devlin, 2017-05-19 01:07:10]

Nice, thanks for the cleanup!

One general thought: Do you think it's worth having the separate GetVersion()
function on extension registry, rather than using
[GetExtensionById()|<set>().GetByID()]->version()?  The advantage to the latter
is that the include mask is set explicitly by the caller, and if we only have a
few call sites, that might be advantageous (to save the look up for what
extensions are accounted for).  WDYT?

https://codereview.chromium.org/2892843002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_registry.cc (right):

https://codereview.chromium.org/2892843002/diff/1/extensions/browser/extensio...
extensions/browser/extension_registry.cc:46: ExtensionRegistry::BLACKLISTED |
Why not terminated?

https://codereview.chromium.org/2892843002/diff/1/extensions/browser/extensio...
extensions/browser/extension_registry.cc:48: const Extension* old =
GetExtensionById(extension->id(), include_mask);
|old| is no longer accurate

[lazyboy, 2017-05-19 01:38:48]

> One general thought: Do you think it's worth having the separate GetVersion()
> function on extension registry, rather than using
> [GetExtensionById()|<set>().GetByID()]->version()?  The advantage to the
latter
> is that the include mask is set explicitly by the caller,

I see that as a disadvantage though: requiring to specify these include masks
can
easily lead to bugs (as you point out about the TERMINATE case).

 and if we only have a
> few call sites, 
We only have 1

that might be advantageous (to save the look up for what
> extensions are accounted for).  

I'd keep this CL focused on removing chrome/ dependency from runtime_api.
I've added a not over GetVersion function.

https://codereview.chromium.org/2892843002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_registry.cc (right):

https://codereview.chromium.org/2892843002/diff/1/extensions/browser/extensio...
extensions/browser/extension_registry.cc:46: ExtensionRegistry::BLACKLISTED |
On 2017/05/19 01:07:09, Devlin (catching up) wrote:
> Why not terminated?

I don't know, this is copied from ExtensionService::GetExtensionById, with
include_disabled = true.
In order to avoid churn, I've added a todo and will add that separately. I don't
really want to introduce any functionality change in this CL.

https://codereview.chromium.org/2892843002/diff/1/extensions/browser/extensio...
extensions/browser/extension_registry.cc:48: const Extension* old =
GetExtensionById(extension->id(), include_mask);
On 2017/05/19 01:07:09, Devlin (catching up) wrote:
> |old| is no longer accurate

Renamed.

[lazyboy, 2017-05-19 01:38:55]

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

[commit-bot: I haz the power, 2017-05-19 01:40:36]

Dry run: CQ is trying da patch.

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

[Devlin, 2017-05-19 02:04:01]

On 2017/05/19 01:38:48, lazyboy wrote:
> > One general thought: Do you think it's worth having the separate
GetVersion()
> > function on extension registry, rather than using
> > [GetExtensionById()|<set>().GetByID()]->version()?  The advantage to the
> latter
> > is that the include mask is set explicitly by the caller,
> 
> I see that as a disadvantage though: requiring to specify these include masks
> can
> easily lead to bugs (as you point out about the TERMINATE case).
> 
>  and if we only have a
> > few call sites, 
> We only have 1
> 
> that might be advantageous (to save the look up for what
> > extensions are accounted for).  
> 
> I'd keep this CL focused on removing chrome/ dependency from runtime_api.
> I've added a not over GetVersion function.

Sure thing, thanks for the TODOs.  This definitely isn't any worse off than we
currently are.

LGTM % bikesheddery

https://codereview.chromium.org/2892843002/diff/20001/extensions/browser/exte...
File extensions/browser/extension_registry.cc (right):

https://codereview.chromium.org/2892843002/diff/20001/extensions/browser/exte...
extensions/browser/extension_registry.cc:8: #include "base/version.h"
(see header comment)

https://codereview.chromium.org/2892843002/diff/20001/extensions/browser/exte...
extensions/browser/extension_registry.cc:50: return current ?
*current->version() : base::Version();
maybe s/current/extension?  |current| to me still implies a temporal state,
which isn't necessarily the case with this method.

https://codereview.chromium.org/2892843002/diff/20001/extensions/browser/exte...
File extensions/browser/extension_registry.h (right):

https://codereview.chromium.org/2892843002/diff/20001/extensions/browser/exte...
extensions/browser/extension_registry.h:24: class Version;
nit: Since we return a non-pointer for base::Version, don't we need to include
version.h?  (I'm suspecting it's transitively included right now)

https://codereview.chromium.org/2892843002/diff/20001/extensions/browser/exte...
extensions/browser/extension_registry.h:96: base::Version GetVersion(const
Extension* extension) const;
Without the context of this patch, it's *very* why this is different than
calling Extension::version() on the passed-in extension.

What if do something like:
// Returns the current version of the extension with the given |id|, if
// one exists.
// Note: If we are currently updating the extension, this returns the
// version stored currently, rather than the in-progress update.
base::Version GetStoredVersion(const ExtensionId& id);

WDYT?

[commit-bot: I haz the power, 2017-05-19 04:04:49]

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

[commit-bot: I haz the power, 2017-05-19 04:04:51]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-05-19 18:24:18]

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

[lazyboy, 2017-05-19 18:24:55]

https://codereview.chromium.org/2892843002/diff/20001/extensions/browser/exte...
File extensions/browser/extension_registry.cc (right):

https://codereview.chromium.org/2892843002/diff/20001/extensions/browser/exte...
extensions/browser/extension_registry.cc:8: #include "base/version.h"
On 2017/05/19 02:04:00, Devlin (catching up) wrote:
> (see header comment)

Removed.

https://codereview.chromium.org/2892843002/diff/20001/extensions/browser/exte...
extensions/browser/extension_registry.cc:50: return current ?
*current->version() : base::Version();
On 2017/05/19 02:04:00, Devlin (catching up) wrote:
> maybe s/current/extension?  |current| to me still implies a temporal state,
> which isn't necessarily the case with this method.

named registry_extension cause it collides with param otherwise.

https://codereview.chromium.org/2892843002/diff/20001/extensions/browser/exte...
File extensions/browser/extension_registry.h (right):

https://codereview.chromium.org/2892843002/diff/20001/extensions/browser/exte...
extensions/browser/extension_registry.h:24: class Version;
On 2017/05/19 02:04:01, Devlin (catching up) wrote:
> nit: Since we return a non-pointer for base::Version, don't we need to include
> version.h?  (I'm suspecting it's transitively included right now)
Yes, fixed.

https://codereview.chromium.org/2892843002/diff/20001/extensions/browser/exte...
extensions/browser/extension_registry.h:96: base::Version GetVersion(const
Extension* extension) const;
On 2017/05/19 02:04:00, Devlin (catching up) wrote:
> Without the context of this patch, it's *very* why this is different than
> calling Extension::version() on the passed-in extension.
> 
> What if do something like:
> // Returns the current version of the extension with the given |id|, if
> // one exists.
> // Note: If we are currently updating the extension, this returns the
> // version stored currently, rather than the in-progress update.
> base::Version GetStoredVersion(const ExtensionId& id);
> 
> WDYT?
GetStoredVersion is definitely better than GetVersion, which would have kept
readers guessing, thanks!

[commit-bot: I haz the power, 2017-05-19 18:25:25]

Dry run: CQ is trying da patch.

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

[Devlin, 2017-05-19 18:28:24]

https://codereview.chromium.org/2892843002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_registry.h (right):

https://codereview.chromium.org/2892843002/diff/40001/extensions/browser/exte...
extensions/browser/extension_registry.h:94: base::Version GetStoredVersion(const
Extension* extension) const;
s/const Extension* extension/const ExtensionId& id?

[commit-bot: I haz the power, 2017-05-19 19:51:50]

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

[commit-bot: I haz the power, 2017-05-19 19:51:50]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-05-19 20:20:47]

https://codereview.chromium.org/2892843002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_registry.h (right):

https://codereview.chromium.org/2892843002/diff/40001/extensions/browser/exte...
extensions/browser/extension_registry.h:94: base::Version GetStoredVersion(const
Extension* extension) const;
On 2017/05/19 18:28:24, Devlin (catching up) wrote:
> s/const Extension* extension/const ExtensionId& id?

Missed it, Done.

[lazyboy, 2017-05-19 20:20:53]

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

[Devlin, 2017-05-19 20:22:44]

(s lgtm)

[commit-bot: I haz the power, 2017-05-19 20:23:01]

Dry run: 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-05-19 21:26:23]

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

[commit-bot: I haz the power, 2017-05-19 21:26:23]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-05-19 21:29:16]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2017-05-19 21:30:07]

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-05-19 21:37:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495229356703370,
"parent_rev": "6ca68936d643cf5636b9d5af16ed798a57b5c43b", "commit_rev":
"640dc5161179cd9058dd3c97788880a8ac4c7f4d"}

[commit-bot: I haz the power, 2017-05-19 21:37:27]

Description was changed from

==========
Remove RuntimeAPIDelegate::GetPreviousVersion.

We currently go to chrome/ to get the extension version, but we don't
need to. ExtensionRegistry in extensions/ has the info already, move
the function to ExtensionRegistry.

Also, the name GetPreviousVersion is misleading. It only gives us the
current version, so rename it to be simply GetVersion.

BUG=723754
Test=None, internal only change.
==========

to

==========
Remove RuntimeAPIDelegate::GetPreviousVersion.

We currently go to chrome/ to get the extension version, but we don't
need to. ExtensionRegistry in extensions/ has the info already, move
the function to ExtensionRegistry.

Also, the name GetPreviousVersion is misleading. It only gives us the
current version, so rename it to be simply GetVersion.

BUG=723754
Test=None, internal only change.

Review-Url: https://codereview.chromium.org/2892843002
Cr-Commit-Position: refs/heads/master@{#473332}
Committed:
https://chromium.googlesource.com/chromium/src/+/640dc5161179cd9058dd3c977888...
==========

[commit-bot: I haz the power, 2017-05-19 21:37:28]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/640dc5161179cd9058dd3c977888...