Bug 90606 - [EFL] Port the test framework to WebKit 2
Summary: [EFL] Port the test framework to WebKit 2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thiago Marcos P. Santos
URL:
Keywords:
: 68510 (view as bug list)
Depends on: 90602
Blocks: 90451 90973
  Show dependency treegraph
 
Reported: 2012-07-05 05:15 PDT by Thiago Marcos P. Santos
Modified: 2012-08-15 11:20 PDT (History)
10 users (show)

See Also:


Attachments
Patch (21.28 KB, patch)
2012-07-05 10:57 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (21.14 KB, patch)
2012-07-06 01:19 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (21.11 KB, patch)
2012-07-06 03:55 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (20.77 KB, patch)
2012-07-06 05:38 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (12.41 KB, patch)
2012-07-09 11:08 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (12.41 KB, patch)
2012-07-09 11:47 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (16.77 KB, patch)
2012-07-10 14:57 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (16.74 KB, patch)
2012-07-12 05:51 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago Marcos P. Santos 2012-07-05 05:15:40 PDT
No excuses for API without tests.
Comment 1 Thiago Marcos P. Santos 2012-07-05 10:57:27 PDT
Created attachment 150962 [details]
Patch
Comment 2 Krzysztof Czech 2012-07-06 00:02:32 PDT
> void EWK2TestBase::shutdown()
> {
>    ecore_evas_shutdown();
> }

EWK2TestBase::shutdown is not used. I think it can be deleted as well as in wk1 tests
Comment 3 Thiago Marcos P. Santos 2012-07-06 01:19:22 PDT
Created attachment 151034 [details]
Patch

Unused method removed. Thanks for reviewing.
Comment 4 Krzysztof Czech 2012-07-06 01:33:53 PDT
LGTM, thanks
Comment 5 Thiago Marcos P. Santos 2012-07-06 03:55:04 PDT
Created attachment 151049 [details]
Patch

Removed unused include path.
Comment 6 Thiago Marcos P. Santos 2012-07-06 05:38:26 PDT
Created attachment 151065 [details]
Patch

Cleanup more unused include paths.
Comment 7 Raphael Kubo da Costa (:rakuco) 2012-07-06 13:40:56 PDT
Comment on attachment 151065 [details]
Patch

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

Do you need to add Intel's copyright to any header?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.cpp:27
> +int EWK2UnitTests::EWK2TestBase::useX11Window;

Shouldn't you add a default value here?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.cpp:40
> +    int count = 0;
> +
> +    while ((count = ecore_evas_shutdown()) > 0) { }

Isn't it better to call ecore_evas_shutdown() only once and ASSERT that itit returns so that faulty code gets evident more easily?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.h:63
> +    static int useX11Window;

Isn't it possible to use a bool here?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestConfig.h:23
> +namespace EWK2UnitTests {
> +namespace Config {

Nit: we normally put an empty line after each namespace declaration.
Comment 8 Thiago Marcos P. Santos 2012-07-06 14:03:34 PDT
(In reply to comment #7)
> (From update of attachment 151065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151065&action=review
> 
> Do you need to add Intel's copyright to any header?
> 

This is basically a copy from WK1 test framework skeleton. Nothing was added, so I kept the original copyright holder since no significant changes (more than 10 lines per file) were made.  

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.cpp:27
> > +int EWK2UnitTests::EWK2TestBase::useX11Window;
> 
> Shouldn't you add a default value here?

I guess since it is static, it is zeroed by default.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.cpp:40
> > +    int count = 0;
> > +
> > +    while ((count = ecore_evas_shutdown()) > 0) { }
> 
> Isn't it better to call ecore_evas_shutdown() only once and ASSERT that itit returns so that faulty code gets evident more easily?

Yes, gonna change that.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.h:63
> > +    static int useX11Window;
> 
> Isn't it possible to use a bool here?

I guess it is int because of compatibility with getopt_long()

> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestConfig.h:23
> > +namespace EWK2UnitTests {
> > +namespace Config {
> 
> Nit: we normally put an empty line after each namespace declaration.

Roger that.
Comment 9 Thiago Marcos P. Santos 2012-07-06 15:30:40 PDT
> > 
> > > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.cpp:40
> > > +    int count = 0;
> > > +
> > > +    while ((count = ecore_evas_shutdown()) > 0) { }
> > 
> > Isn't it better to call ecore_evas_shutdown() only once and ASSERT that itit returns so that faulty code gets evident more easily?
> 
> Yes, gonna change that.
> 

Actually I double checked this on the Ecore docs [1]. It actually makes sense since it returns > 0 when it is still in use. We have too many asynchronous events ongoing and on my tests here it rarely return 0 at the first attempt.

[1] http://docs.enlightenment.org/auto/ecore/group__Ecore__Evas__Group.html#gab8fa311077f749190c9b622f672b2214
Comment 10 Raphael Kubo da Costa (:rakuco) 2012-07-06 15:48:24 PDT
(In reply to comment #9)
> Actually I double checked this on the Ecore docs [1]. It actually makes sense since it returns > 0 when it is still in use. We have too many asynchronous events ongoing and on my tests here it rarely return 0 at the first attempt.

It does what all _shutdown() EFL functions do. If the number of calls to _init() and _shutdown() are properly balanced, in the end the counters should all return 0.
Comment 11 Thiago Marcos P. Santos 2012-07-09 11:08:48 PDT
Created attachment 151284 [details]
Patch
Comment 12 Thiago Marcos P. Santos 2012-07-09 11:47:19 PDT
Created attachment 151289 [details]
Patch
Comment 13 Thiago Marcos P. Santos 2012-07-09 11:57:34 PDT
Ok, after the feedback, the patch was completely reworked. Waiting for comments.

Please note that it is now more gtest-ish.
Comment 14 Gyuyoung Kim 2012-07-09 19:57:56 PDT
Comment on attachment 151289 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:31
> +static void onLoadProgress(void *userData, Evas_Object *webView, void *eventInfo)

nit : Move * to variable type side.
Comment 15 Raphael Kubo da Costa (:rakuco) 2012-07-09 20:32:52 PDT
Comment on attachment 151289 [details]
Patch

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

> Source/WebKit2/PlatformEfl.cmake:215
> +    TARGET_LINK_LIBRARIES(${testName} ${EWK2UnitTests_LIBRARIES} ewk2UnitTestUtils gtest pthread)

Do you really need to pass pthread here? If it's really necessary, you should use find_package(Threads) and link to ${CMAKE_THREAD_LIBS_INIT}.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:62
> +    m_webView = ewk_view_add(evas);

Isn't this leaking? We have an OwnPtr overload for Evas_Object* that's probably handy here.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:70
> +    ecore_evas_free(m_ecoreEvas);

We have an OwnPtr overload for Ecore_Evas*. You can use it here and avoid having to manually keep track of the memory.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:74
> +

Nit: extra empty line.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:83
> +    while (m_loadProgress != 1.f)
> +        ecore_main_loop_iterate();

Where's the main loop being started and shut down? Do you really need to busy wait like this?
Comment 16 Krzysztof Czech 2012-07-10 00:49:57 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=151289&action=review

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:44
> +    Evas_Object* m_webView;

I think OwnPtr can be used instead.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:45
> +    Ecore_Evas* m_ecoreEvas;

I think OwnPtr can be used instead.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:48
> +    unsigned int m_height;

I see m_width, m_height are hardcoded, so maybe it can be changed to
static const int m_width = 800;
static const int m_height = 600;
Comment 17 Thiago Marcos P. Santos 2012-07-10 04:14:04 PDT
(In reply to comment #15)
> (From update of attachment 151289 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151289&action=review
> 
> > Source/WebKit2/PlatformEfl.cmake:215
> > +    TARGET_LINK_LIBRARIES(${testName} ${EWK2UnitTests_LIBRARIES} ewk2UnitTestUtils gtest pthread)
> 
> Do you really need to pass pthread here? If it's really necessary, you should use find_package(Threads) and link to ${CMAKE_THREAD_LIBS_INIT}.
> 

It is need by gtest, so maybe it is in the wrong place. Gonna verify that.

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:62
> > +    m_webView = ewk_view_add(evas);
> 
> Isn't this leaking? We have an OwnPtr overload for Evas_Object* that's probably handy here.
> 

I'm going to double check ecore_evas source code (since the docs doesn't say much), but I assumed that ecore_evas_free was in charge to clean up it's children. My tests using valgrind + system malloc doesn't show any leak though. Maybe I'm been coding Qt for too long. :)

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:70
> > +    ecore_evas_free(m_ecoreEvas);
> 
> We have an OwnPtr overload for Ecore_Evas*. You can use it here and avoid having to manually keep track of the memory.
> 

The reason why I'm not using OwnPtr is because I want to ensure coherency. Not sure if it is a good idea to destruct them after ecore_evas_shutdown() (which would be the case when using auto pointers).

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:74
> > +
> 
> Nit: extra empty line.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:83
> > +    while (m_loadProgress != 1.f)
> > +        ecore_main_loop_iterate();
> 
> Where's the main loop being started and shut down? Do you really need to busy wait like this?

This is not a busy wait, it is totally event/timeout oriented (pretty much what the main loop does). Just a convenience function to load a page synchronously since I'm expecting to most of the tests are going to be "load a page, verify getters". This way you can write your tests inline.

Qt utests uses title change events to ensure a certain state inside the page, since even when the page is 100% loaded you might still have some JS executing. I should write something similar to wait for some title changes/value synchronously. This makes a lot easier and cleaner to write tests.

If needed, each test should start the main_loop and stop it when is most appropriated.
Comment 18 Thiago Marcos P. Santos 2012-07-10 04:14:49 PDT
(In reply to comment #16)
> View in context: https://bugs.webkit.org/attachment.cgi?id=151289&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:44
> > +    Evas_Object* m_webView;
> 
> I think OwnPtr can be used instead.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:45
> > +    Ecore_Evas* m_ecoreEvas;
> 
> I think OwnPtr can be used instead.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:48
> > +    unsigned int m_height;
> 
> I see m_width, m_height are hardcoded, so maybe it can be changed to
> static const int m_width = 800;
> static const int m_height = 600;

Maybe I should put these globals at ::testing::Environment.
Comment 19 Thiago Marcos P. Santos 2012-07-10 14:57:28 PDT
Created attachment 151532 [details]
Patch

The pthreads issue is going to be fixed in another bug. I'm going to make gtest a shared library now that we have many customers for it (TestWebKitAPI is also comming), and fix the link problem for wk1 and wk2 tests. I still not using OwnPtr because of destruction order coherence.
Comment 20 Thiago Marcos P. Santos 2012-07-11 04:46:47 PDT
(In reply to comment #19)
> Created an attachment (id=151532) [details]
> Patch
> 
> The pthreads issue is going to be fixed in another bug. I'm going to make gtest a shared library now that we have many customers for it (TestWebKitAPI is also comming), and fix the link problem for wk1 and wk2 tests. I still not using OwnPtr because of destruction order coherence.

Here: bug 90973
Comment 21 Raphael Kubo da Costa (:rakuco) 2012-07-11 19:42:18 PDT
Right now I only have some questions left:

 - You mention not being able to use OwnPtr with the init/shutdown stuff. I'm not very familiar with how gtest is supposed to work; wouldn't it be possible to init and shutdown in the constructor and the destructor, respectively?

 - On a side note, it's generally good practice to init and shutdown everything you use (evas, for example).

 - On loadUrlSync don't you actually need to init and shut down the main loop since you are using it?
Comment 22 Thiago Marcos P. Santos 2012-07-12 01:08:33 PDT
(In reply to comment #21)
> Right now I only have some questions left:
> 
>  - You mention not being able to use OwnPtr with the init/shutdown stuff. I'm not very familiar with how gtest is supposed to work; wouldn't it be possible to init and shutdown in the constructor and the destructor, respectively?
> 

Yes, it is possible, but it's not a good idea since we use ASSERT_GT on the constructor and gtest team want to make assertions throw exceptions.

>  - On a side note, it's generally good practice to init and shutdown everything you use (evas, for example).
> 

Yes, the latest version of the patch deletes everything. I found no evidence of ecore_evas_free deleting it's children.

>  - On loadUrlSync don't you actually need to init and shut down the main loop since you are using it?

No. Actually that's the point of loadUrlSync.
Comment 23 Thiago Marcos P. Santos 2012-07-12 01:14:52 PDT
> >  - You mention not being able to use OwnPtr with the init/shutdown stuff. I'm not very familiar with how gtest is supposed to work; wouldn't it be possible to init and shutdown in the constructor and the destructor, respectively?
> > 
> 
> Yes, it is possible, but it's not a good idea since we use ASSERT_GT on the constructor and gtest team want to make assertions throw exceptions.

s/constructor/SetUp
Comment 24 Chris Dumez 2012-07-12 01:21:57 PDT
Comment on attachment 151532 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:43
> +    : m_loadProgress(0.f)

Use 0, not 0.f. This is part of the coding style.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:49
> +void EWK2UnitTestBase::SetUp()

Why does this start with a capital letter?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:67
> +    evas_object_focus_set(m_webView, EINA_TRUE);

Should be true, not EINA_TRUE.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:70
> +void EWK2UnitTestBase::TearDown()

Why does this start with a capital letter?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:79
> +    m_loadProgress = 0;

Should we have an EINA SAFETY for url?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:84
> +    while (m_loadProgress != 1.f)

Use 1, not 1.f, as per coding style.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:32
> +    Evas_Object* webView() { return m_webView; }

Should be const.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:49
> +}

// namespace EWK2UnitTest

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:51
> +#endif

// EWK2UnitTestBase_h

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:37
> +}

// namespace EWK2UnitTest

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.h:45
> +#endif

// EWK2UnitTestEnvironment_h
Comment 25 Thiago Marcos P. Santos 2012-07-12 01:50:58 PDT
(In reply to comment #24)
> (From update of attachment 151532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151532&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:43
> > +    : m_loadProgress(0.f)
> 
> Use 0, not 0.f. This is part of the coding style.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:49
> > +void EWK2UnitTestBase::SetUp()
> 
> Why does this start with a capital letter?
> 

This is enforced by gtest.

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:67
> > +    evas_object_focus_set(m_webView, EINA_TRUE);
> 
> Should be true, not EINA_TRUE.
> 

Docs says Eina_Bool.

http://docs.enlightenment.org/auto/evas/group__Evas__Object__Group__Basic.html#ga114336ce1de94dbb13843cca2516e34b

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:70
> > +void EWK2UnitTestBase::TearDown()
> 
> Why does this start with a capital letter?
> 

Enforced by gtest.

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:79
> > +    m_loadProgress = 0;
> 
> Should we have an EINA SAFETY for url?
> 

I don't think so. I'm not accessing "url", just handling it directly to EWK. The check would be at the one that changes/reads it.

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:84
> > +    while (m_loadProgress != 1.f)
> 
> Use 1, not 1.f, as per coding style.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:32
> > +    Evas_Object* webView() { return m_webView; }
> 
> Should be const.
> 

Not really. What about const correctness? I've seen WebKit respecting this rule in many places:

Example -> CSSToStyleMap* styleMap() { return &m_styleMap; }

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:49
> > +}
> 
> // namespace EWK2UnitTest
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:51
> > +#endif
> 
> // EWK2UnitTestBase_h
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:37
> > +}
> 
> // namespace EWK2UnitTest
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.h:45
> > +#endif
> 
> // EWK2UnitTestEnvironment_h
Comment 26 Thiago Marcos P. Santos 2012-07-12 05:51:41 PDT
Created attachment 151925 [details]
Patch

Thanks for having a look.
Comment 27 Kenneth Rohde Christiansen 2012-07-12 09:27:54 PDT
Comment on attachment 151925 [details]
Patch

Looks fine
Comment 28 Raphael Kubo da Costa (:rakuco) 2012-07-12 13:13:00 PDT
(In reply to comment #22)
> >  - On a side note, it's generally good practice to init and shutdown everything you use (evas, for example).
> Yes, the latest version of the patch deletes everything. I found no evidence of ecore_evas_free deleting it's children.

I mean, you should probably call evas_init() and evas_shutdown().

> >  - On loadUrlSync don't you actually need to init and shut down the main loop since you are using it?
> No. Actually that's the point of loadUrlSync.

So you can/should iterate without starting the main loop?

(In reply to comment #25)
> (In reply to comment #24)
> > > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:32
> > > +    Evas_Object* webView() { return m_webView; }
> > 
> > Should be const.
> Not really. What about const correctness? I've seen WebKit respecting this rule in many places:
> 
> Example -> CSSToStyleMap* styleMap() { return &m_styleMap; }

It depends on whether callers are expected to call functions which take a non-const Evas_Object*. Right now only ewk_view_uri_get() is called, but I guess non-const functions will be called at some point.
Comment 29 Chris Dumez 2012-07-12 13:22:16 PDT
(In reply to comment #28)
> (In reply to comment #22)
> > >  - On a side note, it's generally good practice to init and shutdown everything you use (evas, for example).
> > Yes, the latest version of the patch deletes everything. I found no evidence of ecore_evas_free deleting it's children.
> 
> I mean, you should probably call evas_init() and evas_shutdown().
> 
> > >  - On loadUrlSync don't you actually need to init and shut down the main loop since you are using it?
> > No. Actually that's the point of loadUrlSync.
> 
> So you can/should iterate without starting the main loop?
> 
> (In reply to comment #25)
> > (In reply to comment #24)
> > > > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:32
> > > > +    Evas_Object* webView() { return m_webView; }
> > > 
> > > Should be const.
> > Not really. What about const correctness? I've seen WebKit respecting this rule in many places:
> > 
> > Example -> CSSToStyleMap* styleMap() { return &m_styleMap; }
> 
> It depends on whether callers are expected to call functions which take a non-const Evas_Object*. Right now only ewk_view_uri_get() is called, but I guess non-const functions will be called at some point.

I meant that the getter should be const since it does not alter the object, not a const return value. For e.g.:

inline HTMLFrameOwnerElement* Frame::ownerElement() const
{
    return m_ownerElement;
}
Comment 30 Raphael Kubo da Costa (:rakuco) 2012-07-12 14:28:20 PDT
Comment on attachment 151925 [details]
Patch

cq+'ing so Thiago can go to bed in peace :-)

I finally understood what Chris was trying to say, and I agree with him. Thiago seems to be OK with it as well, but he'll do that in another patch.
Comment 31 Thiago Marcos P. Santos 2012-07-12 14:35:42 PDT
(In reply to comment #30)
> (From update of attachment 151925 [details])
> cq+'ing so Thiago can go to bed in peace :-)
> 
> I finally understood what Chris was trying to say, and I agree with him. Thiago seems to be OK with it as well, but he'll do that in another patch.

I will ignore what I learned from the Scott Meyers books in favor of consistence with the rest of EFL port. :-P
Comment 32 WebKit Review Bot 2012-07-12 14:46:18 PDT
Comment on attachment 151925 [details]
Patch

Clearing flags on attachment: 151925

Committed r122512: <http://trac.webkit.org/changeset/122512>
Comment 33 WebKit Review Bot 2012-07-12 14:46:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Thiago Marcos P. Santos 2012-08-15 11:20:31 PDT
*** Bug 68510 has been marked as a duplicate of this bug. ***