History View CRect to gfx Cleanup

History View  CRect to gfx Cleanup

BUG=2186
(http://crbug.com/2186)

[Mohamed Mansour (USE mhm), 2008-12-07 07:36:07]

Hey, made more ctype to gfxtypes switches to history view, some simple
coordinate pane to rectangular pane math has been made (double check if
correct). Should be correct, tested it, and it works.

[Peter Kasting, 2008-12-07 08:32:35]

http://codereview.chromium.org/13236/diff/1/2
File chrome/browser/history_view.cc (right):

http://codereview.chromium.org/13236/diff/1/2#newcode350
Line 350: rect->set_y(kEntryPadding + kThumbnailHeight);
Isn't the y coord the top, not the bottom?  This is the old rect->bottom value.

http://codereview.chromium.org/13236/diff/1/2#newcode375
Line 375: thumbnail_rect.Inset(MirroredLeftPointForRect(mirrored_rect), 0);
I don't know the Rect API, but "Inset()" sounds like a function that makes a
relative move, while "MoveToX()" sounds like one that makes an absolute move...

[Mohamed Mansour (USE mhm), 2008-12-07 10:03:17]

http://codereview.chromium.org/13236/diff/1/2
File chrome/browser/history_view.cc (right):

http://codereview.chromium.org/13236/diff/1/2#newcode350
Line 350: rect->set_y(kEntryPadding + kThumbnailHeight);
On 2008/12/07 08:32:35, pkasting wrote:
> Isn't the y coord the top, not the bottom?  This is the old rect->bottom
value.

Hmm good observation, so should y be kEntryPadding or 0? I had that at the
beginning, but I need to double checked.

But the thumbnails seem working. How should I check?

http://codereview.chromium.org/13236/diff/1/2#newcode375
Line 375: thumbnail_rect.Inset(MirroredLeftPointForRect(mirrored_rect), 0);
On 2008/12/07 08:32:35, pkasting wrote:
> I don't know the Rect API, but "Inset()" sounds like a function that makes a
> relative move, while "MoveToX()" sounds like one that makes an absolute
move...

I checked the method MoveToX and Inset, they are the exact same thing, Inset
moves it wrt to the direction of Rectangular Coordinate plane, same thing with
MoveX but just   on the x axis. 

I doubled checked my answers by paper to ensure both solutions have the same
results.

[Mohamed Mansour (USE mhm), 2008-12-07 17:43:12]

Last time programming after midnight, I woke up this morning, and realized that
I was misunderstanding this problem. 

The coordinate system starts at 0,0 from the top left, we are painting the
thumbnail at the ends of the bounds of the screen. So x should be (width -
thumbnail size - border size). But y should be 0, relative to the pane.

No need to check if the UI layout is right-to-left, because we are using a left
to right layout. If we applied that Offset (MoveToX), it will double the bounds
and the bitmap will be painted off screen.

I have compiled that, and checked if it works by testing the  beta build and my
build. Went to History > Searched for something > and checked the thumbnail
positioning.

It seems working.

Once again sorry for the mistakes :x I am learning gradually

http://codereview.chromium.org/13236/diff/1/2
File chrome/browser/history_view.cc (right):

http://codereview.chromium.org/13236/diff/1/2#newcode350
Line 350: rect->set_y(kEntryPadding + kThumbnailHeight);
On 2008/12/07 08:32:35, pkasting wrote:
> Isn't the y coord the top, not the bottom?  This is the old rect->bottom
value.

Your right, It should be 0

http://codereview.chromium.org/13236/diff/1/2#newcode375
Line 375: thumbnail_rect.Inset(MirroredLeftPointForRect(mirrored_rect), 0);
Your right, It should be thumbnail_rect.Offset. Last time programming after
midnight, sorry for letting you spot problems that should have never existed

[Peter Kasting, 2008-12-07 23:07:13]

http://codereview.chromium.org/13236/diff/7/202
File chrome/browser/history_view.cc (left):

http://codereview.chromium.org/13236/diff/7/202#oldcode375
Line 375: thumbnail_rect.MoveToX(MirroredLeftPointForRect(mirrored_rect));
It can't be true that just by switching from one rect type to another you remove
the need for this call.  Either the call was wrong in the old code or you need
it in the new code.  Same with the removal you did below.

http://codereview.chromium.org/13236/diff/7/202
File chrome/browser/history_view.cc (right):

http://codereview.chromium.org/13236/diff/7/202#newcode350
Line 350: rect->set_y(0);
This still looks wrong, shouldn't it be kEntryPadding?

[Mohamed Mansour (USE mhm), 2008-12-08 00:39:08]

last attempt :/

http://codereview.chromium.org/13236/diff/7/202
File chrome/browser/history_view.cc (right):

http://codereview.chromium.org/13236/diff/7/202#newcode350
Line 350: rect->set_y(0);
On 2008/12/07 23:07:13, pkasting wrote:
> This still looks wrong, shouldn't it be kEntryPadding?

Done.

http://codereview.chromium.org/13236/diff/8/9
File chrome/browser/history_view.cc (left):

http://codereview.chromium.org/13236/diff/8/9#oldcode375
Line 375: thumbnail_rect.MoveToX(MirroredLeftPointForRect(mirrored_rect));
MoveX is just this (according to atlmisc.h):
  right = Width() + x;
  left = x;

Where for gfx::Rect equals:
x = left
y = top
width = right - left
height = bottom - top

So the equivalent MoveX would be:
set_x(x);
set_width(width() + x - x) 

Setting a width is useless since the width never changes,
just the point (x, y) changes.

[Peter Kasting, 2008-12-08 01:58:51]

On 2008/12/08 00:39:08, Mohamed Mansour wrote:
> http://codereview.chromium.org/13236/diff/8/9#oldcode375
> Line 375: thumbnail_rect.MoveToX(MirroredLeftPointForRect(mirrored_rect));
> MoveX is just this (according to atlmisc.h):
>   right = Width() + x;
>   left = x;

But the old code is a gfx::Rect function called MoveToX(), not an atlmisc.h
function MoveX().

It seems pretty clear from context that the code is translating the rect to an
absolute location that's mirrored for RTL.  The precise location is calculated
by MirroredLeftPointForRect().

[Mohamed Mansour (USE mhm), 2008-12-08 02:47:42]

> But the old code is a gfx::Rect function called MoveToX(), not an atlmisc.h
> function MoveX().
> 
> It seems pretty clear from context that the code is translating the rect to an
> absolute location that's mirrored for RTL.  The precise location is calculated
> by MirroredLeftPointForRect().

Sorry, I mistyped MoveX (MoveToX), and CRect is part of atlmisc.h. And the old
code wasn't wan't part of gfx::Rect, it was still using CRect.

CRect thumbnail_loc;
gfx::Rect mirrored_loc(thumbnail_loc);
thumbnail_loc.MoveToX(MirroredLeftPointForRect(mirrored_loc));

Since we needed to convert CRect to gfx::Rect, I removed mirrored_loc since we
don't need to convert to gfx::Rect cause we already have a gfx::Rect, and the
MoveToX for CRect is the same as set_x for gfx::Rect.

Yea, it seems clear that is is translating the rect to an absolute location and
translating that for gfx::Rect only requires set_x and set_y.

Correct?

So LGTM?

[Peter Kasting, 2008-12-08 18:45:40]

On 2008/12/08 02:47:42, Mohamed Mansour wrote:
> Sorry, I mistyped MoveX (MoveToX), and CRect is part of atlmisc.h. And the old
> code wasn't wan't part of gfx::Rect, it was still using CRect.

You're right, I misread the code slightly.

> Yea, it seems clear that is is translating the rect to an absolute location
and
> translating that for gfx::Rect only requires set_x and set_y.

Yes, when I wrote the previous comment I was somehow looking at patch set 2
instead of patch set 3 (where you added the set_x() calls).  Now that they're in
this looks more sane to me.

Over to Ben to comment/commit.

[Peter Kasting, 2009-03-27 18:43:45]

history_view is gone, this change is obsolete.  Closing.