Bug 90162 - Improve test cases for network information APIs
: Improve test cases for network information APIs
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 90333
:
  Show dependency treegraph
 
Reported: 2012-06-28 04:43 PST by
Modified: 2012-07-03 03:13 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-06-28 04:43:36 PST
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 From 2012-06-28 04:53:09 PST -------
Created an attachment (id=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 From 2012-06-28 22:50:12 PST -------
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 From 2012-06-28 23:09:44 PST -------
Created an attachment (id=150087) [details]
Patch
------- Comment #4 From 2012-06-28 23:10:44 PST -------
I would like to get review except for eeze_net_free().
------- Comment #5 From 2012-06-28 23:18:57 PST -------
(From update of attachment 150087 [details])
Attachment 150087 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13117208
------- Comment #6 From 2012-06-28 23:39:25 PST -------
(From update of attachment 150087 [details])
Attachment 150087 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13119238
------- Comment #7 From 2012-06-28 23:40:36 PST -------
(From update of attachment 150087 [details])
Attachment 150087 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13118196
------- Comment #8 From 2012-06-29 00:44:35 PST -------
(From update of attachment 150087 [details])
Attachment 150087 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13120197
------- Comment #9 From 2012-06-29 22:25:43 PST -------
Created an attachment (id=150305) [details]
Patch
------- Comment #10 From 2012-06-29 23:14:59 PST -------
(From update of attachment 150305 [details])
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 From 2012-06-29 23:15:04 PST -------
Created an attachment (id=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 From 2012-06-30 17:24:56 PST -------
Created an attachment (id=150324) [details]
Patch
------- Comment #13 From 2012-07-01 10:42:45 PST -------
Created an attachment (id=150331) [details]
Patch
------- Comment #14 From 2012-07-01 17:18:15 PST -------
Created an attachment (id=150347) [details]
Patch
------- Comment #15 From 2012-07-01 22:34:19 PST -------
CC'ing Adam and Ryusuke.
------- Comment #16 From 2012-07-01 23:32:43 PST -------
(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?
------- Comment #17 From 2012-07-01 23:37:50 PST -------
(In reply to comment #16)
> (From update of attachment 150347 [details] [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 From 2012-07-02 21:37:43 PST -------
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 From 2012-07-02 21:42:45 PST -------
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 From 2012-07-02 22:18:22 PST -------
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 From 2012-07-02 22:31:01 PST -------
(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 From 2012-07-02 22:35:42 PST -------
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 From 2012-07-02 22:59:30 PST -------
(From update of attachment 150347 [details])
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 From 2012-07-02 23:49:26 PST -------
> 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 From 2012-07-03 01:45:21 PST -------
Created an attachment (id=150560) [details]
Patch
------- Comment #26 From 2012-07-03 01:47:20 PST -------
I update this patch to fix existing problems.
------- Comment #27 From 2012-07-03 01:48:00 PST -------
(From update of attachment 150560 [details])
Thanks.
------- Comment #28 From 2012-07-03 03:13:06 PST -------
(From update of attachment 150560 [details])
Clearing flags on attachment: 150560

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