Bug 75695 – [EFL] Modify some API's implementation to let it directly pass Eina_Rectangle into IntRect.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[KwangHyuk]

For the simple assignment from Eina_Rectagle to IntRect, IntRect(const Eina_Rectangle*) will be added.
And some API's implementation will be modified to let it directly pass Eina_Rectangle* into IntRect.

[KwangHyuk]

Created attachment 121840 [details]
Propose a patch.

[Andreas Kling]

Comment on attachment 121840 [details]
Propose a patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=121840&action=review

> Source/WebCore/platform/graphics/IntRect.h:192
>      explicit IntRect(const Eina_Rectangle&);
> +    explicit IntRect(const Eina_Rectangle*);

This looks like a bad idea to me, it would be better to update call sites to dereference the Eina_Rectangle* so they can go through the Eina_Rectangle& constructor, e.g:
WebCore::IntRect rect(*area);
The problem with either approach is that you have no handling of null Eina_Rectangle*, but I suppose that's something for the call sites to know about.

[KwangHyuk]

(In reply to comment #2)
> (From update of attachment 121840 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121840&action=review
> 
> > Source/WebCore/platform/graphics/IntRect.h:192
> >      explicit IntRect(const Eina_Rectangle&);
> > +    explicit IntRect(const Eina_Rectangle*);
> 
> This looks like a bad idea to me, it would be better to update call sites to dereference the Eina_Rectangle* so they can go through the Eina_Rectangle& constructor, e.g:
> WebCore::IntRect rect(*area);
Yes,It might be the reason why other ports didn't add pointer type.
Ok, I will try it.


> The problem with either approach is that you have no handling of null Eina_Rectangle*, but I suppose that's something for the call sites to know about.
I have considered handling of null.
But, as far as I know, WebCore doesn't like null handling for every cases. :)

[KwangHyuk]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 121840 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=121840&action=review
> > 
> > > Source/WebCore/platform/graphics/IntRect.h:192
> > >      explicit IntRect(const Eina_Rectangle&);
> > > +    explicit IntRect(const Eina_Rectangle*);
> > 
> > This looks like a bad idea to me, it would be better to update call sites to dereference the Eina_Rectangle* so they can go through the Eina_Rectangle& constructor, e.g:
> > WebCore::IntRect rect(*area);
> Yes,It might be the reason why other ports didn't add pointer type.
> Ok, I will try it.
> 
> 
> > The problem with either approach is that you have no handling of null Eina_Rectangle*, but I suppose that's something for the call sites to know about.
> I have considered handling of null.
> But, as far as I know, WebCore doesn't like null handling for every cases. :)

"WebCore doesn't like null handling for every cases." meant "WebCore doesn't seem to force it to check null for every cases." :)

[KwangHyuk]

Created attachment 122148 [details]
Patch is updated according to Kling's suggestion.

[KwangHyuk]

(In reply to comment #2)
> (From update of attachment 121840 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121840&action=review
> 
> > Source/WebCore/platform/graphics/IntRect.h:192
> >      explicit IntRect(const Eina_Rectangle&);
> > +    explicit IntRect(const Eina_Rectangle*);
> 
> This looks like a bad idea to me, it would be better to update call sites to dereference the Eina_Rectangle* so they can go through the Eina_Rectangle& constructor, e.g:
> WebCore::IntRect rect(*area);
> The problem with either approach is that you have no handling of null Eina_Rectangle*, but I suppose that's something for the call sites to know about.

I updated patch according to your suggestion. Thx. :)

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 122148 [details]
Patch is updated according to Kling's suggestion.

View in context: https://bugs.webkit.org/attachment.cgi?id=122148&action=review

Looks OK.

> Source/WebKit/efl/ChangeLog:10
> +        As some APIs in ewk_view doesn't make the best use of direct passing of
> +        Eina_Rectangle parameter into IntRect although IntRect supports it,
> +        I just refactor the code to let it directly pass Eina_Rectangle into IntRect.

I'd just reword it as "Dereference Eina_Rectangle pointers so the shorter IntRect constructor can be used".

[KwangHyuk]

Created attachment 122164 [details]
ChangeLog updated.

[KwangHyuk]

(In reply to comment #7)
> (From update of attachment 122148 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122148&action=review
> 
> Looks OK.
> 
> > Source/WebKit/efl/ChangeLog:10
> > +        As some APIs in ewk_view doesn't make the best use of direct passing of
> > +        Eina_Rectangle parameter into IntRect although IntRect supports it,
> > +        I just refactor the code to let it directly pass Eina_Rectangle into IntRect.
> 
> I'd just reword it as "Dereference Eina_Rectangle pointers so the shorter IntRect constructor can be used".

Ok, it looks easier to understand.
So, I have changed ChangeLog. :)

[WebKit Review Bot]

Comment on attachment 122164 [details]
ChangeLog updated.

Clearing flags on attachment: 122164

Committed r104799: <http://trac.webkit.org/changeset/104799>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.