Issue 71980: Extensions code should use UTF-16 for user-visible Unicode strings

Updating extensions code to use UTF16
Changes std::string error to string16 in various locations of the Extensions file, spanning up the tree from ExtensionErrorReporter and Extension::InitFromValue, with conversion to string16 beginning at extension_l10n_util::LocalizeExtension and Extension::Create.  Later patches can continue/expand the conversion.

BUG=71980
TEST=Run existing unit tests and browser tests, which have been adjusted for string16s.

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

[clintstaley, 2011-12-14 23:44:40]

Thanks for doing this, Devlin.  And I like your plan of cutting it off at a
reasonable level so that we can incrementally change over to string16 as
different devs have time to do so.

[Aaron Boodman, 2011-12-15 02:25:36]

http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
chrome/common/extensions/extension.cc:230: std::string* std_error) {
utf8_error would be a better name.

http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
chrome/common/extensions/extension.cc:240: *std_error = UTF16ToUTF8(error);
I think (and hope) that the error is never populated here, so you can skip this.

http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
chrome/common/extensions/extension.cc:877: // TODO(si): Continue removing
std::string errors and replace with string16
The convention for TODO is that the username inside the parens is whoever can be
asked for more information about the TODO. I think you should use 'aa' (or some
other chromium committer if there is one that is more appropriate for a given
chunk of code) until you have your own @chromium.org.

http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
chrome/common/extensions/extension.cc:878: std::string default_page, std_error;
Each variable should be declared on its own line.

http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
chrome/common/extensions/extension.cc:879: if
(extension_sidebar->HasKey(keys::kSidebarDefaultPage)) {
Sidebar can actually be removed.

I don't know how many of these warm up CLs your team wants to do, but deleting
code from the project is always looked upon very kindly.

crbug.com/107646

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

http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
chrome/common/extensions/extension.h:208: std::string* std_error);
No need to change this. If it makes implementation sense for the name in the cc
file to be different, that's fine. But no need to leak that implementation
detail into the header.

[Devlin Cronin, 2011-12-15 19:36:11]

Thank you for the comments.  All suggested changes should be implemented in the
latest upload.

On 2011/12/15 02:25:36, Aaron Boodman wrote:
>
http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
> File chrome/common/extensions/extension.cc (right):
> 
>
http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
> chrome/common/extensions/extension.cc:230: std::string* std_error) {
> utf8_error would be a better name.
> 
>
http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
> chrome/common/extensions/extension.cc:240: *std_error = UTF16ToUTF8(error);
> I think (and hope) that the error is never populated here, so you can skip
this.
> 
>
http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
> chrome/common/extensions/extension.cc:877: // TODO(si): Continue removing
> std::string errors and replace with string16
> The convention for TODO is that the username inside the parens is whoever can
be
> asked for more information about the TODO. I think you should use 'aa' (or
some
> other chromium committer if there is one that is more appropriate for a given
> chunk of code) until you have your own @chromium.org.
> 
>
http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
> chrome/common/extensions/extension.cc:878: std::string default_page,
std_error;
> Each variable should be declared on its own line.
> 
>
http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
> chrome/common/extensions/extension.cc:879: if
> (extension_sidebar->HasKey(keys::kSidebarDefaultPage)) {
> Sidebar can actually be removed.
> 
> I don't know how many of these warm up CLs your team wants to do, but deleting
> code from the project is always looked upon very kindly.
> 
> crbug.com/107646
> 
>
http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
> File chrome/common/extensions/extension.h (right):
> 
>
http://codereview.chromium.org/8890086/diff/28/chrome/common/extensions/exten...
> chrome/common/extensions/extension.h:208: std::string* std_error);
> No need to change this. If it makes implementation sense for the name in the
cc
> file to be different, that's fine. But no need to leak that implementation
> detail into the header.

[Aaron Boodman, 2011-12-15 20:25:03]

LGTM

Are you provisional commiters? If not I will get you nominated as that will
allow you to commit patches on your own w/o waiting on us.

[commit-bot: I haz the power, 2011-12-15 23:52:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/RDevlin.Cronin@gmail.com/8890086/16003

[commit-bot: I haz the power, 2011-12-15 23:52:29]

Presubmit check for 8890086-16003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
RDevlin.Cronin@gmail.com is not in AUTHORS file. If you are a new contributor,
please visit
http://www.chromium.org/developers/contributing-code and read the "Legal"
section
If you are a chromite, verify the contributor signed the CLA.

Presubmit checks took 4.2s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Aaron Boodman, 2011-12-15 23:52:58]

I'm going to try and let commit queue land (an automated system) land this now.

It will run tests and then land it if they succeed. I've never tried it for a
change I didn't write, so I hope it gets the authorship right.

[M-A Ruel, 2011-12-15 23:53:52]

On 2011/12/15 23:52:58, Aaron Boodman wrote:
> I'm going to try and let commit queue land (an automated system) land this
now.
> 
> It will run tests and then land it if they succeed. I've never tried it for a
> change I didn't write, so I hope it gets the authorship right.

Wait, was this written by a googler?

[commit-bot: I haz the power, 2011-12-15 23:57:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/RDevlin.Cronin@gmail.com/8890086/16003

[Aaron Boodman, 2011-12-16 00:01:04]

On 2011/12/15 23:53:52, Marc-Antoine Ruel wrote:
> On 2011/12/15 23:52:58, Aaron Boodman wrote:
> > I'm going to try and let commit queue land (an automated system) land this
> now.
> > 
> > It will run tests and then land it if they succeed. I've never tried it for
a
> > change I didn't write, so I hope it gets the authorship right.
> 
> Wait, was this written by a googler?

No, Devlin isn't a Googler. I was hoping that the CQ would mark him as the
author, as if I landed it with `git cl dcommit -c`.

It seems it's on the right track since it's asking about the OWNERS file.

[Aaron Boodman, 2011-12-16 01:46:58]

Hi Devlin,

The patch has a few problems. You can see the results from all the bots here:

https://chromium-status.appspot.com/cq/RDevlin.Cronin%40gmail.com/8890086/16003

(I got there by following the link from commit-bot a few comments up).

There are two sets of problems:

1) Compile errors on the clang compiler. You can easily build with the clang
compiler on linux to reproduce this:
http://code.google.com/p/chromium/wiki/Clang. I recommend just always building
with clang. It's got better error reporting.

2) There are some browser_tests failures. Alas, some browser tests always fail.
Regression tests are hard to keep stable. So it might not necessarily be related
to your change. However, the IncorrectManifest1, IncorrectManifest2, and
WebstoreInlineInstallUnpackFailureTest.Test failures showed up on both bots, so
that seems likely to be real. You can reproduce these failures by building the
browser_tests binary, and running it like so:

./out/(Debug|Release)/browser_tests
--gest_filter=WebstoreInlineInstallUnpackFailureTest.Test

./out/(Debug|Release)/browser_tests
--gest_filter=ExtensionWebstorePrivateApiTest.IncorrectManifest*

HTH, find me in #crx if you have more questions.

[M-A Ruel, 2011-12-16 16:06:37]

FTR, just make sure to add yourself in the AUTHORS file, as described in
https://sites.google.com/a/chromium.org/dev/developers/contributing-code#TOC-...

So please upload another patchset with AUTHORS fixed accordingly.

Then, you can use the CQ to have your change committed and properly attributed
to you.

Last note: I'm not sure how you uploaded your patch but chromium-reviews@ wasn't
on the cc list. Make sure your environment is setup correctly, e.g. read the
doc. I've fixed that.

With the AUTHORS file updated, lgtm

[Devlin Cronin, 2011-12-17 01:29:01]

The latest patch should compile on Clang, and passes the pertinent unit and
browser tests (at least, it does on my machine). Ran tests are below.

Unittests:
  ExtensionFromUserScript
  ExtensionServiceTest
  SandboxedExtensionUnpackerTest
  ExtensionUnpackerTest
  ManifestTest

Browsertests:
  ExtensionCrxInstallerTest
  DownloadTest
  WebstoreInlineInstallTest
  WebstoreInlineInstallUnpackFailureTest
  ExtensionWebstorePrivateApiTest
  ExtensionWebstoreGetWebGLStatusTest
  ExtensionWebstorePrivateBundleTest

The authors file has been added.

Thank you for all your thoughtful comments and your patience,
  Devlin

[clintstaley_gmail.com, 2011-12-17 01:40:52]

Did you post any notes on the clang process?  If we each are to do this, 
it'd be nice to benefit from your experience w/it.

Clint

On 12/16/2011 5:29 PM, RDevlin.Cronin@gmail.com wrote:
> The latest patch should compile on Clang, and passes the pertinent 
> unit and
> browser tests (at least, it does on my machine). Ran tests are below.
>
> Unittests:
>   ExtensionFromUserScript
>   ExtensionServiceTest
>   SandboxedExtensionUnpackerTest
>   ExtensionUnpackerTest
>   ManifestTest
>
> Browsertests:
>   ExtensionCrxInstallerTest
>   DownloadTest
>   WebstoreInlineInstallTest
>   WebstoreInlineInstallUnpackFailureTest
>   ExtensionWebstorePrivateApiTest
>   ExtensionWebstoreGetWebGLStatusTest
>   ExtensionWebstorePrivateBundleTest
>
> The authors file has been added.
>
> Thank you for all your thoughtful comments and your patience,
>   Devlin
>
> http://codereview.chromium.org/8890086/
>

[Aaron Boodman, 2011-12-17 07:16:18]

(note to self: sent to try-bots: 8890086#891da6)

[commit-bot: I haz the power, 2011-12-17 08:23:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/RDevlin.Cronin@gmail.com/8890086/32008

[commit-bot: I haz the power, 2011-12-17 09:33:52]

Change committed as 114917

[Aaron Boodman, 2011-12-17 16:55:45]

On Sat, Dec 17, 2011 at 1:33 AM,  <commit-bot@chromium.org> wrote:
> Change committed as 114917

Congrats!

- a