Bug 95984

Summary:

[EFL] Add unit test for ewk_frame_source_get.

Product:

WebKit

Reporter:

Kangil Han <kangil.han>

Component:

WebKit EFL

Assignee:

Kangil Han <kangil.han>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

gyuyoung.kim, kenneth, laszlo.gombos, lucas.de.marchi, rakuco, ryuan.choi, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

Linux

Bug Depends on:

94925

Bug Blocks:

Attachments:

Description	Flags
patch	none
patch	none
patch	none
patch	gyuyoung.kim: review+, g.czajkowski: commit-queue-
patch	none

Kangil Han

Reported 2012-09-06 07:55:54 PDT

To test if WebKit automatically insert <html></html> tags.

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Kangil Han

Comment 1 2012-10-08 21:40:42 PDT

Created attachment 167686 [details] patch

Ryuan Choi

Comment 2 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 ?

Grzegorz Czajkowski

Comment 3 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.

Gyuyoung Kim

Comment 4 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 ?

Ryuan Choi

Comment 5 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

Kenneth Rohde Christiansen

Comment 6 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.

Kangil Han

Comment 7 2012-10-17 02:06:13 PDT

Created attachment 169130 [details] patch

Kenneth Rohde Christiansen

Comment 8 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?

Gyuyoung Kim

Comment 9 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.

Grzegorz Czajkowski

Comment 10 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.

Kangil Han

Comment 11 2012-10-17 17:35:11 PDT

Created attachment 169312 [details] patch Done. :)

Kangil Han

Comment 12 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. :-(

Gyuyoung Kim

Comment 13 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.

Kangil Han

Comment 14 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'. ;)

Kangil Han

Comment 15 2012-10-17 23:07:13 PDT

Created attachment 169348 [details] patch Done. :)

Ryuan Choi

Comment 16 2012-10-17 23:17:31 PDT

LGTM.

Grzegorz Czajkowski

Comment 17 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.

Kangil Han

Comment 18 2012-10-18 01:04:14 PDT

Created attachment 169364 [details] patch Done. :)

WebKit Review Bot

Comment 19 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>

WebKit Review Bot

Comment 20 2012-10-18 01:25:59 PDT

All reviewed patches have been landed. Closing bug.

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