Improve test cases for network information APIs

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Gyuyoung Kim]

Internals has supported test framework for network info API so far. However, concrete implementation of network info APIs are implemented by WebKit layer, not WebCore. So, test framework of network info should be moved from Internals to LTC in order to test each port implementation. In addition, expected result needs to be modified. Because, the return value can be changed by each port implementation. So, existing test case only checks return type, not return value.

[Gyuyoung Kim]

Created attachment 149926 [details]
Candidate Patch

If eeze_net_free is called twice, crash occurs. It looks this is a bug in eeze. This patch should solve this problem.

[Gyuyoung Kim]

I file a bug for this problem in EFL bug tracker.
http://trac.enlightenment.org/e/ticket/1101

However, I'm not sure when bug I filed is fixed.

[Gyuyoung Kim]

Created attachment 150087 [details]
Patch

[Gyuyoung Kim]

I would like to get review except for eeze_net_free().

[Build Bot]

Comment on attachment 150087 [details]
Patch

Attachment 150087 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13117208

[Early Warning System Bot]

Comment on attachment 150087 [details]
Patch

Attachment 150087 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13119238

[Early Warning System Bot]

Comment on attachment 150087 [details]
Patch

Attachment 150087 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13118196

[Build Bot]

Comment on attachment 150087 [details]
Patch

Attachment 150087 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13120197

[Gyuyoung Kim]

Created attachment 150305 [details]
Patch

[WebKit Review Bot]

Comment on attachment 150305 [details]
Patch

Attachment 150305 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13110613

New failing tests:
svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html

[WebKit Review Bot]

Created attachment 150308 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Gyuyoung Kim]

Created attachment 150324 [details]
Patch

[Gyuyoung Kim]

Created attachment 150331 [details]
Patch

[Gyuyoung Kim]

Created attachment 150347 [details]
Patch

[Gyuyoung Kim]

CC'ing Adam and Ryusuke.

[Adam Barth]

Comment on attachment 150347 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:818
> +    WebCore::Page* page = EWKPrivate::corePage(ewkView);
> +    WebCore::NetworkInfoController::from(page)->didChangeNetworkInformation(String(eventType->ustring().impl()), WebCore::NetworkInfo::create(bandwidth, metered));

I'm not sure I understand.  This function seems to talk directly to WebCore as well.  Why is this better than Internals?

[Gyuyoung Kim]

(In reply to comment #16)
> (From update of attachment 150347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150347&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:818
> > +    WebCore::Page* page = EWKPrivate::corePage(ewkView);
> > +    WebCore::NetworkInfoController::from(page)->didChangeNetworkInformation(String(eventType->ustring().impl()), WebCore::NetworkInfo::create(bandwidth, metered));
> 
> I'm not sure I understand.  This function seems to talk directly to WebCore as well.  Why is this better than Internals?

I think that the most important thing in this feature is to support bandwidth and metered property. But, this property should be supported by each port. So, concrete implementation is able to be different between ports. As far as I know, Internals only can support common implementation regardless of port specific implementation, right? If so, network information feature should tested by LTC instead of Internals in order to test *bandwidth* and *metered* property.

[Gyuyoung Kim]

Hello Adam,

Network Information API has two properties. bandwidth and metered as below,

double NetworkInfoConnection::bandwidth() const
{
    if (m_networkInfo)
        return m_networkInfo->bandwidth();
   
    return m_controller->client()->bandwidth();
}
	
bool NetworkInfoConnection::metered() const
{
    if (m_networkInfo)
        return m_networkInfo->metered();
   
    return m_controller->client()->metered();
}
	
void NetworkInfoConnection::didChangeNetworkInformation(PassRefPtr<Event> event, PassRefPtr<NetworkInfo> networkInfo)
{
    m_networkInfo = networkInfo;
    dispatchEvent(event); // This calls event handler in test case, then 
}

http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/networkinfo/NetworkInfoConnection.cpp


Existing test framework by Internals has tested  *event handler*, *bandwidth* and *metered*. However, in my humble opinion, bandwidth and metered can be implemented by port layer(WebKit layer). As far as I know, Internals can't support test cases which depend on port implementation. That's why I submit this patch.

However, if you still think that Internals is proper test framework for network information APIs, I would like to fix a problem in existing test framework. The problem is that existing test framework doesn't test bandwidth and metered based on port implementation. 

I'd like to get your comment.

[Gyuyoung Kim]

On more thing, dispatchEvent() calls eventually event handler in test case, then test case calls bandwidth and metered functions. Eventually, bandwidth and metered in port implementation are invoked.

void NetworkInfoConnection::didChangeNetworkInformation(PassRefPtr<Event> event, PassRefPtr<NetworkInfo> networkInfo)
{
    m_networkInfo = networkInfo;
    dispatchEvent(event); // This calls event handler in test case, then 
}

[Adam Barth]

I guess I don't understand why wiring this call through LayoutTestController buys anything.  It seems like it's just calling DumpRenderTreeSupportEfl::setNetworkInformation, which does the same thing as the internals function you're removing.

I'm sorry that I'm not understanding.

[Gyuyoung Kim]

(In reply to comment #20)
> I guess I don't understand why wiring this call through LayoutTestController buys anything.  It seems like it's just calling DumpRenderTreeSupportEfl::setNetworkInformation, which does the same thing as the internals function you're removing.

Of course, Internals can invoke DumpRenderTreeSupportEfl::setNetworkInformation as well. However, according to my experiences to move test cases to Internals, if there are port implementation dependency, reviewers([e-mail]) said LTC is more proper test framework because Internals can't support to test  specific port implementation. In this case, bandwidth and metered attributes should be implemented by port implementation. In addition, each port may be able to modify them by itself. So, I think LTC is more proper test framework. 

However, if you can't agree with this patch, I would like to just fix existing problem, which doesn't test bandwidth and metered properties on port implementation.

> 
> I'm sorry that I'm not understanding.
No, I always appreciate to your review.

[Adam Barth]

In this case, LayoutTestController isn't exercising any port-specific code.  It's just calling WebCore functions, which something Internals can do just fine.

[Gyuyoung Kim]

Comment on attachment 150347 [details]
Patch

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

(In reply to comment #22)
> In this case, LayoutTestController isn't exercising any port-specific code.  It's just calling WebCore functions, which something Internals can do just fine.

Ok, if you think existing implementation is correct, I don't wanna stuck to my opinion. 

By the way, I would like to use two things I mention below. How do you think about them ?

> Source/WebCore/Modules/networkinfo/NetworkInfoConnection.cpp:-61
> -        return m_networkInfo->bandwidth();

I would like to remove this code in order to test bandwidth function implemented by port. In addition, it is correct to return result from port implementation.

> Source/WebCore/Modules/networkinfo/NetworkInfoConnection.cpp:-69
> -        return m_networkInfo->metered();

ditto.

> LayoutTests/networkinformation/add-listener-from-callback-expected.txt:6
> +PASS typeof connection.bandwidth is "number"

I would like to use these test cases. Because, it seems to me it is more correct to check if type of bandwidth and metered is number | boolean. Unfortunately, I think we can't compare with fixed number because detail result can be different according to port implementation.

[Adam Barth]

> By the way, I would like to use two things I mention below. How do you think about them ?
> 
> > Source/WebCore/Modules/networkinfo/NetworkInfoConnection.cpp:-61
> > -        return m_networkInfo->bandwidth();
> 
> I would like to remove this code in order to test bandwidth function implemented by port. In addition, it is correct to return result from port implementation.
> 
> > Source/WebCore/Modules/networkinfo/NetworkInfoConnection.cpp:-69
> > -        return m_networkInfo->metered();
> 
> ditto.

Those changes make sense.

> > LayoutTests/networkinformation/add-listener-from-callback-expected.txt:6
> > +PASS typeof connection.bandwidth is "number"
> 
> I would like to use these test cases. Because, it seems to me it is more correct to check if type of bandwidth and metered is number | boolean. Unfortunately, I think we can't compare with fixed number because detail result can be different according to port implementation.

Ok.  They seems like less specific tests.  We can always generalize them later when we have multiple ports with different values.

[Gyuyoung Kim]

Created attachment 150560 [details]
Patch

[Gyuyoung Kim]

I update this patch to fix existing problems.

[Adam Barth]

Comment on attachment 150560 [details]
Patch

Thanks.

[WebKit Review Bot]

Comment on attachment 150560 [details]
Patch

Clearing flags on attachment: 150560

Committed r121754: <http://trac.webkit.org/changeset/121754>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.