Bug 128779 - [EFL][WK1] Do not include cairo header in the public headers
Summary: [EFL][WK1] Do not include cairo header in the public headers
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: Ryuan Choi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-13 17:31 PST by Ryuan Choi
Modified: 2014-02-15 00:34 PST (History)
8 users (show)

See Also:


Attachments
Patch (20.70 KB, patch)
2014-02-13 19:53 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (21.38 KB, patch)
2014-02-13 21:06 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (22.34 KB, patch)
2014-02-13 22:47 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (20.57 KB, patch)
2014-02-14 21:51 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2014-02-13 17:31:40 PST
Although ewebkit used cairo internally, it's not good for EFL developers to use it.
Comment 1 Ryuan Choi 2014-02-13 19:53:23 PST
Created attachment 224150 [details]
Patch
Comment 2 WebKit Commit Bot 2014-02-13 19:55:50 PST
Attachment 224150 [details] did not pass style-queue:


ERROR: Tools/EWebLauncher/main.c:208:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/EWebLauncher/main.c:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Gyuyoung Kim 2014-02-13 20:12:26 PST
Comment on attachment 224150 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_view.h:2499
> +EAPI Evas_Object *ewk_view_screenshot_contents_get(const Evas_Object *o, const Eina_Rectangle *area, float scale);

I will be pleased to land this patch with an unit test for this new APIs
Comment 4 Ryuan Choi 2014-02-13 21:06:04 PST
Created attachment 224158 [details]
Patch
Comment 5 Ryuan Choi 2014-02-13 21:11:06 PST
(In reply to comment #3)
> (From update of attachment 224150 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224150&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2499
> > +EAPI Evas_Object *ewk_view_screenshot_contents_get(const Evas_Object *o, const Eina_Rectangle *area, float scale);
> 
> I will be pleased to land this patch with an unit test for this new APIs

I agree,
But, in order to test this API fully, we should check the pixels of image objects.
And it will be difficult little bit because current framework for webkit1/efl are poor.

Or, do you think that it is enough to check image size using scale?

I considered little bit, but i didn't because this API will be used for pixel tests and we can check the functionality using EWebLauncher.
Comment 6 Gyuyoung Kim 2014-02-13 21:16:53 PST
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 224150 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=224150&action=review
> > 
> > > Source/WebKit/efl/ewk/ewk_view.h:2499
> > > +EAPI Evas_Object *ewk_view_screenshot_contents_get(const Evas_Object *o, const Eina_Rectangle *area, float scale);
> > 
> > I will be pleased to land this patch with an unit test for this new APIs
> 
> I agree,
> But, in order to test this API fully, we should check the pixels of image objects.
> And it will be difficult little bit because current framework for webkit1/efl are poor.

I think we can leave a comment. For instance, "FIXME: EFL WK1 has still poor DRT to test this API."
 
> Or, do you think that it is enough to check image size using scale?

It looks we need to check it by using the pixel test. However, it is difficult now, we need to check the image size or image is not null at least.
Comment 7 Ryuan Choi 2014-02-13 21:36:09 PST
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > (From update of attachment 224150 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=224150&action=review
> > > 
> > > > Source/WebKit/efl/ewk/ewk_view.h:2499
> > > > +EAPI Evas_Object *ewk_view_screenshot_contents_get(const Evas_Object *o, const Eina_Rectangle *area, float scale);
> > > 
> > > I will be pleased to land this patch with an unit test for this new APIs
> > 
> > I agree,
> > But, in order to test this API fully, we should check the pixels of image objects.
> > And it will be difficult little bit because current framework for webkit1/efl are poor.
> 
> I think we can leave a comment. For instance, "FIXME: EFL WK1 has still poor DRT to test this API."
> 
> > Or, do you think that it is enough to check image size using scale?
> 
> It looks we need to check it by using the pixel test. However, it is difficult now, we need to check the image size or image is not null at least.

OK, I will
Comment 8 Ryuan Choi 2014-02-13 22:47:06 PST
Created attachment 224163 [details]
Patch
Comment 9 Gyuyoung Kim 2014-02-14 21:27:45 PST
Comment on attachment 224163 [details]
Patch

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

> Tools/EWebLauncher/main.c:462
> +            static Evas_Object *screenshot = NULL;

Any reason to define *static* ? Besides could you let me know what functionality do you want to do here ? It seems you just test ewk_view_screenshot_contents_get() API here.
Comment 10 Ryuan Choi 2014-02-14 21:51:17 PST
Created attachment 224285 [details]
Patch
Comment 11 Ryuan Choi 2014-02-14 21:51:52 PST
Comment on attachment 224163 [details]
Patch

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

>> Tools/EWebLauncher/main.c:462
>> +            static Evas_Object *screenshot = NULL;
> 
> Any reason to define *static* ? Besides could you let me know what functionality do you want to do here ? It seems you just test ewk_view_screenshot_contents_get() API here.

Yes, it is for visual testing.

I removed
Comment 12 Gyuyoung Kim 2014-02-14 21:53:25 PST
Comment on attachment 224285 [details]
Patch

LGTM. r=me.
Comment 13 WebKit Commit Bot 2014-02-15 00:34:08 PST
Comment on attachment 224285 [details]
Patch

Clearing flags on attachment: 224285

Committed r164168: <http://trac.webkit.org/changeset/164168>
Comment 14 WebKit Commit Bot 2014-02-15 00:34:12 PST
All reviewed patches have been landed.  Closing bug.