Review request - fix issue 6103 and 13217 -- mirror download item in download shelf

This CL fixes issue 6103 -- Individual download items on the download shelf are not mirrored on RTL locales,
and issue 13217 -- Dangerous File warning on the shelf in RTL seems to misplace the buttons

Enable UI mirroring for DownloadItemView.
During painting each download items, for those items without corresponding mirrored images, flip the canvas to paint the mirrored image. And flip canvas back to normal to paint others.

BUG=http://crbug.com/6103
BUG=http://crbug.com/13217
Test=Open Hebrew Chrome, go to "http://build.chromium.org/buildbot/continuous/LATEST/", download mini_installer.exe, the layout of the warning should be "cancel_button ok_button warning_message warning_icon" from left to right. Click "ok" to download, the layout of the download item in download shelf should be "drop_down_icon download_file_name(or download_file_name with download_status) download_progress_animation and download icon" from left to right.


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

[xji, 2009-06-04 22:00:14]

I do not totally understand the canvas->save() and
canvas->restore() part. 

Especially the 2 canvas->restore() originally exist in
DownloadItemView::Paint(). I thought canvas->save() and canvas->restore() should
be paired, apparently, they are not in the original code. And I did not find
test case the goes through those 2 canvas->restore() functions. I do not know
whether the fix still work if one or both of them is hit.
(It might, please see my next statement that 
canvas->restore() does not restore the canvas back to its normal -- unflipped
stage)

Also, I thought canvas->restore() restores the canvas back to its previous
status, so, instead of "Flip back the flipped canvas", I could just use
"canvas->restore()", which apparently is not the case.

[jeremy, 2009-06-04 22:09:04]

lg in general, adding sky who might be more familiar with this code.

http://codereview.chromium.org/118260/diff/1008/1010
File chrome/browser/views/download_item_view.cc (right):

http://codereview.chromium.org/118260/diff/1008/1010#newcode483
Line 483: // for RTL UI, we flip the canvans to draw those images mirrorly.
canvans->canvas mirrorly->mirrored

http://codereview.chromium.org/118260/diff/1008/1010#newcode556
Line 556: canvas->restore();
I think you can make do with a short comment here such as:
Restore canvas state since we needed to flip it for the RTL case.

[sky, 2009-06-04 22:30:27]

http://codereview.chromium.org/118260/diff/1008/1010
File chrome/browser/views/download_item_view.cc (right):

http://codereview.chromium.org/118260/diff/1008/1010#newcode487
Line 487: canvas->save();
Why are you saving after translating/scaling? Shouldn't you save first, then
translate so that restore reverts back to the original matrix and makes it to
that you don't have to invert the matrix after restoring?

[xji, 2009-06-04 23:22:29]

On 2009/06/04 22:09:04, jeremy wrote:
> lg in general, adding sky who might be more familiar with this code.
> 
> http://codereview.chromium.org/118260/diff/1008/1010
> File chrome/browser/views/download_item_view.cc (right):
> 
> http://codereview.chromium.org/118260/diff/1008/1010#newcode483
> Line 483: // for RTL UI, we flip the canvans to draw those images mirrorly.
> canvans->canvas mirrorly->mirrored

Done.

> 
> http://codereview.chromium.org/118260/diff/1008/1010#newcode556
> Line 556: canvas->restore();
> I think you can make do with a short comment here such as:
> Restore canvas state since we needed to flip it for the RTL case.

I prefer the original one since it explains why restore canvas better.

[xji, 2009-06-04 23:26:26]

Thanks for the review.
Yes, you are right.
Fixed. And please also check the code I added after the original 
canvas->restore().


On 2009/06/04 22:30:27, sky wrote:
> http://codereview.chromium.org/118260/diff/1008/1010
> File chrome/browser/views/download_item_view.cc (right):
> 
> http://codereview.chromium.org/118260/diff/1008/1010#newcode487
> Line 487: canvas->save();
> Why are you saving after translating/scaling? Shouldn't you save first, then
> translate so that restore reverts back to the original matrix and makes it to
> that you don't have to invert the matrix after restoring?

[jeremy, 2009-06-04 23:29:29]

LGTM

[sky, 2009-06-04 23:40:04]

LGTM

[idana, 2009-06-04 23:53:31]

LGTM

[jungshik at Google, 2009-06-04 23:58:22]

LGTM