Added support for animated/nonanimated user image.

Added support for animated/nonanimated user image. There are no
optimizations (for instance, image is still decoded to PNG at each
request) and bug with replaying animation at each click on already
active user pod, but CL is big enough, so we should start review.

BUG=114083
TEST=none


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

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

[Nikita (slow), 2012-05-29 05:46:29]

Ivan seems to be a better reviewer for this code. I'll take a quick look
too.
28.05.2012 21:39 пользователь <ygorshenin@chromium.org> написал:

> Reviewers: Nikita Kostylev,
>
> Description:
> Added support for animated/nonanimated user image. There are no
> optimizations (for instance, image is still decoded to PNG at each
> request) and bug with replaying animation at each click on already
> active user pod, but CL is big enough, so we should start review.
>
> BUG=114083
> TEST=none
>
>
> Please review this at
http://codereview.chromium.**org/10454044/<http://codereview.chromium.org/104...
>
> SVN Base:
http://git.chromium.org/**chromium/src.git@master<http://git.chromium.org/chr...
>
> Affected files:
>  M chrome/browser/automation/**automation_provider_observers_**chromeos.cc
>  M chrome/browser/chromeos/login/**user.h
>  M chrome/browser/chromeos/login/**user.cc
>  A chrome/browser/chromeos/login/**user_image.h
>  A chrome/browser/chromeos/login/**user_image.cc
>  M chrome/browser/chromeos/login/**user_image_loader.h
>  M chrome/browser/chromeos/login/**user_image_loader.cc
>  M chrome/browser/chromeos/login/**user_image_screen.cc
>  M chrome/browser/chromeos/login/**user_manager.h
>  M chrome/browser/chromeos/login/**user_manager_impl.h
>  M chrome/browser/chromeos/login/**user_manager_impl.cc
>  M chrome/browser/image_decoder.h
>  M chrome/browser/image_decoder.**cc
>  M chrome/browser/resources/**chromeos/login/user_pod_row.js
>  M chrome/browser/resources/**options2/chromeos/change_**
> picture_options.js
>  M chrome/browser/ui/webui/**options2/chromeos/change_**
> picture_options_handler2.cc
>  M chrome/browser/ui/webui/**options2/chromeos/user_image_**source2.h
>  M chrome/browser/ui/webui/**options2/chromeos/user_image_**source2.cc
>  M chrome/chrome_browser.gypi
>
>
>

[ygorshenin1, 2012-05-29 09:22:50]

+ Ivan

[Ivan Korotkov, 2012-05-29 10:50:23]

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user.cc (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.cc:30: bool IsGIFImage(const
std::vector<unsigned char>& data) {
Feels like this should part of UserImage.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.cc:31: const char* kGIFStamp = "GIF";
Better: static const char kGIFStamp[] = "GIF";
then you can use sizeof(kGIFStamp)

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.cc:74: gif_image_.swap(tmp);
Why not vector::clear?

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user.h (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.h:69: bool has_gif_image() const { return
has_gif_image_; }
This may be kind of "architecture astronautism" but I suggest using "animated
image" instead of "gif image" in names.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.h:71: bool gif_image(std::vector<unsigned
char>* raw_data) const {
Only trivial inline setters/getters should be unix_hacker_style. Please rename
to Get[GIF|Animated]Image and move to .cc

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.h:124: SkBitmap image_;
It looks like there is some duplication between these 3 fields and UserImage.
Could we just store UserImage instance instead?

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.h:126: std::vector<unsigned char> gif_image_;
Use UserImage::RawImage instead.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user_image.h (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image.h:15: // Wrapper class, that always
contains SkBitmap and constains raw
Please expand what "when possible" means :)

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image.h:21: explicit UserImage(const
SkBitmap& image);
Please document the purpose of overloaded constructors.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image.h:25: const SkBitmap& image() const;
unix_style getters/setters should be inline.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user_image_loader.h (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image_loader.h:37: // If |save_raw_image| is
true, raw image is also passed to callback.
It should probably be |load_raw_image|, right?

http://codereview.chromium.org/10454044/diff/1/chrome/browser/image_decoder.h
File chrome/browser/image_decoder.h (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/image_decoder.h...
chrome/browser/image_decoder.h:43: bool get_image_data(std::vector<unsigned
char>* image_data) const;
Maybe just const vector& get_image_data() const { return image_data_; } ?
Otherwise, this should be GetImageData.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/login/user_pod_row.js (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/user_pod_row.js:112: // |fast swapping
between them (for instance, when user pod
Stray |

http://codereview.chromium.org/10454044/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/user_pod_row.js:112: // |fast swapping
between them (for instance, when user pod
/swapping/switching/

http://codereview.chromium.org/10454044/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/user_pod_row.js:368: if (!this.isGuest)
{
Let's remove this function and move the code to "set userImageIsAnimated"
instead.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/ui/webui/option...
File chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:29: void
ParseRequest(const string& path, string* email, bool* is_image_animated) {
Please use GURL and something like GetValueForKeyInQuery for parsing.

[ygorshenin1, 2012-05-30 12:17:33]

Many thanks! PTAL.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user.cc (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.cc:30: bool IsGIFImage(const
std::vector<unsigned char>& data) {
On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> Feels like this should part of UserImage.

Done.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.cc:31: const char* kGIFStamp = "GIF";
On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> Better: static const char kGIFStamp[] = "GIF";
> then you can use sizeof(kGIFStamp)

Done.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.cc:74: gif_image_.swap(tmp);
To release buffer allocated by vector. Usually, clear or resize methods changes
size of vector, not capacity.

On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> Why not vector::clear?

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user.h (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.h:69: bool has_gif_image() const { return
has_gif_image_; }
On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> This may be kind of "architecture astronautism" but I suggest using "animated
> image" instead of "gif image" in names.

Done.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.h:71: bool gif_image(std::vector<unsigned
char>* raw_data) const {
On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> Only trivial inline setters/getters should be unix_hacker_style. Please rename
> to Get[GIF|Animated]Image and move to .cc

Done.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.h:124: SkBitmap image_;
On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> It looks like there is some duplication between these 3 fields and UserImage.
> Could we just store UserImage instance instead?

Done.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.h:126: std::vector<unsigned char> gif_image_;
As recommended to use UserImage instead of SkBitmap and vector, gif_image_ is
removed.

On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> Use UserImage::RawImage instead.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user_image.h (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image.h:15: // Wrapper class, that always
contains SkBitmap and constains raw
On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> Please expand what "when possible" means :)

Done.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image.h:21: explicit UserImage(const
SkBitmap& image);
On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> Please document the purpose of overloaded constructors.

Done.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image.h:25: const SkBitmap& image() const;
On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> unix_style getters/setters should be inline.

Done.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user_image_loader.h (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image_loader.h:37: // If |save_raw_image| is
true, raw image is also passed to callback.
On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> It should probably be |load_raw_image|, right?

Done.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/image_decoder.h
File chrome/browser/image_decoder.h (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/image_decoder.h...
chrome/browser/image_decoder.h:43: bool get_image_data(std::vector<unsigned
char>* image_data) const;
On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> Maybe just const vector& get_image_data() const { return image_data_; } ?
> Otherwise, this should be GetImageData.

Done.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/login/user_pod_row.js (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/user_pod_row.js:112: // |fast swapping
between them (for instance, when user pod
On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> Stray |

Done.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/user_pod_row.js:368: if (!this.isGuest)
{
In discussion for crbug.com/114083 it was decided to show only animated version
when it possible. So, all that code with image switching is useless now.

On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> Let's remove this function and move the code to "set userImageIsAnimated"
> instead.

http://codereview.chromium.org/10454044/diff/1/chrome/browser/ui/webui/option...
File chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc (right):

http://codereview.chromium.org/10454044/diff/1/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:29: void
ParseRequest(const string& path, string* email, bool* is_image_animated) {
On 2012/05/29 10:50:23, Ivan Korotkov wrote:
> Please use GURL and something like GetValueForKeyInQuery for parsing.

Done.

[Ivan Korotkov, 2012-05-30 13:06:51]

lgtm

http://codereview.chromium.org/10454044/diff/6003/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_image.cc (right):

http://codereview.chromium.org/10454044/diff/6003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_image.cc:44: RawImage().swap(raw_image_);
As discussed offline, this idiom is OK but please add comment like "Clear
|raw_image_|."

[ygorshenin1, 2012-05-30 13:19:14]

+ James

James, I need a review for chrome/browser/ui/webui/options2 and
chrome/browser/resources/options2.

http://codereview.chromium.org/10454044/diff/6003/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_image.cc (right):

http://codereview.chromium.org/10454044/diff/6003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_image.cc:44: RawImage().swap(raw_image_);
On 2012/05/30 13:06:51, Ivan Korotkov wrote:
> As discussed offline, this idiom is OK but please add comment like "Clear
> |raw_image_|."

Done.

[James Hawkins, 2012-05-30 15:18:48]

LGTM with nits.

http://codereview.chromium.org/10454044/diff/14005/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc (right):

http://codereview.chromium.org/10454044/diff/14005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:20: const char
kKeyAnimated[] = "animated";
nit: Document (e.g. rationale, use, etc.).

http://codereview.chromium.org/10454044/diff/14005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:22: void
ParseRequest(const std::string& path,
nit: Document function and parameters.

http://codereview.chromium.org/10454044/diff/14005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:27: if (email) {
The current uses always pass a non-NULL |email|.  I'd remove the check for now.

[Nikita (slow), 2012-05-30 17:19:11]

lgtm

http://codereview.chromium.org/10454044/diff/24/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/user_image.h (right):

http://codereview.chromium.org/10454044/diff/24/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_image.h:17: // images) and user wallpapers.
nit: why wallpaper is mentioned? Is there a plan to use it here?

http://codereview.chromium.org/10454044/diff/24/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_image.h:33: void SetImage(const SkBitmap&
image);
nit: Add comment that setting image would reset raw_image_ as well.

http://codereview.chromium.org/10454044/diff/24/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc (right):

http://codereview.chromium.org/10454044/diff/24/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:7: #include
"base/logging.h"
nit: not needed

[ygorshenin1, 2012-05-31 11:17:46]

Many thanks!

http://codereview.chromium.org/10454044/diff/14005/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc (right):

http://codereview.chromium.org/10454044/diff/14005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:20: const char
kKeyAnimated[] = "animated";
On 2012/05/30 15:18:48, James Hawkins wrote:
> nit: Document (e.g. rationale, use, etc.).

Done.

http://codereview.chromium.org/10454044/diff/14005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:22: void
ParseRequest(const std::string& path,
On 2012/05/30 15:18:48, James Hawkins wrote:
> nit: Document function and parameters.

Done.

http://codereview.chromium.org/10454044/diff/14005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:27: if (email) {
On 2012/05/30 15:18:48, James Hawkins wrote:
> The current uses always pass a non-NULL |email|.  I'd remove the check for
now.

Done.

http://codereview.chromium.org/10454044/diff/24/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/user_image.h (right):

http://codereview.chromium.org/10454044/diff/24/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_image.h:17: // images) and user wallpapers.
As UserImageLoader is used to load both wallpapers and user images, it interface
was changed to return UserImage instead of SkBitmap. So, in some cases UserImage
wraps user's wallpaper.

On 2012/05/30 17:19:11, Nikita Kostylev wrote:
> nit: why wallpaper is mentioned? Is there a plan to use it here?

http://codereview.chromium.org/10454044/diff/24/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_image.h:33: void SetImage(const SkBitmap&
image);
On 2012/05/30 17:19:11, Nikita Kostylev wrote:
> nit: Add comment that setting image would reset raw_image_ as well.

Done.

http://codereview.chromium.org/10454044/diff/24/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc (right):

http://codereview.chromium.org/10454044/diff/24/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:7: #include
"base/logging.h"
On 2012/05/30 17:19:11, Nikita Kostylev wrote:
> nit: not needed

Done.

[commit-bot: I haz the power, 2012-05-31 11:29:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/10454044/26

[commit-bot: I haz the power, 2012-05-31 13:49:44]

Change committed as 139779

[ygorshenin1, 2012-06-01 14:18:21]

On 2012/05/31 13:49:44, I haz the power (commit-bot) wrote:
> Change committed as 139779

Sorry that I was inattentive. As that CL was reverted, could you PTAL on the
latest patch set?

[James Hawkins, 2012-06-01 21:03:33]

On 2012/06/01 14:18:21, ygorshenin1 wrote:
> On 2012/05/31 13:49:44, I haz the power (commit-bot) wrote:
> > Change committed as 139779
> 
> Sorry that I was inattentive. As that CL was reverted, could you PTAL on the
> latest patch set?

Who are you asking this of?

[ygorshenin1, 2012-06-03 19:36:17]

James, could you please take another look on the changes in
chrome/browser/ui/webui/* part?


On 2012/06/01 21:03:33, James Hawkins wrote:
> On 2012/06/01 14:18:21, ygorshenin1 wrote:
> > On 2012/05/31 13:49:44, I haz the power (commit-bot) wrote:
> > > Change committed as 139779
> > 
> > Sorry that I was inattentive. As that CL was reverted, could you PTAL on the
> > latest patch set?
> 
> Who are you asking this of?

[James Hawkins, 2012-06-04 15:50:13]

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_url_data_manager_backend.cc (right):

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:174: // Used to
notify us that the data mime type is ready for us.
nit: Don't use pronouns (us) in comments; pronouns are ambiguous.

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:174: // Used to
notify us that the data mime type is ready for us.
s/mime type/|mime_type|/

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:335: namespace {
There's already an unnamed namespace at the top of the file.  Please move this
block to that namespace.

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:337: void
GetMimeTypeOnUI(ChromeURLDataManager::DataSource* source,
nit: Document method and parameters.

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc (right):

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:37: if
(parsed.username.is_valid() && parsed.host.is_valid()) {
Optional nit: Save indentation (and lines of code) by reversing the logic and
returning early.

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:46: if
(parsed.query.is_valid()) {
Optional nit: Same thing here about saving indentation.

[ygorshenin1, 2012-06-09 09:34:26]

PTAL

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_url_data_manager_backend.cc (right):

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:174: // Used to
notify us that the data mime type is ready for us.
On 2012/06/04 15:50:13, James Hawkins wrote:
> s/mime type/|mime_type|/

Done.

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:174: // Used to
notify us that the data mime type is ready for us.
On 2012/06/04 15:50:13, James Hawkins wrote:
> nit: Don't use pronouns (us) in comments; pronouns are ambiguous.

Done.

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:335: namespace {
On 2012/06/04 15:50:13, James Hawkins wrote:
> There's already an unnamed namespace at the top of the file.  Please move this
> block to that namespace.

It can't be moved before URLRequestChromeJob declaration because it references
to URLRequestChromeJob::MimeTypeAvailable.

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:337: void
GetMimeTypeOnUI(ChromeURLDataManager::DataSource* source,
On 2012/06/04 15:50:13, James Hawkins wrote:
> nit: Document method and parameters.

Done.

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc (right):

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:37: if
(parsed.username.is_valid() && parsed.host.is_valid()) {
On 2012/06/04 15:50:13, James Hawkins wrote:
> Optional nit: Save indentation (and lines of code) by reversing the logic and
> returning early.

Done.

http://codereview.chromium.org/10454044/diff/7008/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/user_image_source2.cc:46: if
(parsed.query.is_valid()) {
On 2012/06/04 15:50:13, James Hawkins wrote:
> Optional nit: Same thing here about saving indentation.

Done.

[James Hawkins, 2012-06-12 17:04:15]

LGTM

[commit-bot: I haz the power, 2012-06-12 17:53:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/10454044/29002

[commit-bot: I haz the power, 2012-06-12 19:52:36]

Change committed as 141716