Closed Bug 878874 Opened 7 years ago Closed 7 years ago

Introduce CSSIntPoint and use it for HTMLImageElement::GetXY()

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

Attachments

(2 files)

Attached patch Patch v1Splinter Review
No description provided.
Attachment #757479 - Flags: review?(bugmail.mozilla)
Comment on attachment 757479 [details] [diff] [review]
Patch v1

Review of attachment 757479 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/Units.h
@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public

All of the other files in this folder have tab-width: 2 or tab-width: 20. Is there a reason you want to change it to 8? My impression was that setting it something large like 20 makes it obvious when tab characters are inserted into the file.

@@ +56,2 @@
>  typedef gfx::PointTyped<CSSPixel> CSSPoint;
> +typedef gfx::PointTyped<CSSIntPixel> CSSIntPoint;

You shouldn't need the CSSIntPixel struct here. If you look at the latest version of layout/base/Units.h in inbound (I landed some changes this morning) you'll see the pattern more clearly. CSSIntPoint should be defined like so:

typedef gfx::IntPointTyped<CSSPixel> CSSIntPoint;

You can also add the conversion functions with the following signatures to the existing CSSPixel class:

static gfx::IntPointTyped<CSSPixel> FromAppUnits(const nsPoint& pt);
static nsPoint ToAppUnits(const gfx::IntPointTyped<CSSPixel>& pt);

which can be used like so:

CSSIntPoint foo = CSSIntPoint::FromAppUnits(bar);
nsPoint bar = CSSIntPoint::ToAppUnits(foo);

I would also prefer to avoid the private functions here since I think it makes the code a bit harder to read.
Attachment #757479 - Flags: review?(bugmail.mozilla) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> Comment on attachment 757479 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 757479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/Units.h
> @@ +1,3 @@
> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> All of the other files in this folder have tab-width: 2 or tab-width: 20. Is
> there a reason you want to change it to 8? My impression was that setting it
> something large like 20 makes it obvious when tab characters are inserted
> into the file.

I needed to add a vim modeline because emacs tab-width alone messes with my editor, so I copied the standard modelines from <https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style>.

> @@ +56,2 @@
> >  typedef gfx::PointTyped<CSSPixel> CSSPoint;
> > +typedef gfx::PointTyped<CSSIntPixel> CSSIntPoint;
> 
> You shouldn't need the CSSIntPixel struct here. If you look at the latest
> version of layout/base/Units.h in inbound (I landed some changes this
> morning) you'll see the pattern more clearly. CSSIntPoint should be defined
> like so:
> 
> typedef gfx::IntPointTyped<CSSPixel> CSSIntPoint;
> 
> You can also add the conversion functions with the following signatures to
> the existing CSSPixel class:
> 
> static gfx::IntPointTyped<CSSPixel> FromAppUnits(const nsPoint& pt);

This won't work, we already have

static gfx::PointTyped<CSSPixel> FromAppUnits(const nsPoint &pt);

> static nsPoint ToAppUnits(const gfx::IntPointTyped<CSSPixel>& pt);
> 
> which can be used like so:
> 
> CSSIntPoint foo = CSSIntPoint::FromAppUnits(bar);
> nsPoint bar = CSSIntPoint::ToAppUnits(foo);
> 
> I would also prefer to avoid the private functions here since I think it
> makes the code a bit harder to read.

I found it easier than copying the same call several times, but sure.
(In reply to :Ms2ger from comment #2)
> I needed to add a vim modeline because emacs tab-width alone messes with my
> editor, so I copied the standard modelines from
> <https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style>.

Ok.

> > static gfx::IntPointTyped<CSSPixel> FromAppUnits(const nsPoint& pt);
> 
> This won't work, we already have
> 
> static gfx::PointTyped<CSSPixel> FromAppUnits(const nsPoint &pt);

Ah, sorry. In that case the name should indicate what sort of rounding is being used, since converting from nsPoint to a IntPointTyped<CSSPixel> necessarily involves some sort of rounding. In this case the name would be "FromAppUnitsRounded" for consistency with other methods (e.g. LayerPixel::FromCSSRectRounded).
Attached patch Patch v2Splinter Review
Attachment #758171 - Flags: review?(bugmail.mozilla)
Comment on attachment 758171 [details] [diff] [review]
Patch v2

Review of attachment 758171 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

::: content/html/content/src/HTMLImageElement.cpp
@@ +167,4 @@
>    }
>  
>    nsIFrame* layer = nsLayoutUtils::GetClosestLayer(frame->GetParent());
> +  return CSSPixel::FromAppUnitsRounded(frame->GetOffsetTo(layer));

Here I would s/CSSPixel/CSSIntPoint/ just for a tad more clarity. Both work fine but since the function is creating and returning a CSSIntPoint I think it makes more sense to use that here. Also it makes it more future-proof if we ever end up splitting CSSIntPoint to be a separate class all on its own.
Attachment #758171 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/67a6841cbe3d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
The rounding should be consistent with (preferably, actually use) nsPoint::ToNearestPixels. And the function should use the term "Nearest" also.

Also, having a mix of "To" and "From" functions seems suboptimal. Do we really need that? Can't we just stick to To methods? They read nicely from left to right.
Except I guess they don't because these aren't inline methods :-(
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> The rounding should be consistent with (preferably, actually use)
> nsPoint::ToNearestPixels. And the function should use the term "Nearest"
> also.

The rounding I used is consistent with what HTMLImageElement::GetXY() was doing before; if that isn't right, that seems like a separate bug anyway.

> Also, having a mix of "To" and "From" functions seems suboptimal. Do we
> really need that? Can't we just stick to To methods? They read nicely from
> left to right.

That would work for me, I guess. That's not new in this bug, though.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Except I guess they don't because these aren't inline methods :-(

Not sure what you're referring to here.
FWIW I will probably change some or all of these things as part of bug 880676.
You need to log in before you can comment on or make changes to this bug.