Use the non-localized "canonical name" of an extension to write to its sync

Use the non-localized "canonical name" of an extension to write to its sync
data, rather than the localized name.


BUG=123214
TEST=none


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

[Nicolas Zea, 2012-04-13 22:58:35]

Note that re-purposing an existing proto field is not going to fix ping-ponging
if clients are syncing with an older chrome client (which many are). In that
case, the older client is going to want to write the localized name, while the
new client wants to write the canonical name, and you're in the same boat.

Given that, I think the sync guard is necessary.

[not at google - send to devlin, 2012-04-14 00:17:20]

On 2012/04/13 22:58:35, nzea wrote:
> Note that re-purposing an existing proto field is not going to fix
ping-ponging
> if clients are syncing with an older chrome client (which many are). In that
> case, the older client is going to want to write the localized name, while the
> new client wants to write the canonical name, and you're in the same boat.
> 
> Given that, I think the sync guard is necessary.

That's true.

Though keep in mind that "old clients" are pretty rare since everyone get
updated at once; the time frame that you refer to would be pretty small.  We
could theoretically also cherry-pick this change, it's pretty tiny.

In any case, that sounds fine (though these highly stateful control flow things
make me nervous), but we should do it in a separate patch.

[Nicolas Zea, 2012-04-14 00:59:09]

On 2012/04/14 00:17:20, kalman wrote:
> On 2012/04/13 22:58:35, nzea wrote:
> > Note that re-purposing an existing proto field is not going to fix
> ping-ponging
> > if clients are syncing with an older chrome client (which many are). In that
> > case, the older client is going to want to write the localized name, while
the
> > new client wants to write the canonical name, and you're in the same boat.
> > 
> > Given that, I think the sync guard is necessary.
> 
> That's true.
> 
> Though keep in mind that "old clients" are pretty rare since everyone get
> updated at once; the time frame that you refer to would be pretty small.  We
> could theoretically also cherry-pick this change, it's pretty tiny.
> 
> In any case, that sounds fine (though these highly stateful control flow
things
> make me nervous), but we should do it in a separate patch.

A separate patch is fine by me.

That said, although it's normally a small window of time, there are plenty of
users with machines on two different development channels, as well as those who
simply take their sweet time in upgrading. This fix requires both clients be
upgraded, while the guard means only one needs to be upgraded. So although most
would be covered by this fix because they upgrade all their machines at once,
given that this is an infinite spin issue that involves continuous commits to
the server, even a small percentage of people with only one upgraded machine can
cause serious load issues.

In this case though the impact isn't that high due to the language setting
requirement (which very few users fall into), but if another bug were to creep
in that was more common and we didn't have the sync guard, it could become a
much larger issue.

Anyways, this is just a drive-by, no need to wait for my LG.

[Aaron Boodman, 2012-04-15 06:52:17]

Test? (extension_manifests_unittest.cc would be fine)

Also, I agree the guard seems like a wise idea.

http://codereview.chromium.org/10086019/diff/1/chrome/common/extensions/exten...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/10086019/diff/1/chrome/common/extensions/exten...
chrome/common/extensions/extension.h:563: const std::string& canonical_name()
const { return canonical_name_; }
non_localized_name would be more clear I think.

[not at google - send to devlin, 2012-04-15 19:44:57]

Not sure how useful the tests are, but there you go, definitely better than
nothing.

https://chromiumcodereview.appspot.com/10086019/diff/1/chrome/common/extensio...
File chrome/common/extensions/extension.h (right):

https://chromiumcodereview.appspot.com/10086019/diff/1/chrome/common/extensio...
chrome/common/extensions/extension.h:563: const std::string& canonical_name()
const { return canonical_name_; }
On 2012/04/15 06:52:17, Aaron Boodman wrote:
> non_localized_name would be more clear I think.

Done.

[Aaron Boodman, 2012-04-15 19:48:57]

LGTM

[commit-bot: I haz the power, 2012-04-15 19:50:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10086019/6001

[commit-bot: I haz the power, 2012-04-15 23:30:30]

Try job failure for 10086019-6001 (retry) (retry) on win_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-16 00:19:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10086019/6001

[commit-bot: I haz the power, 2012-04-16 02:45:42]

Try job failure for 10086019-6001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-16 06:04:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10086019/6001

[commit-bot: I haz the power, 2012-04-16 07:31:30]

Try job failure for 10086019-6001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "update" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-17 04:07:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10086019/6001

[commit-bot: I haz the power, 2012-04-17 05:13:47]

Change committed as 132543