Bug 42653 – IntSize / FloatSize: add invalid() and isValid() helper methods

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[Antonio Gomes]

In comment https://bugs.webkit.org/show_bug.cgi?id=40197#c41 , it was suggested a helper / getter for invalid IntSize's. 
Maybe something simple like this:

static IntSize invalid() { return IntSize(-1, -1); }

similarly to the static IntPoint zero() { return IntPoint(0, 0); } added in bug 37220.

Thoughts?

ps: If I hear it is ok, upload a patch.

[Antonio Gomes]

isValid() would work similarly to Qt's version: "Returns true if both the width and height is equal to or greater than 0; otherwise returns false." [1]

[1] http://doc.trolltech.com/4.7-snapshot/qsize.html#isValid

[Darin Adler]

This use of -1,-1 as a magic value seems questionable to me, even though Qt uses it.

[Antonio Gomes]

(In reply to comment #2)
> This use of -1,-1 as a magic value seems questionable to me, even though Qt uses it.

In fact qt does not use it as there is not static invalid method in QSize.

If we decide to not go for adding invalid() since -1,-1 is in fact "magic", I can keep using IntSize(-1, -1) where appropriated.

However, least isValid() seems useful. It does have a different meaning from isEmpty and isZero.

[Antonio Gomes]

Other possible users of isValid() and maybe IntSize::invalid() 

static void constructDeletedValue(IntSize& slot)
{
  new (&slot) IntSize(-1, -1);
}

static bool isDeletedValue(const IntSize& value)
{
  return value.width() == -1 && value.height() == -1;
}

[Antonio Gomes]

... and:

void FrameView::init()
{
    reset();
    m_margins = IntSize(-1, -1); // undefined

sorry for bug-spamming.

[Antonio Gomes]

Created attachment 62096 [details]
patch1 - v1 - isValid addition.

Lets go with isValid for now. Darin?

[Darin Adler]

(In reply to comment #3)
> However, least isValid() seems useful. It does have a different meaning from isEmpty and isZero.

Sure, but is this a useful distinction? Again, I know Qt has it, but my starting point would be assuming we should not use invalid points.

The distinction between null and empty string is an eternal headache and it would be nice to not have more things like that.

[Antonio Gomes]

(In reply to comment #7)
> (In reply to comment #3)
> > However, least isValid() seems useful. It does have a different meaning from isEmpty and isZero.
> 
> Sure, but is this a useful distinction? Again, I know Qt has it, but my starting point would be assuming we should not use invalid points.
> 
> The distinction between null and empty string is an eternal headache and it would be nice to not have more things like that.

Imho, it is an useful distiction in the context of the rect-based hit test, for example. The situation there is the following: if one calls nodesFromRect(x, y, 0, 0), a valid rect is still built up.

It means the caller of this method still wants a rect-based hit test but he (the caller) is saying to not expand the original point.

In fact, that is what he will get since the formula we are using the build up the rect from the point + padding still accepts the previous method input. Here is it:

inline IntRect HitTestResult::rectFromPoint(const IntPoint& p) const
{
  return IntRect(p.x() - m_padding.width(),   // x
                 p.y() - m_padding.height(),  // y
                 2 * m_padding.width() + 1,   // weigth
                 2 * m_padding.height() + 1); // height
}


So in this context, non-negative IntSize's are valid.

If user explicitly passes an invalid/undefined/negative IntSize(-anyValue, -anyValue) the current is{Empty,Zero} methods are not what we need to perform the check.

Of course I can still make isValid internally in my class, but would be useful to have this int IntSize class.

Hope I could had made it clear about my of use for adding this simple helper. Let me know if you still thing it is bad thing to do.

[Antonio Gomes]

s/weigth/width

[Darin Adler]

Comment on attachment 62096 [details]
patch1 - v1 - isValid addition.

Core Graphics has this concept for rectangles, but not for sizes. And it calls the concept “is null” rather than “is (in)valid”.

I’m not sure our functions that manipulate sizes all respect this notion. It doesn’t do a lot of harm to add these functions, but I have to say the concept seems a bit unclear to me.

Where are some of the call sites where we want to use this?

[Antonio Gomes]

(In reply to comment #10)

I see your conceptual concerns. If we go for not adding this method, I can make it an inliner in my class, no problem.

> Where are some of the call sites where we want to use this?

See some possible users of this in comment #4 and comment #5, and again, the validation check of m_padding (IntSize) in HitTestResult - see bug 40197.

[Darin Adler]

(In reply to comment #11)
> See some possible users of this in comment #4 and comment #5

I would prefer not to use it in either of those places.

> the validation check of m_padding (IntSize) in HitTestResult - see bug 40197.

I don’t know exactly what a validation check is. Do we really need a separate "invalid" value for padding? This seems unnecessarily tricky. Typically it's best to avoid special values and instead use explicit signaling. A separate boolean or some other way of saying there is no padding value, rather than a special "null" value.

[Antonio Gomes]

> > the validation check of m_padding (IntSize) in HitTestResult - see bug 40197.
> 
> I don’t know exactly what a validation check is. Do we really need a separate "invalid" value for padding? This seems unnecessarily tricky. Typically it's best to avoid special values and instead use explicit signaling. A separate boolean or some other way of saying there is no padding value, rather than a special "null" value.

Sure. A boolean works for me, and I am already using one. I was think it'd be more elegant to not use, though, but the change proved itself rather unneeded, so closing the bug as INVALID.

Thank you for your time on this, Darin.