WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
95984
[EFL] Add unit test for ewk_frame_source_get.
https://bugs.webkit.org/show_bug.cgi?id=95984
Summary
[EFL] Add unit test for ewk_frame_source_get.
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.
Top of Page
Format For Printing
XML
Clone This Bug