Remove "theme" in extension/app unpacker error messages, make those messages localizable, and add u…

Remove "theme" in extension/app unpacker error messages, make those messages localizable, and add unit tests for them.

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

[dharcourt, 2012-08-23 18:48:33]

This is more of a request for education since this is my first Chromium C++
code, and the first C++ code I write in 18 years... In addition to catching the
usual bugs, please find in these changes as many ways as possible to fill me in
on style and other things I'm missing.

Thanks!

    - Charles

PS: The added unit tests do not test whether for localizability of error
messages, and no effort was made to refactor the unit tests to reduce
boilerplate code and code duplication. Let me know of course if any of this
should be reconsidered.

[dharcourt, 2012-08-23 18:57:16]

FYI, I uploaded a second patch set. The only difference with the first patch set
is that trailing periods where removed from error messages because these
messages get included before being shown to users in another message that has a
period of its own, so if they have periods the message shown to users will have
two periods.

[asargent_no_longer_on_chrome, 2012-08-23 22:16:36]

https://chromiumcodereview.appspot.com/10868050/diff/3002/chrome/common/exten...
File chrome/common/extensions/unpacker.cc (right):

https://chromiumcodereview.appspot.com/10868050/diff/3002/chrome/common/exten...
chrome/common/extensions/unpacker.cc:157:
base::i18n::WrapPathWithLTRFormatting(temp_install_dir_, &dir_string);
As discussed offline, it would be interesting to figure out why no one else in
the codebase is using this function. Is it because it is new, or because it is
old? If it turns out that it's old, then we'll want to remove that comment from
file_path.h (and perhaps the function itself). 

I just noticed that it looks like what gets used alot in other places in the
codebase is the FilePath::LossyDisplayName function. Perhaps you should just use
that here instead.

https://chromiumcodereview.appspot.com/10868050/diff/3002/chrome/common/exten...
File chrome/common/extensions/unpacker_unittest.cc (right):

https://chromiumcodereview.appspot.com/10868050/diff/3002/chrome/common/exten...
chrome/common/extensions/unpacker_unittest.cc:212: false)) << "Expected prefix:
\"" << kExpected <<
nit: the chrome-specific style guide encourages a slightly different technique
for wrapping logging lines like this:

http://dev.chromium.org/developers/coding-style#TOC-Code-formatting

"When you have a log line that is longer than 80 characters, subsequent lines
should start with the << operator and should be aligned based on the first <<
from the original line"

[dharcourt, 2012-08-27 17:55:55]

Incorporated suggestions from Patch 2 reviewer comments, and also added
path/file information to a couple of error messages that could benefit from it.

https://chromiumcodereview.appspot.com/10868050/diff/7003/chrome/common/exten...
File chrome/common/extensions/unpacker.cc (right):

https://chromiumcodereview.appspot.com/10868050/diff/7003/chrome/common/exten...
chrome/common/extensions/unpacker.cc:151: // <profile>/Extensions/CRX_INSTALL
Corrected comment: Temporary installation directories are now called CRX_INSTALL
instead of INSTALL_TEMP and don't contain an extension version in their path
(they never did :-).

https://chromiumcodereview.appspot.com/10868050/diff/7003/chrome/common/exten...
chrome/common/extensions/unpacker.cc:153:
extension_path_.DirName().AppendASCII(filenames::kTempExtensionName);
Corrected indentation: 4 spaces for continuation lines (unless I'm mistaken?)

https://chromiumcodereview.appspot.com/10868050/diff/7003/chrome/common/exten...
chrome/common/extensions/unpacker.cc:160:
temp_install_dir_.LossyDisplayName())));
Replacing the not-used-anywhere-else WrapPathWithLTRFormatting() with a
combination of LossyDisplayName() and GetDisplayStringInLTRDirectionality().
This also has the benefit of making the intent clearer (this is a lossy string
for display to the user, not a totally faithful representation of the path).

https://chromiumcodereview.appspot.com/10868050/diff/7003/chrome/common/exten...
chrome/common/extensions/unpacker.cc:165:
SetUTF16Error(l10n_util::GetStringUTF16(IDS_EXTENSION_PACKAGE_UNZIP_ERROR));
Made error message localizable.

https://chromiumcodereview.appspot.com/10868050/diff/7003/chrome/common/exten...
chrome/common/extensions/unpacker.cc:277: path.LossyDisplayName())));
Added path to the error message because that could be useful to CRX developers.

https://chromiumcodereview.appspot.com/10868050/diff/7003/chrome/common/exten...
chrome/common/extensions/unpacker.cc:287: path.BaseName().LossyDisplayName())));
Added the name of the problematic image to the error message because that will
probably be useful to CRX developers.

https://chromiumcodereview.appspot.com/10868050/diff/7003/chrome/common/exten...
chrome/common/extensions/unpacker.cc:336: error_message_ = error;
Added to avoid unnecessary UTF16->UTF8->UTF16 conversions.

https://chromiumcodereview.appspot.com/10868050/diff/7003/chrome/common/exten...
File chrome/common/extensions/unpacker_unittest.cc (right):

https://chromiumcodereview.appspot.com/10868050/diff/7003/chrome/common/exten...
chrome/common/extensions/unpacker_unittest.cc:214: << "\"";
Corrected formatting (<< at the beginning of lines instead of end).

https://chromiumcodereview.appspot.com/10868050/diff/7003/chrome/common/exten...
chrome/common/extensions/unpacker_unittest.cc:241: const char* kExpected =
"Illegal path (absolute or relative with '..'): ";
Tweaked the message to make it more compatible with including the path name.

https://chromiumcodereview.appspot.com/10868050/diff/7003/chrome/common/exten...
chrome/common/extensions/unpacker_unittest.cc:268: << "\"";
Moved the ImageDecodingError test in this patch to match the order of the tests
match the order of the corresponding code.

[asargent_no_longer_on_chrome, 2012-08-27 18:06:23]

LGTM

[commit-bot: I haz the power, 2012-08-27 18:08:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/10868050/7003

[commit-bot: I haz the power, 2012-08-27 18:08:36]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 404: Not Found

[commit-bot: I haz the power, 2012-08-27 18:10:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/10868050/7003

[commit-bot: I haz the power, 2012-08-27 18:10:13]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 404: Not Found

[dharcourt, 2012-08-31 17:50:01]

FYI, I'm re-committing this after Antony submitted the binaries as a separate
manual commit (https://chromiumcodereview.appspot.com/10870116). The source code
has not changed but the binary files were removed from the CL. Unlike the
previous commit, this commit should trybot and commit without error... let's
see...

[commit-bot: I haz the power, 2012-08-31 17:50:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/10868050/18001

[commit-bot: I haz the power, 2012-08-31 21:32:34]

Try job failure for 10868050-18001 (retry) on linux_chromeos for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-31 21:47:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/10868050/18001

[commit-bot: I haz the power, 2012-09-01 02:40:10]

Change committed as 154580