Bug 90162 - Improve test cases for network information APIs
: Improve test cases for network information APIs
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Gyuyoung Kim
:
Depends on: 90333
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-28 04:43 PDT by Gyuyoung Kim
Modified: 2012-07-03 03:13 PDT (History)
10 users (show)

See Also:


Attachments
Candidate Patch (25.62 KB, patch)
2012-06-28 04:53 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (27.41 KB, patch)
2012-06-28 23:09 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (33.51 KB, patch)
2012-06-29 22:25 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.01 MB, application/zip)
2012-06-29 23:15 PDT, WebKit Review Bot
no flags Details
Patch (33.46 KB, patch)
2012-06-30 17:24 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (34.25 KB, patch)
2012-07-01 10:42 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (33.53 KB, patch)
2012-07-01 17:18 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (11.86 KB, patch)
2012-07-03 01:45 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2012-06-28 04:43:36 PDT
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.
Comment 1 Gyuyoung Kim 2012-06-28 04:53:09 PDT
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.
Comment 2 Gyuyoung Kim 2012-06-28 22:50:12 PDT
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.
Comment 3 Gyuyoung Kim 2012-06-28 23:09:44 PDT
Created attachment 150087 [details]
Patch
Comment 4 Gyuyoung Kim 2012-06-28 23:10:44 PDT
I would like to get review except for eeze_net_free().
Comment 5 Build Bot 2012-06-28 23:18:57 PDT
Comment on attachment 150087 [details]
Patch

Attachment 150087 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13117208
Comment 6 Early Warning System Bot 2012-06-28 23:39:25 PDT
Comment on attachment 150087 [details]
Patch

Attachment 150087 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13119238
Comment 7 Early Warning System Bot 2012-06-28 23:40:36 PDT
Comment on attachment 150087 [details]
Patch

Attachment 150087 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13118196
Comment 8 Build Bot 2012-06-29 00:44:35 PDT
Comment on attachment 150087 [details]
Patch

Attachment 150087 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13120197
Comment 9 Gyuyoung Kim 2012-06-29 22:25:43 PDT
Created attachment 150305 [details]
Patch
Comment 10 WebKit Review Bot 2012-06-29 23:14:59 PDT
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
Comment 11 WebKit Review Bot 2012-06-29 23:15:04 PDT
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
Comment 12 Gyuyoung Kim 2012-06-30 17:24:56 PDT
Created attachment 150324 [details]
Patch
Comment 13 Gyuyoung Kim 2012-07-01 10:42:45 PDT
Created attachment 150331 [details]
Patch
Comment 14 Gyuyoung Kim 2012-07-01 17:18:15 PDT
Created attachment 150347 [details]
Patch
Comment 15 Gyuyoung Kim 2012-07-01 22:34:19 PDT
CC'ing Adam and Ryusuke.
Comment 16 Adam Barth 2012-07-01 23:32:43 PDT
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?
Comment 17 Gyuyoung Kim 2012-07-01 23:37:50 PDT
(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.
Comment 18 Gyuyoung Kim 2012-07-02 21:37:43 PDT
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.
Comment 19 Gyuyoung Kim 2012-07-02 21:42:45 PDT
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 
}
Comment 20 Adam Barth 2012-07-02 22:18:22 PDT
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.
Comment 21 Gyuyoung Kim 2012-07-02 22:31:01 PDT
(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(ap@webkit.org) 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.
Comment 22 Adam Barth 2012-07-02 22:35:42 PDT
In this case, LayoutTestController isn't exercising any port-specific code.  It's just calling WebCore functions, which something Internals can do just fine.
Comment 23 Gyuyoung Kim 2012-07-02 22:59:30 PDT
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.
Comment 24 Adam Barth 2012-07-02 23:49:26 PDT
> 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.
Comment 25 Gyuyoung Kim 2012-07-03 01:45:21 PDT
Created attachment 150560 [details]
Patch
Comment 26 Gyuyoung Kim 2012-07-03 01:47:20 PDT
I update this patch to fix existing problems.
Comment 27 Adam Barth 2012-07-03 01:48:00 PDT
Comment on attachment 150560 [details]
Patch

Thanks.
Comment 28 WebKit Review Bot 2012-07-03 03:13:06 PDT
Comment on attachment 150560 [details]
Patch

Clearing flags on attachment: 150560

Committed r121754: <http://trac.webkit.org/changeset/121754>
Comment 29 WebKit Review Bot 2012-07-03 03:13:13 PDT
All reviewed patches have been landed.  Closing bug.