Bug 95984 - [EFL] Add unit test for ewk_frame_source_get.
Summary: [EFL] Add unit test for ewk_frame_source_get.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Kangil Han
URL:
Keywords:
Depends on: 94925
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-06 07:55 PDT by Kangil Han
Modified: 2012-10-18 01:25 PDT (History)
7 users (show)

See Also:


Attachments
patch (3.83 KB, patch)
2012-10-08 21:40 PDT, Kangil Han
no flags Details | Formatted Diff | Diff
patch (6.94 KB, patch)
2012-10-17 02:06 PDT, Kangil Han
no flags Details | Formatted Diff | Diff
patch (6.22 KB, patch)
2012-10-17 17:35 PDT, Kangil Han
no flags Details | Formatted Diff | Diff
patch (5.02 KB, patch)
2012-10-17 23:07 PDT, Kangil Han
gyuyoung.kim: review+
g.czajkowski: commit-queue-
Details | Formatted Diff | Diff
patch (5.02 KB, patch)
2012-10-18 01:04 PDT, Kangil Han
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kangil Han 2012-09-06 07:55:54 PDT
To test if WebKit automatically insert <html></html> tags.
Comment 1 Kangil Han 2012-10-08 21:40:42 PDT
Created attachment 167686 [details]
patch
Comment 2 Ryuan Choi 2012-10-08 23:51:29 PDT
Comment on attachment 167686 [details]
patch

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

> Source/WebKit/efl/tests/test_ewk_frame.cpp:33
> +#include <Ecore.h>
> +#include <wtf/OwnPtr.h>
> +#include <wtf/PassOwnPtr.h>

Are these necessary?

> Source/WebKit/efl/tests/test_ewk_frame.cpp:46
> +    size_t read = ewk_frame_source_get(ewk_view_frame_main_get(webView()), &buffer);
> +    const char* expected = "<html><head>";
> +    int isStartsWithExpected = strncmp(buffer, expected, strlen(expected));

It looks weird to me.

Can we have more rich tests ?
Comment 3 Grzegorz Czajkowski 2012-10-09 00:01:49 PDT
Comment on attachment 167686 [details]
patch

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

> Source/WebKit/efl/tests/test_ewk_frame.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''

I'd prefer not mention about Apple copyrights here. Please use pure BSD license.

> Source/WebKit/efl/tests/test_ewk_frame.cpp:38
> +* @brief Checking if function returns correct html source strings.

1) Please correct indentions for '*'.
2) Please provide more detailed information what the tests is really checking. The default tests page contains HTML and BODY tags only but it looks that you are expecting HTML and HEAD.
3) Are you planing to add others test cases for ewk_frame_source_get()?

> Source/WebKit/efl/tests/test_ewk_frame.cpp:44
> +    size_t read = ewk_frame_source_get(ewk_view_frame_main_get(webView()), &buffer);

It seems 'read' variable is not used anywhere.

> Source/WebKit/efl/tests/test_ewk_frame.cpp:45
> +    const char* expected = "<html><head>";

Why are you expecting html instead of HTML? The default tests page contains HTML tag.

> Source/WebKit/efl/tests/test_ewk_frame.cpp:48
> +    if (buffer)

Can be skipped, if buffer is NULL, no operation is performed by free.
Comment 4 Gyuyoung Kim 2012-10-09 00:28:15 PDT
Comment on attachment 167686 [details]
patch

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

>> Source/WebKit/efl/tests/test_ewk_frame.cpp:13
>> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
> 
> I'd prefer not mention about Apple copyrights here. Please use pure BSD license.

Can we use pure BSD license in WebKit ?
Comment 5 Ryuan Choi 2012-10-09 00:44:13 PDT
(In reply to comment #4)
> (From update of attachment 167686 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167686&action=review
> 
> >> Source/WebKit/efl/tests/test_ewk_frame.cpp:13
> >> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
> > 
> > I'd prefer not mention about Apple copyrights here. Please use pure BSD license.
> 
> Can we use pure BSD license in WebKit ?

I am not sure, but I found different license from Source/WebCore/platform/chromium/FileSystemChromiumLinux.cpp
Comment 6 Kenneth Rohde Christiansen 2012-10-09 02:42:17 PDT
http://www.webkit.org/coding/bsd-license.html

When submitting a patch you should have noticed this:

Legal:	WebKit Contribution Terms:

Hello and thank you for contributing a patch. Here is our licensing policy and terms for contributing code to the WebKit project.

If you are sending in a patch to existing WebKit code, you agree by clicking below that your changes are licensed under the existing license terms of the file you are modifying (i.e., BSD license or GNU Lesser General Public License v.2.1, LGPL v. 2.1). Please also add your copyright (name and year) to the relevant files for changes that are more than 10 lines of code.

If you are sending in a new file for inclusion in WebKit (no code copied from another source), the preferred license is BSD, but LGPL 2.1 is an option as well. Please include your copyright (name and year) and license preference (BSD or LGPL 2.1). By clicking below you agree that your file is licensed under either the BSD license or LGPL 2.1, as indicated in your file.

If you aren't the author of the patch, you agree to include the original copyright notices and licensing terms with it, to the extent that they exist. If there wasn't a copyright notice or license, please make a note of it. Generally we can only take in patches that are BSD- or LGPL-licensed in order to maintain license compatibility within the project.
Comment 7 Kangil Han 2012-10-17 02:06:13 PDT
Created attachment 169130 [details]
patch
Comment 8 Kenneth Rohde Christiansen 2012-10-17 02:23:49 PDT
Comment on attachment 169130 [details]
patch

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

> Source/WebKit/efl/ewk/ewk_frame.cpp:1636
> +    const int failed = -1;

WebKit style is to not use const for local variables

> Source/WebKit/efl/tests/test_ewk_frame.cpp:60
> + * @brief Checking if function works properly when loading non-existed url.

non-existing.

> Source/WebKit/efl/tests/test_ewk_frame.cpp:64
> +    loadUrl("http://www.abcdefg.com");

what if that url suddently exists?
Comment 9 Gyuyoung Kim 2012-10-17 02:26:04 PDT
Comment on attachment 169130 [details]
patch

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

> Source/WebKit/efl/ChangeLog:3
> +        [EFL][Refactoring] Optimize ewk_frame_source_get.

I think this patch is to add unit test for ewk_frame_source_get(). So, this title is not proper.

> Source/WebKit/efl/ChangeLog:8
> +        1. Change return type as 'int' to match with function description.

Generally, we don't use 1. 2. 3. in ChangeLog. It would be good if you move this description to each modified functions below.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1636
> +    const int failed = -1;

It looks this is unneeded. Existing one is fine to me.

> Source/WebKit/efl/tests/test_ewk_frame.cpp:50
> +TEST_F(EWKTestBase, ewk_frame_source_get_2)

Why not adding these tests into TEST_F(EWKTestBase, ewk_frame_source_get) ?

> Source/WebKit/efl/tests/test_ewk_frame.cpp:62
> +TEST_F(EWKTestBase, ewk_frame_source_get_3)

ditto.
Comment 10 Grzegorz Czajkowski 2012-10-17 02:45:03 PDT
Comment on attachment 169130 [details]
patch

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

> Source/WebKit/efl/ewk/ewk_frame.cpp:-1634
> -ssize_t ewk_frame_source_get(const Evas_Object* ewkFrame, char** frameSource)

ssize_t is enough to store -1 value.
"The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}]."

> Source/WebKit/efl/tests/test_ewk_frame.cpp:43
> +    if (buffer)

Unneeded, free() works properly for NULL pointers. Please omit it for all tests cases.
Comment 11 Kangil Han 2012-10-17 17:35:11 PDT
Created attachment 169312 [details]
patch

Done. :)
Comment 12 Kangil Han 2012-10-17 17:36:38 PDT
(In reply to comment #9)
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1636
> > +    const int failed = -1;
> 
> It looks this is unneeded. Existing one is fine to me.
> 

Shame, seems there is no existing one for this case. :-(
Comment 13 Gyuyoung Kim 2012-10-17 21:30:00 PDT
(In reply to comment #12)
> (In reply to comment #9)
> > > Source/WebKit/efl/ewk/ewk_frame.cpp:1636
> > > +    const int failed = -1;
> > 
> > It looks this is unneeded. Existing one is fine to me.
> > 
> 
> Shame, seems there is no existing one for this case. :-(

I mean -1. I think everyone can know that "-1" means error or failed in this case.
Comment 14 Kangil Han 2012-10-17 21:59:04 PDT
(In reply to comment #13)
> I mean -1. I think everyone can know that "-1" means error or failed in this case.

Okay, be back with '-1'. ;)
Comment 15 Kangil Han 2012-10-17 23:07:13 PDT
Created attachment 169348 [details]
patch

Done. :)
Comment 16 Ryuan Choi 2012-10-17 23:17:31 PDT
LGTM.
Comment 17 Grzegorz Czajkowski 2012-10-18 00:51:32 PDT
Comment on attachment 169348 [details]
patch

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

> Source/WebKit/efl/tests/test_ewk_frame.cpp:37
> +    buffer = 0;

If you'd like to save NULL to this pointer the buffer should be passed as char** buffer. This works as a local variable.
Comment 18 Kangil Han 2012-10-18 01:04:14 PDT
Created attachment 169364 [details]
patch

Done. :)
Comment 19 WebKit Review Bot 2012-10-18 01:25:53 PDT
Comment on attachment 169364 [details]
patch

Clearing flags on attachment: 169364

Committed r131715: <http://trac.webkit.org/changeset/131715>
Comment 20 WebKit Review Bot 2012-10-18 01:25:59 PDT
All reviewed patches have been landed.  Closing bug.