Have the browser process rewrite manifest.json and theme images that the...

Have the browser process rewrite manifest.json and theme/page action images
that the extension unpacker process parsed.

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

[Matt Perry, 2009-05-20 23:56:24]

Note: this depends on http://codereview.chromium.org/115491

[Erik does not do reviews, 2009-05-21 18:28:09]

http://codereview.chromium.org/115595/diff/2001/3002
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/115595/diff/2001/3002#newcode811
Line 811: const DictionaryValue& manifest,
don't we need to write this new manifest out to disk as well?

http://codereview.chromium.org/115595/diff/2001/3002#newcode847
Line 847: if (!PNGEncoder::EncodeBGRASkBitmap(image, false, &image_data)) {
Does this mean that all images must be PNG?  If the original filename was
foo.gif, will our code handle that?

http://codereview.chromium.org/115595/diff/2001/3001
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/115595/diff/2001/3001#newcode42
Line 42: const std::vector< Tuple2<SkBitmap, FilePath> >& images) {}
add #include <vector>

http://codereview.chromium.org/115595/diff/2001/3006
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/115595/diff/2001/3006#newcode220
Line 220: // TODO(mpcomplete): it looks like this isn't actually necessary.
more detail in this comment would be nice

http://codereview.chromium.org/115595/diff/2001/3006#newcode258
Line 258: DictionaryValue* images = extension.GetThemeImages();
what about the page action images?

http://codereview.chromium.org/115595/diff/2001/3006#newcode274
Line 274: if (path.IsAbsolute()) {
also check for ".."

http://codereview.chromium.org/115595/diff/2001/3007
File chrome/common/extensions/extension_unpacker.h (right):

http://codereview.chromium.org/115595/diff/2001/3007#newcode47
Line 47: bool PostProcess();
add comment

http://codereview.chromium.org/115595/diff/2001/3008
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/115595/diff/2001/3008#newcode10
Line 10: #include <utility>
not referenced here?

http://codereview.chromium.org/115595/diff/2001/3009
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/115595/diff/2001/3009#newcode35
Line 35: public:
1 space indent

http://codereview.chromium.org/115595/diff/2001/3009#newcode43
Line 43: private:
1 space indent.  newline before.

[Matt Perry, 2009-05-21 21:01:35]

new snapshot

http://codereview.chromium.org/115595/diff/2001/3002
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/115595/diff/2001/3002#newcode811
Line 811: const DictionaryValue& manifest,
On 2009/05/21 18:28:09, Erik Kay wrote:
> don't we need to write this new manifest out to disk as well?

Oops, thanks!

http://codereview.chromium.org/115595/diff/2001/3002#newcode847
Line 847: if (!PNGEncoder::EncodeBGRASkBitmap(image, false, &image_data)) {
On 2009/05/21 18:28:09, Erik Kay wrote:
> Does this mean that all images must be PNG?  If the original filename was
> foo.gif, will our code handle that?

It looks like our theme code only handles PNGs, while the page action handles
anything that our webkit decoder handles.  Neither one cares about the file
extension.

Still, it is pretty lame to rewrite foo.gif with PNG encoding... I'll file a bug
about doing something less stupid here.

http://codereview.chromium.org/115595/diff/2001/3001
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/115595/diff/2001/3001#newcode42
Line 42: const std::vector< Tuple2<SkBitmap, FilePath> >& images) {}
On 2009/05/21 18:28:09, Erik Kay wrote:
> add #include <vector>

Done.

http://codereview.chromium.org/115595/diff/2001/3006
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/115595/diff/2001/3006#newcode220
Line 220: // TODO(mpcomplete): it looks like this isn't actually necessary.
On 2009/05/21 18:28:09, Erik Kay wrote:
> more detail in this comment would be nice

Done.

http://codereview.chromium.org/115595/diff/2001/3006#newcode258
Line 258: DictionaryValue* images = extension.GetThemeImages();
On 2009/05/21 18:28:09, Erik Kay wrote:
> what about the page action images?

Done.

http://codereview.chromium.org/115595/diff/2001/3006#newcode274
Line 274: if (path.IsAbsolute()) {
On 2009/05/21 18:28:09, Erik Kay wrote:
> also check for ".."

I don't think that's necessary, because if it references a file outside of the
sandboxed directory, reading the file will fail.

http://codereview.chromium.org/115595/diff/2001/3007
File chrome/common/extensions/extension_unpacker.h (right):

http://codereview.chromium.org/115595/diff/2001/3007#newcode47
Line 47: bool PostProcess();
On 2009/05/21 18:28:09, Erik Kay wrote:
> add comment

Done.

http://codereview.chromium.org/115595/diff/2001/3008
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/115595/diff/2001/3008#newcode10
Line 10: #include <utility>
On 2009/05/21 18:28:09, Erik Kay wrote:
> not referenced here?

Done.

http://codereview.chromium.org/115595/diff/2001/3009
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/115595/diff/2001/3009#newcode35
Line 35: public:
On 2009/05/21 18:28:09, Erik Kay wrote:
> 1 space indent

Oops.  Didn't mean to leave this class here.  It's not used.

[Erik does not do reviews, 2009-05-21 22:11:11]

http://codereview.chromium.org/115595/diff/2001/3006
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/115595/diff/2001/3006#newcode274
Line 274: if (path.IsAbsolute()) {
On 2009/05/21 21:01:35, Matt Perry wrote:
> On 2009/05/21 18:28:09, Erik Kay wrote:
> > also check for ".."
> 
> I don't think that's necessary, because if it references a file outside of the
> sandboxed directory, reading the file will fail.

Sure, but we don't want an extension to trick us into reading an arbitrary file
off of the filesystem.  Even though we're protected, there's no reason to do it.

http://codereview.chromium.org/115595/diff/1009/34
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/115595/diff/1009/34#newcode865
Line 865: if (!PNGEncoder::EncodeBGRASkBitmap(image, false, &image_data)) {
add a comment referencing the PNG vs GIF bug

http://codereview.chromium.org/115595/diff/1009/42
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/115595/diff/1009/42#newcode290
Line 290: // TODO(finnur): bug with loading page actions.
reference a bug#?  I assume you meant to leave these commented out.

[Matt Perry, 2009-05-21 23:00:53]

http://codereview.chromium.org/115595/diff/2001/3006
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/115595/diff/2001/3006#newcode274
Line 274: if (path.IsAbsolute()) {
On 2009/05/21 22:11:11, Erik Kay wrote:
> On 2009/05/21 21:01:35, Matt Perry wrote:
> > On 2009/05/21 18:28:09, Erik Kay wrote:
> > > also check for ".."
> > 
> > I don't think that's necessary, because if it references a file outside of
the
> > sandboxed directory, reading the file will fail.
> 
> Sure, but we don't want an extension to trick us into reading an arbitrary
file
> off of the filesystem.  Even though we're protected, there's no reason to do
it.

Done.

http://codereview.chromium.org/115595/diff/1009/42
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/115595/diff/1009/42#newcode290
Line 290: // TODO(finnur): bug with loading page actions.
On 2009/05/21 22:11:12, Erik Kay wrote:
> reference a bug#?  I assume you meant to leave these commented out.

Actually finnur already checked in the fix, so I'll uncomment these.

[Erik does not do reviews, 2009-05-21 23:03:54]

LGTM!