Bug 77320 – [EFL] Add basic DRT/Efl implementation to support viewport test.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

Patch will be added.

[Ryuan Choi]

Created attachment 124501 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 124501 [details]
Patch

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

I wonder how many viewport test cases are passed by this patch.

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:403
> +    printf("viewport size %dx%d scale %f with limits [%f, %f] and userScalable %f\n", attributes.layoutSize.width(), attributes.layoutSize.height(), attributes.initialScale, attributes.minimumScale, attributes.maximumScale, attributes.userScalable);

printf can influence on layout test result. Remove it or change other thing.

[Ryuan Choi]

(In reply to comment #2)
> (From update of attachment 124501 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124501&action=review
> 
> I wonder how many viewport test cases are passed by this patch.
> 

143 test cases are passed with this.

> > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:403
> > +    printf("viewport size %dx%d scale %f with limits [%f, %f] and userScalable %f\n", attributes.layoutSize.width(), attributes.layoutSize.height(), attributes.initialScale, attributes.minimumScale, attributes.maximumScale, attributes.userScalable);
> 
> printf can influence on layout test result. Remove it or change other thing.

It is what I want, because The expected result of viewport tests check above.
Gtk use fprintf(stdout, ... and Qt use fputs for it.
I found that DRT/Efl are using printf for the test result, so I choosed printf.

[Gyuyoung Kim]

Comment on attachment 124501 [details]
Patch

Personally, I would like to avoid to use printf directly. Because, IMHO, printf can make us confuse if this is real debug message or not. Though tiled backing store files are using printf in efl port now, I think the printf should be replaced by other's.

[Ryuan Choi]

(In reply to comment #4)
> (From update of attachment 124501 [details])
> Personally, I would like to avoid to use printf directly. Because, IMHO, printf can make us confuse if this is real debug message or not. Though tiled backing store files are using printf in efl port now, I think the printf should be replaced by other's.

I used printf because DRT/Efl are using printf in other area.
But, printf, fprintf, fputs are not important for me.

If you and kubo and other guys like fprintf more, I can revise the patch.

Kubo, how do you think about it ?

[KwangHyuk]

Just a few fixes may be required.

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:389
> +void DumpRenderTreeSupportEfl::dumpConfigurationForViewport(Evas_Object* ewkView, int deviceDPI, const WebCore::IntSize& deviceSize, const WebCore::IntSize& availableSize)

Why don't you use const type for deviceDPI ?

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:397
> +            /*default layout width for non-mobile pages */ 980,

/*default <= check this ?

> LayoutTests/ChangeLog:8
> +        Removed fast/viewport from Skipped and add some tests which is not passed.

Removed -> Remove :)
is not passed -> aren't passed

[Ryuan Choi]

Created attachment 124662 [details]
Patch

[Ryuan Choi]

Created attachment 124663 [details]
Patch

[Ryuan Choi]

(In reply to comment #6)
> Just a few fixes may be required.
> 
> > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:389
> > +void DumpRenderTreeSupportEfl::dumpConfigurationForViewport(Evas_Object* ewkView, int deviceDPI, const WebCore::IntSize& deviceSize, const WebCore::IntSize& availableSize)
> 
> Why don't you use const type for deviceDPI ?
Does it have any benefits ?
IMO, it's not problem.

> 
> > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:397
> > +            /*default layout width for non-mobile pages */ 980,
> 
> /*default <= check this ?
fixed.

> 
> > LayoutTests/ChangeLog:8
> > +        Removed fast/viewport from Skipped and add some tests which is not passed.
> 
> Removed -> Remove :)
> is not passed -> aren't passed
fixed.

Thanks for my typo.

[Gyuyoung Kim]

Comment on attachment 124663 [details]
Patch

LGTM. :-)

[WebKit Review Bot]

Comment on attachment 124663 [details]
Patch

Clearing flags on attachment: 124663

Committed r106431: <http://trac.webkit.org/changeset/106431>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.