Enable forced extension updates on NaCl arch mismatch.

Enable forced extension updates on NaCl arch mismatch.

This causes ExtensionUpdater to scan the contents of
installed extensions prior to initiating update queries. If
any extensions are found with a _platform_specific
directory that does NOT contain a subdirectory which
corresponds to the current NaCl arch, that extension is
placed into forced-update mode.

An extension in forced-update mode will be treated as
version 0.0.0.0 when querying the update server.

BUG=409948
Committed: https://crrev.com/b864d8b8a8d837bb844f6ac2de64ad4fd0899881
Cr-Commit-Position: refs/heads/master@{#293548}

[Ken Rockot(use gerrit already), 2014-09-03 23:59:33]

Patchset #1 (id:1) has been deleted

[Ken Rockot(use gerrit already), 2014-09-04 00:02:13]

rockot@chromium.org changed reviewers:
+ yoz@chromium.org

[Ken Rockot(use gerrit already), 2014-09-04 00:02:14]

Yoyo could you take a look at this? It's a bit different from the last CL. This
fixes the threading/IO issues that had, and I think InstalledLoader is a better
place to do this collection anyway. Thanks

[Yoyo Zhou, 2014-09-04 02:14:24]

https://chromiumcodereview.appspot.com/540673002/diff/20001/extensions/common...
File extensions/common/extension.h (right):

https://chromiumcodereview.appspot.com/540673002/diff/20001/extensions/common...
extensions/common/extension.h:356: void SetPlatformSpecificResourceArchs(const
std::set<std::string>& archs);
Does this really need to be stored on Extension? I don't think we should be
adding stuff like this in general to it. I think it'd be cleaner to have some
sort of PlatformSpecificArchChecker that the InstalledLoader and the downloader
both make use of. If that doesn't work, I think even InstalledLoader would be a
better place to store this data...

[Ken Rockot(use gerrit already), 2014-09-04 03:00:32]

On 2014/09/04 02:14:24, Yoyo Zhou wrote:
>
https://chromiumcodereview.appspot.com/540673002/diff/20001/extensions/common...
> File extensions/common/extension.h (right):
> 
>
https://chromiumcodereview.appspot.com/540673002/diff/20001/extensions/common...
> extensions/common/extension.h:356: void SetPlatformSpecificResourceArchs(const
> std::set<std::string>& archs);
> Does this really need to be stored on Extension? I don't think we should be
> adding stuff like this in general to it. I think it'd be cleaner to have some
> sort of PlatformSpecificArchChecker that the InstalledLoader and the
downloader
> both make use of. If that doesn't work, I think even InstalledLoader would be
a
> better place to store this data...

Hmm, that's a fair point. How would you feel about ExtensionPrefs holding this
data? A separate PlatformSpecificArchAnything seems like overkill. Though
ExtensionPrefs is already incredibly overloaded...

[Yoyo Zhou, 2014-09-04 07:39:33]

On 2014/09/04 03:00:32, Ken Rockot wrote:
> On 2014/09/04 02:14:24, Yoyo Zhou wrote:
> >
>
https://chromiumcodereview.appspot.com/540673002/diff/20001/extensions/common...
> > File extensions/common/extension.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/540673002/diff/20001/extensions/common...
> > extensions/common/extension.h:356: void
SetPlatformSpecificResourceArchs(const
> > std::set<std::string>& archs);
> > Does this really need to be stored on Extension? I don't think we should be
> > adding stuff like this in general to it. I think it'd be cleaner to have
some
> > sort of PlatformSpecificArchChecker that the InstalledLoader and the
> downloader
> > both make use of. If that doesn't work, I think even InstalledLoader would
be
> a
> > better place to store this data...
> 
> Hmm, that's a fair point. How would you feel about ExtensionPrefs holding this
> data? A separate PlatformSpecificArchAnything seems like overkill. Though
> ExtensionPrefs is already incredibly overloaded...

How about ExtensionUpdater? While ExtensionPrefs is possible, it's not really a
pref, and I'd also prefer if it had less specific knowledge about things.

[Ken Rockot(use gerrit already), 2014-09-04 16:41:36]

While this information is currently only used for update logic, it may turn
out to be more generally useful in the future. This is really about
providing general-purpose metadata on an extension without having to touch
the filesystem. IMO the right solution would actually be for the webstore
to add the platform-specific arch info to the manifest when generating
different platform CRXs, and then we get the information for free on
extension load without having to scan the filesystem - but that's probably
not going to happen in the immediate future.

+Chary: Do you think it would be possible in the future for the Webstore to
modify the manifest to include some "platform_specific_archs" field which
lists any architectures present in a generated platform CRX package?

If we can get this into the manifest eventually, I would feel comfortable
tying this logic to ExtensionUpdater in the interim.


On Thu, Sep 4, 2014 at 12:39 AM, <yoz@chromium.org> wrote:

> On 2014/09/04 03:00:32, Ken Rockot wrote:
>
>> On 2014/09/04 02:14:24, Yoyo Zhou wrote:
>> >
>>
>
> https://chromiumcodereview.appspot.com/540673002/diff/
> 20001/extensions/common/extension.h
>
>> > File extensions/common/extension.h (right):
>> >
>> >
>>
>
> https://chromiumcodereview.appspot.com/540673002/diff/
> 20001/extensions/common/extension.h#newcode356
>
>> > extensions/common/extension.h:356: void
>>
> SetPlatformSpecificResourceArchs(const
>
>> > std::set<std::string>& archs);
>> > Does this really need to be stored on Extension? I don't think we
>> should be
>> > adding stuff like this in general to it. I think it'd be cleaner to have
>>
> some
>
>> > sort of PlatformSpecificArchChecker that the InstalledLoader and the
>> downloader
>> > both make use of. If that doesn't work, I think even InstalledLoader
>> would
>>
> be
>
>> a
>> > better place to store this data...
>>
>
>  Hmm, that's a fair point. How would you feel about ExtensionPrefs holding
>> this
>> data? A separate PlatformSpecificArchAnything seems like overkill. Though
>> ExtensionPrefs is already incredibly overloaded...
>>
>
> How about ExtensionUpdater? While ExtensionPrefs is possible, it's not
> really a
> pref, and I'd also prefer if it had less specific knowledge about things.
>
> https://chromiumcodereview.appspot.com/540673002/
>

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

[chromium-reviews, 2014-09-04 17:05:26]

We tend to keep the manifest intact as much as possible. However we also
have the _metadata folder for such purpose. How about the name
metadata.json in _metadata folder?


On Thu, Sep 4, 2014 at 9:41 AM, Ken Rockot <rockot@chromium.org> wrote:

> While this information is currently only used for update logic, it may
> turn out to be more generally useful in the future. This is really about
> providing general-purpose metadata on an extension without having to touch
> the filesystem. IMO the right solution would actually be for the webstore
> to add the platform-specific arch info to the manifest when generating
> different platform CRXs, and then we get the information for free on
> extension load without having to scan the filesystem - but that's probably
> not going to happen in the immediate future.
>
> +Chary: Do you think it would be possible in the future for the Webstore
> to modify the manifest to include some "platform_specific_archs" field
> which lists any architectures present in a generated platform CRX package?
>
> If we can get this into the manifest eventually, I would feel comfortable
> tying this logic to ExtensionUpdater in the interim.
>
>
> On Thu, Sep 4, 2014 at 12:39 AM, <yoz@chromium.org> wrote:
>
>> On 2014/09/04 03:00:32, Ken Rockot wrote:
>>
>>> On 2014/09/04 02:14:24, Yoyo Zhou wrote:
>>> >
>>>
>>
>> https://chromiumcodereview.appspot.com/540673002/diff/
>> 20001/extensions/common/extension.h
>>
>>> > File extensions/common/extension.h (right):
>>> >
>>> >
>>>
>>
>> https://chromiumcodereview.appspot.com/540673002/diff/
>> 20001/extensions/common/extension.h#newcode356
>>
>>> > extensions/common/extension.h:356: void
>>>
>> SetPlatformSpecificResourceArchs(const
>>
>>> > std::set<std::string>& archs);
>>> > Does this really need to be stored on Extension? I don't think we
>>> should be
>>> > adding stuff like this in general to it. I think it'd be cleaner to
>>> have
>>>
>> some
>>
>>> > sort of PlatformSpecificArchChecker that the InstalledLoader and the
>>> downloader
>>> > both make use of. If that doesn't work, I think even InstalledLoader
>>> would
>>>
>> be
>>
>>> a
>>> > better place to store this data...
>>>
>>
>>  Hmm, that's a fair point. How would you feel about ExtensionPrefs
>>> holding this
>>> data? A separate PlatformSpecificArchAnything seems like overkill. Though
>>> ExtensionPrefs is already incredibly overloaded...
>>>
>>
>> How about ExtensionUpdater? While ExtensionPrefs is possible, it's not
>> really a
>> pref, and I'd also prefer if it had less specific knowledge about things.
>>
>> https://chromiumcodereview.appspot.com/540673002/
>>
>
>


-- 
If you received this communication by mistake, please don't forward it to
anyone else (it may contain confidential or privileged information), please
erase all copies of it, including all attachments, and please let the
sender know it went to the wrong person. Thanks.

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

[Ken Rockot(use gerrit already), 2014-09-04 20:23:27]

Patchset #4 (id:80001) has been deleted

[Ken Rockot(use gerrit already), 2014-09-04 20:23:32]

Patchset #3 (id:60001) has been deleted

[Ken Rockot(use gerrit already), 2014-09-04 20:23:38]

Patchset #2 (id:40001) has been deleted

[Ken Rockot(use gerrit already), 2014-09-04 20:24:07]

Alright, how's this look? :)

[Yoyo Zhou, 2014-09-04 22:41:26]

LGTM

https://chromiumcodereview.appspot.com/540673002/diff/120001/chrome/browser/e...
File chrome/browser/extensions/updater/extension_updater.cc (right):

https://chromiumcodereview.appspot.com/540673002/diff/120001/chrome/browser/e...
chrome/browser/extensions/updater/extension_updater.cc:109: const
base::Callback<void(const std::set<std::string>&)>& callback) {
Consider DCHECKing that we're on a non-UI thread.

https://chromiumcodereview.appspot.com/540673002/diff/120001/chrome/browser/e...
chrome/browser/extensions/updater/extension_updater.cc:157:
CollectExtensionsFromSet(registry->enabled_extensions(), extensions.get());
Why not use registry->GenerateInstalledExtensionsSet?

https://chromiumcodereview.appspot.com/540673002/diff/120001/chrome/browser/e...
File chrome/browser/extensions/updater/extension_updater.h (right):

https://chromiumcodereview.appspot.com/540673002/diff/120001/chrome/browser/e...
chrome/browser/extensions/updater/extension_updater.h:290: // update check.
This comment should indicate that the set is of extension IDs.

[Ken Rockot(use gerrit already), 2014-09-05 01:41:34]

https://codereview.chromium.org/540673002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_updater.cc (right):

https://codereview.chromium.org/540673002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_updater.cc:109: const
base::Callback<void(const std::set<std::string>&)>& callback) {
On 2014/09/04 22:41:26, Yoyo Zhou wrote:
> Consider DCHECKing that we're on a non-UI thread.

Done.

https://codereview.chromium.org/540673002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_updater.cc:157:
CollectExtensionsFromSet(registry->enabled_extensions(), extensions.get());
On 2014/09/04 22:41:26, Yoyo Zhou wrote:
> Why not use registry->GenerateInstalledExtensionsSet?

Because I totally missed it when looking over the class to find something that
does exactly what it does. :)

Done.

https://codereview.chromium.org/540673002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_updater.h (right):

https://codereview.chromium.org/540673002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_updater.h:290: // update check.
On 2014/09/04 22:41:26, Yoyo Zhou wrote:
> This comment should indicate that the set is of extension IDs.

Done.

[Ken Rockot(use gerrit already), 2014-09-05 01:41:45]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-09-05 01:46:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/540673002/140001

[commit-bot: I haz the power, 2014-09-05 05:14:40]

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

[commit-bot: I haz the power, 2014-09-05 05:14:41]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ken Rockot(use gerrit already), 2014-09-05 16:24:39]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-09-05 16:25:09]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-09-05 16:28:17]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-09-05 16:29:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/540673002/160001

[commit-bot: I haz the power, 2014-09-05 17:33:35]

Committed patchset #5 (id:160001) as f1f26f8bc9f206da4bff4ec0f611676953ad1ae7

[commit-bot: I haz the power, 2014-09-10 03:40:29]

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