Added checks to handle unsyncable extensions.

Added checks to handle unsyncable extensions.

BUG=49346, 46516
TEST=manual, unit tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53127

[akalin, 2010-07-20 00:19:49]

+tim for sync stuff

[akalin, 2010-07-20 00:21:31]

+asargent for extensions stuff

[asargent_no_longer_on_chrome, 2010-07-20 00:37:57]

Extension part lgtm.

Does this open a small window where the user could be trying to manually install
an extension at the same time that sync has decided it couldn't be synced, and
it will fail?

[akalin (wrong akalin), 2010-07-20 00:43:49]

Hmm you're right, yes it does.

There's no more reliable way to see if an extension was installed via
sync.  If I remove that check, it would mean that existing synced
NPAPI extensions and third-party extensions would be "grandfathered"
in.  Suggestions?

On Mon, Jul 19, 2010 at 5:37 PM,  <asargent@chromium.org> wrote:
> Extension part lgtm.
>
> Does this open a small window where the user could be trying to manually
> install
> an extension at the same time that sync has decided it couldn't be synced,
> and
> it will fail?
>
> http://codereview.chromium.org/2884027/show
>



-- 
Frederick Akalin
Software Engineer

[akalin, 2010-07-20 01:37:10]

+inferno, in case he has any comments

[asargent_no_longer_on_chrome, 2010-07-20 02:10:35]

How large is the window? Would it just be for a few seconds while sync
pushed over knowledge of this extension, or would it permanently prevent the
user from installing it manually?

On Mon, Jul 19, 2010 at 5:43 PM, Fred Akalin <akalin@google.com> wrote:

> Hmm you're right, yes it does.
>
> There's no more reliable way to see if an extension was installed via
> sync.  If I remove that check, it would mean that existing synced
> NPAPI extensions and third-party extensions would be "grandfathered"
> in.  Suggestions?
>
> On Mon, Jul 19, 2010 at 5:37 PM,  <asargent@chromium.org> wrote:
> > Extension part lgtm.
> >
> > Does this open a small window where the user could be trying to manually
> > install
> > an extension at the same time that sync has decided it couldn't be
> synced,
> > and
> > it will fail?
> >
> > http://codereview.chromium.org/2884027/show
> >
>
>
>
> --
> Frederick Akalin
> Software Engineer
>

[tim (not reviewing), 2010-07-20 03:05:31]

LGTM with one question

http://codereview.chromium.org/2884027/diff/11001/10
File chrome/browser/sync/glue/extension_model_associator.cc (right):

http://codereview.chromium.org/2884027/diff/11001/10#newcode154
chrome/browser/sync/glue/extension_model_associator.cc:154: }
It would be cool if we could remove items from the cloud without treating it as
a client delete everywhere... but we cant... sigh.

http://codereview.chromium.org/2884027/diff/11001/10#newcode223
chrome/browser/sync/glue/extension_model_associator.cc:223: if
(!IsExtensionSyncable(*extension)) {
This is getting called in several places... there isn't a higher level spot
where we have the id and could check this by any chance?

[akalin (wrong akalin), 2010-07-20 03:16:09]

It's only for a few seconds; it's the time between sync pushing the
pending extension data to the auto-update service and nudging it, and
the extension being downloaded and installed.

I tested a manual install and was able to do so.

On Mon, Jul 19, 2010 at 7:09 PM, Antony Sargent <asargent@chromium.org> wrote:
> How large is the window? Would it just be for a few seconds while sync
> pushed over knowledge of this extension, or would it permanently prevent the
> user from installing it manually?
>
> On Mon, Jul 19, 2010 at 5:43 PM, Fred Akalin <akalin@google.com> wrote:
>>
>> Hmm you're right, yes it does.
>>
>> There's no more reliable way to see if an extension was installed via
>> sync.  If I remove that check, it would mean that existing synced
>> NPAPI extensions and third-party extensions would be "grandfathered"
>> in.  Suggestions?
>>
>> On Mon, Jul 19, 2010 at 5:37 PM,  <asargent@chromium.org> wrote:
>> > Extension part lgtm.
>> >
>> > Does this open a small window where the user could be trying to manually
>> > install
>> > an extension at the same time that sync has decided it couldn't be
>> > synced,
>> > and
>> > it will fail?
>> >
>> > http://codereview.chromium.org/2884027/show
>> >
>>
>>
>>
>> --
>> Frederick Akalin
>> Software Engineer
>
>



-- 
Frederick Akalin
Software Engineer

[akalin, 2010-07-20 06:05:58]

http://codereview.chromium.org/2884027/diff/11001/10
File chrome/browser/sync/glue/extension_model_associator.cc (right):

http://codereview.chromium.org/2884027/diff/11001/10#newcode223
chrome/browser/sync/glue/extension_model_associator.cc:223: if
(!IsExtensionSyncable(*extension)) {
On 2010/07/20 03:05:31, timsteele wrote:
> This is getting called in several places... there isn't a higher level spot
> where we have the id and could check this by any chance?

The checks with the DFATALs aren't strictly necessary; I'm just being paranoid. 
I think I can move some of the real checks to the ChangeProcessor and convert
the rest in this file to DFATAL checks.  I'll do that.

[akalin, 2010-07-20 18:36:23]

On 2010/07/20 06:05:58, akalin wrote:
> http://codereview.chromium.org/2884027/diff/11001/10
> File chrome/browser/sync/glue/extension_model_associator.cc (right):
> 
> http://codereview.chromium.org/2884027/diff/11001/10#newcode223
> chrome/browser/sync/glue/extension_model_associator.cc:223: if
> (!IsExtensionSyncable(*extension)) {
> On 2010/07/20 03:05:31, timsteele wrote:
> > This is getting called in several places... there isn't a higher level spot
> > where we have the id and could check this by any chance?
> 
> The checks with the DFATALs aren't strictly necessary; I'm just being
paranoid. 
> I think I can move some of the real checks to the ChangeProcessor and convert
> the rest in this file to DFATAL checks.  I'll do that.

Moved a bunch of IsExtensionSyncable() checks earlier to
extension_change_processor.

[akalin, 2010-07-20 22:29:37]

inferno, any more comments?

On 2010/07/20 18:36:23, akalin wrote:
> On 2010/07/20 06:05:58, akalin wrote:
> > http://codereview.chromium.org/2884027/diff/11001/10
> > File chrome/browser/sync/glue/extension_model_associator.cc (right):
> > 
> > http://codereview.chromium.org/2884027/diff/11001/10#newcode223
> > chrome/browser/sync/glue/extension_model_associator.cc:223: if
> > (!IsExtensionSyncable(*extension)) {
> > On 2010/07/20 03:05:31, timsteele wrote:
> > > This is getting called in several places... there isn't a higher level
spot
> > > where we have the id and could check this by any chance?
> > 
> > The checks with the DFATALs aren't strictly necessary; I'm just being
> paranoid. 
> > I think I can move some of the real checks to the ChangeProcessor and
convert
> > the rest in this file to DFATAL checks.  I'll do that.
> 
> Moved a bunch of IsExtensionSyncable() checks earlier to
> extension_change_processor.

[inferno, 2010-07-20 22:39:05]

SGTM, although i am not that much familiar with this code.