Bug 75695 - [EFL] Modify some API's implementation to let it directly pass Eina_Rectangle into IntRect.
Summary: [EFL] Modify some API's implementation to let it directly pass Eina_Rectangle...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-06 02:43 PST by KwangHyuk
Modified: 2012-01-12 01:51 PST (History)
4 users (show)

See Also:


Attachments
Propose a patch. (5.07 KB, patch)
2012-01-10 06:27 PST, KwangHyuk
no flags Details | Formatted Diff | Diff
Patch is updated according to Kling's suggestion. (3.07 KB, patch)
2012-01-11 18:42 PST, KwangHyuk
no flags Details | Formatted Diff | Diff
ChangeLog updated. (2.92 KB, patch)
2012-01-11 21:24 PST, KwangHyuk
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description KwangHyuk 2012-01-06 02:43:39 PST
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.
Comment 1 KwangHyuk 2012-01-10 06:27:49 PST
Created attachment 121840 [details]
Propose a patch.
Comment 2 Andreas Kling 2012-01-10 06:51:55 PST
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.
Comment 3 KwangHyuk 2012-01-10 07:13:31 PST
(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. :)
Comment 4 KwangHyuk 2012-01-10 10:06:13 PST
(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." :)
Comment 5 KwangHyuk 2012-01-11 18:42:26 PST
Created attachment 122148 [details]
Patch is updated according to Kling's suggestion.
Comment 6 KwangHyuk 2012-01-11 18:43:11 PST
(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. :)
Comment 7 Raphael Kubo da Costa (:rakuco) 2012-01-11 20:11:43 PST
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".
Comment 8 KwangHyuk 2012-01-11 21:24:07 PST
Created attachment 122164 [details]
ChangeLog updated.
Comment 9 KwangHyuk 2012-01-11 21:25:27 PST
(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. :)
Comment 10 WebKit Review Bot 2012-01-12 01:51:09 PST
Comment on attachment 122164 [details]
ChangeLog updated.

Clearing flags on attachment: 122164

Committed r104799: <http://trac.webkit.org/changeset/104799>
Comment 11 WebKit Review Bot 2012-01-12 01:51:16 PST
All reviewed patches have been landed.  Closing bug.