Bug 95923

Summary: [EFL] Implement GCActivityCallback
Product: WebKit Reporter: Hojong Han <hojong.han>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, barraclough, cdumez, commit-queue, d-r, ggaren, gyuyoung.kim, hausmann, kenneth, laszlo.gombos, mhahnenberg, rakuco, tmpsantos, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Bug Depends on: 96045    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Hojong Han 2012-09-05 18:51:17 PDT
to add a GCActivityCallbackEfl.cpp, 
and implement GCActivityCallback with platform timer
Comment 1 Hojong Han 2012-09-05 18:59:37 PDT
Created attachment 162393 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-09-06 06:24:04 PDT
Comment on attachment 162393 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Implement GCActivityCallback and HeapTimer for EFL port.

It would be good if you could explain what you need these for.
Comment 3 Gyuyoung Kim 2012-09-06 17:37:15 PDT
Comment on attachment 162393 [details]
Patch

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

> Source/JavaScriptCore/PlatformEfl.cmake:2
> +    runtime/GCActivityCallback.cpp

In this file case, I think we're able to remove this file in CMakeLists.txt. Then, we can add a port specific file to PlatformXXX.cmake by port itself. So, I'm going to file a bug for this.

> Source/JavaScriptCore/PlatformEfl.cmake:10
> +    runtime/GCActivityCallbackEfl.cpp

Wrong alphabetical order.

> Source/JavaScriptCore/PlatformEfl.cmake:19
> +    ${ECORE_X_INCLUDE_DIRS}

ditto.

> Source/JavaScriptCore/heap/HeapTimer.cpp:147
> +        ecore_timer_del(m_timer);

A new line ?

> Source/JavaScriptCore/heap/HeapTimer.h:77
> +    Ecore_Timer* m_timer;

Can't you use OwnPtr here ? If you can use OwnPtr for this, you'd be able to remove most of the checks for m_timer as well as calls to ecore_timer_del() in this patch.

See also, RunLoopEfl.h / cpp.
Comment 4 Hojong Han 2012-09-06 19:37:17 PDT
Created attachment 162655 [details]
Patch
Comment 5 Hojong Han 2012-09-06 19:38:05 PDT
(In reply to comment #2)
> (From update of attachment 162393 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162393&action=review
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        Implement GCActivityCallback and HeapTimer for EFL port.
> 
> It would be good if you could explain what you need these for.
These are for efficient use of JSC heap.
JSC heap grows rapidly at some web sites and it causes fragmentation a lot in marked blocks so that we put the codes calling collect without sweep according to the allocated size in order to make use of available spaces in marked blocks better.
Comment 6 Simon Hausmann 2012-09-06 23:11:20 PDT
I see an issue here that is not EFL specific:

The only thing that is not cross-platform in an implementation of HeapTimer and GCActivityCallback is the timer. The rest is copy & paste across the ports. Wouldn't it be nicer to have the simple runloop timer functionality needed here abstracted away in WTF and make these two JSC classes cross-platform?

Kenneth, what do you think? Doesn't this smell like an opportunity for code sharing? :)
Comment 7 Kenneth Rohde Christiansen 2012-09-07 00:49:53 PDT
(In reply to comment #6)
> I see an issue here that is not EFL specific:
> 
> The only thing that is not cross-platform in an implementation of HeapTimer and GCActivityCallback is the timer. The rest is copy & paste across the ports. Wouldn't it be nicer to have the simple runloop timer functionality needed here abstracted away in WTF and make these two JSC classes cross-platform?
> 
> Kenneth, what do you think? Doesn't this smell like an opportunity for code sharing? :)

I think that sounds like a good idea.
Comment 8 Gyuyoung Kim 2012-09-07 01:05:05 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > I see an issue here that is not EFL specific:
> > 
> > The only thing that is not cross-platform in an implementation of HeapTimer and GCActivityCallback is the timer. The rest is copy & paste across the ports. Wouldn't it be nicer to have the simple runloop timer functionality needed here abstracted away in WTF and make these two JSC classes cross-platform?
> > 
> > Kenneth, what do you think? Doesn't this smell like an opportunity for code sharing? :)
> 
> I think that sounds like a good idea.

I think so. BTW, do you think this patch supports it as well? Isn’t it better to resolve that in a new bug after finishing this bug ? :-)
Comment 9 Hojong Han 2012-09-10 18:21:00 PDT
Geoffrey, 
Could you take a look into this patch?
Give me any feedback in terms of JSC heap.
Comment 10 Gyuyoung Kim 2012-11-24 02:21:21 PST
Comment on attachment 162393 [details]
Patch

Hojong, I wonder if EFL port still needs to have this patch.
Comment 11 Hojong Han 2012-11-25 17:00:47 PST
(In reply to comment #10)
> (From update of attachment 162393 [details])
> Hojong, I wonder if EFL port still needs to have this patch.

Yes, I think it needs this.
Comment 12 Gyuyoung Kim 2012-12-02 08:50:27 PST
Comment on attachment 162655 [details]
Patch

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

> Source/JavaScriptCore/PlatformEfl.cmake:18
> +    ${ECORE_X_INCLUDE_DIRS}

Why do you incluce ECORE_X instead of ECORE ? There is no ecore_x usage in this patch.
Comment 13 Chris Dumez 2012-12-02 09:04:11 PST
Comment on attachment 162655 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Implement GCActivityCallback and HeapTimer for EFL port.

Please explain in the changelog why those are needed (i.e. the benefit of implementing this).
Comment 14 Hojong Han 2012-12-02 20:37:01 PST
Created attachment 177172 [details]
Patch
Comment 15 Hojong Han 2012-12-02 20:38:51 PST
(In reply to comment #12)
> (From update of attachment 162655 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162655&action=review
> 
> > Source/JavaScriptCore/PlatformEfl.cmake:18
> > +    ${ECORE_X_INCLUDE_DIRS}
> 
> Why do you incluce ECORE_X instead of ECORE ? There is no ecore_x usage in this patch.
I changed it to ECORE and added ENIA which should be included with ECORE.
Comment 16 Gyuyoung Kim 2012-12-03 07:41:21 PST
Comment on attachment 177172 [details]
Patch

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

> Source/JavaScriptCore/PlatformEfl.cmake:2
> +    runtime/GCActivityCallback.cpp

As Simon said, I also think this file is not cross-platform. At least, I think we shouldn't remove existing file for port implementation as here. Generally, each port has its own implementation file for similar case. For example, in this case, I think we should have GCActivityCallbackEfl.h/.cpp for now at least. I think you need to file a new bug for this issue regardless of which method is better(Simon's suggestion or support for each port implementation(CF, blackberry, EFL port and so on). Then, I think you can add this patch for EFL port.
Comment 17 Simon Hausmann 2012-12-10 01:36:41 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > I see an issue here that is not EFL specific:
> > > 
> > > The only thing that is not cross-platform in an implementation of HeapTimer and GCActivityCallback is the timer. The rest is copy & paste across the ports. Wouldn't it be nicer to have the simple runloop timer functionality needed here abstracted away in WTF and make these two JSC classes cross-platform?
> > > 
> > > Kenneth, what do you think? Doesn't this smell like an opportunity for code sharing? :)
> > 
> > I think that sounds like a good idea.
> 
> I think so. BTW, do you think this patch supports it as well? Isn’t it better to resolve that in a new bug after finishing this bug ? :-)

Given that this doesn't seem to be urgent (the patch has been hanging around here for a good while now), why not do it all in one shot?
Comment 18 Allan Sandfeld Jensen 2013-01-08 02:53:49 PST
Btw, see my patch in bug #103996 https://bugs.webkit.org/attachment.cgi?id=181678&action=prettypatch

I have tried to split HeapTimer.cpp into platform specific files. EFL would need your own file under that system.
Comment 19 Gyuyoung Kim 2013-01-08 21:38:37 PST
(In reply to comment #18)
> Btw, see my patch in bug #103996 https://bugs.webkit.org/attachment.cgi?id=181678&action=prettypatch
> 
> I have tried to split HeapTimer.cpp into platform specific files. EFL would need your own file under that system.

I'm considering to split HeapTimer.cpp into each port file on Bug 96054. If you land it, I think this patch also should follow it. BTW, how do you think to split GCACtivityCallback.cpp as HeapTimer.cpp ?
Comment 20 Hojong Han 2013-05-21 22:08:39 PDT
Created attachment 202494 [details]
Patch
Comment 21 Gyuyoung Kim 2013-05-21 22:36:56 PDT
Comment on attachment 202494 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        [EFL] Implement GCActivityCallback

It looks you need to change bug title according to new patch.

> Source/JavaScriptCore/PlatformEfl.cmake:2
> +    ${ECORE_X_INCLUDE_DIRS}

It looks you don't use ecore_x. So, ${ECORE_INCLUDE_DIRS} is enough.

> Source/JavaScriptCore/runtime/GCActivityCallback.cpp:-105
> -

Do not modify code unrelated to this patch.

> Source/JavaScriptCore/runtime/GCActivityCallback.cpp:124
> +    stop();

Why do you need to call stop() here ?
Comment 22 Hojong Han 2013-05-21 22:53:22 PDT
(In reply to comment #21)
> (From update of attachment 202494 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202494&action=review
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        [EFL] Implement GCActivityCallback
> 
> It looks you need to change bug title according to new patch.

Title changed to match with the patch.

> > Source/JavaScriptCore/PlatformEfl.cmake:2
> > +    ${ECORE_X_INCLUDE_DIRS}
> 
> It looks you don't use ecore_x. So, ${ECORE_INCLUDE_DIRS} is enough.

yes, ECORE_INCLUDE_DIRS is enough. I'll change it.

> > Source/JavaScriptCore/runtime/GCActivityCallback.cpp:-105
> > -
> 
> Do not modify code unrelated to this patch.

It's just kind of improving readability, but no biggie whether keep it as it was.

> > Source/JavaScriptCore/runtime/GCActivityCallback.cpp:124
> > +    stop();
> 
> Why do you need to call stop() here ?

Doesn't it need to stop the previous before setting new one ?
Comment 23 Hojong Han 2013-05-21 23:03:05 PDT
Created attachment 202500 [details]
Patch
Comment 24 Chris Dumez 2013-05-21 23:10:34 PDT
Comment on attachment 202500 [details]
Patch

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

> Source/JavaScriptCore/heap/HeapTimer.cpp:188
> +    return ECORE_CALLBACK_CANCEL;

You need to set heapTimer->m_timer to 0 before returning. When you return ECORE_CALLBACK_CANCEL, the handle will become invalid and ecore_timer_del(m_timer) will fail in the destructor.

> Source/JavaScriptCore/heap/HeapTimer.h:42
> +#include <Ecore.h>

A forward declaration of Ecore_Timer would suffice here. And this include can be moved to the cpp.

> Source/JavaScriptCore/heap/HeapTimer.h:87
> +    void stop()

Can you put the method body in the cpp file instead?

> Source/JavaScriptCore/heap/HeapTimer.h:89
> +        if (m_timer) {

We usually do the opposite in WebKit, return early.
If (!m_timer)
    return;

> Source/JavaScriptCore/runtime/GCActivityCallback.cpp:127
> +    m_timer = ecore_timer_add(newDelay, reinterpret_cast<Ecore_Task_Cb>(HeapTimer::timerEvent), this);

I would add a ASSERT(!m_timer); right before this line to make sure we never leak.
Comment 25 Hojong Han 2013-05-21 23:41:59 PDT
Comment on attachment 202500 [details]
Patch

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

>> Source/JavaScriptCore/heap/HeapTimer.cpp:188
>> +    return ECORE_CALLBACK_CANCEL;
> 
> You need to set heapTimer->m_timer to 0 before returning. When you return ECORE_CALLBACK_CANCEL, the handle will become invalid and ecore_timer_del(m_timer) will fail in the destructor.

I'll apply your review to the patch.

>> Source/JavaScriptCore/heap/HeapTimer.h:42
>> +#include <Ecore.h>
> 
> A forward declaration of Ecore_Timer would suffice here. And this include can be moved to the cpp.

Do you mean that it'd better move "#include <Ecore.h>" to the cpp?

>> Source/JavaScriptCore/heap/HeapTimer.h:87
>> +    void stop()
> 
> Can you put the method body in the cpp file instead?

This'll be moved as your comment.

>> Source/JavaScriptCore/heap/HeapTimer.h:89
>> +        if (m_timer) {
> 
> We usually do the opposite in WebKit, return early.
> If (!m_timer)
>     return;

This'll be moved as your comment.

>> Source/JavaScriptCore/runtime/GCActivityCallback.cpp:127
>> +    m_timer = ecore_timer_add(newDelay, reinterpret_cast<Ecore_Task_Cb>(HeapTimer::timerEvent), this);
> 
> I would add a ASSERT(!m_timer); right before this line to make sure we never leak.

I'll fix to set m_timer as 0 in stop().
Comment 26 Chris Dumez 2013-05-21 23:45:12 PDT
Comment on attachment 202500 [details]
Patch

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

>>> Source/JavaScriptCore/heap/HeapTimer.h:42
>>> +#include <Ecore.h>
>> 
>> A forward declaration of Ecore_Timer would suffice here. And this include can be moved to the cpp.
> 
> Do you mean that it'd better move "#include <Ecore.h>" to the cpp?

Yes, and have a forward declaration here:
typedef struct _Ecore_Timer Ecore_Timer;

>>> Source/JavaScriptCore/runtime/GCActivityCallback.cpp:127
>>> +    m_timer = ecore_timer_add(newDelay, reinterpret_cast<Ecore_Task_Cb>(HeapTimer::timerEvent), this);
>> 
>> I would add a ASSERT(!m_timer); right before this line to make sure we never leak.
> 
> I'll fix to set m_timer as 0 in stop().

You already set m_timer to 0 in stop. All I ask for in an extra assert to make sure someone does not remove the stop() later on :)
Comment 27 Gyuyoung Kim 2013-05-21 23:51:04 PDT
Comment on attachment 202500 [details]
Patch

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

> Source/JavaScriptCore/heap/HeapTimer.cpp:174
> +    ecore_init();

Why do you need to call ecore_init() again though it is already called during the webprocess initialization ?

> Source/JavaScriptCore/heap/HeapTimer.cpp:180
> +    ecore_shutdown();

ditto.
Comment 28 Hojong Han 2013-05-21 23:56:28 PDT
Comment on attachment 202500 [details]
Patch

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

>>>> Source/JavaScriptCore/heap/HeapTimer.h:42
>>>> +#include <Ecore.h>
>>> 
>>> A forward declaration of Ecore_Timer would suffice here. And this include can be moved to the cpp.
>> 
>> Do you mean that it'd better move "#include <Ecore.h>" to the cpp?
> 
> Yes, and have a forward declaration here:
> typedef struct _Ecore_Timer Ecore_Timer;

HeapTimer.h is included in GCActivityCallback.h and there's Ecore_Task_Cb in GCActivityCallback.cpp.
What do you think keeping it in HeapTimer.h?

>>>> Source/JavaScriptCore/runtime/GCActivityCallback.cpp:127
>>>> +    m_timer = ecore_timer_add(newDelay, reinterpret_cast<Ecore_Task_Cb>(HeapTimer::timerEvent), this);
>>> 
>>> I would add a ASSERT(!m_timer); right before this line to make sure we never leak.
>> 
>> I'll fix to set m_timer as 0 in stop().
> 
> You already set m_timer to 0 in stop. All I ask for in an extra assert to make sure someone does not remove the stop() later on :)

yes, it's good to apply for the case as you mentioned.
Comment 29 Chris Dumez 2013-05-22 00:08:29 PDT
Comment on attachment 202500 [details]
Patch

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

>>>>> Source/JavaScriptCore/heap/HeapTimer.h:42
>>>>> +#include <Ecore.h>
>>>> 
>>>> A forward declaration of Ecore_Timer would suffice here. And this include can be moved to the cpp.
>>> 
>>> Do you mean that it'd better move "#include <Ecore.h>" to the cpp?
>> 
>> Yes, and have a forward declaration here:
>> typedef struct _Ecore_Timer Ecore_Timer;
> 
> HeapTimer.h is included in GCActivityCallback.h and there's Ecore_Task_Cb in GCActivityCallback.cpp.
> What do you think keeping it in HeapTimer.h?

No, please add the proper include to GCActivityCallback.cpp then.
Comment 30 Hojong Han 2013-05-22 00:11:49 PDT
Comment on attachment 202500 [details]
Patch

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

>> Source/JavaScriptCore/heap/HeapTimer.cpp:174
>> +    ecore_init();
> 
> Why do you need to call ecore_init() again though it is already called during the webprocess initialization ?

There's a binary to run in shell. That binary runs without ecore_init but ecore_timer is in GCActivityCallback.
Comment 31 Hojong Han 2013-05-22 00:20:39 PDT
Created attachment 202503 [details]
Patch
Comment 32 Gyuyoung Kim 2013-05-22 00:28:46 PDT
(In reply to comment #30)
> (From update of attachment 202500 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202500&action=review
> 
> >> Source/JavaScriptCore/heap/HeapTimer.cpp:174
> >> +    ecore_init();
> > 
> > Why do you need to call ecore_init() again though it is already called during the webprocess initialization ?
> 
> There's a binary to run in shell. That binary runs without ecore_init but ecore_timer is in GCActivityCallback.

If so, how about adding it to main() of Jsc.cpp ? I don't agree to call ecore_init() / ecore_shutdown() again here. I heard that there may be problems inside EFL when we call it twice.
Comment 33 Chris Dumez 2013-05-22 00:35:21 PDT
Comment on attachment 202503 [details]
Patch

Similarly to the BlackBerry implementation, your HeapTimer implementation will only work for the main thread. I would make that obvious with a comment and a check in the constructor, as BlackBerry is doing. Please make sure to test your patch in debug mode to make sure you're not introducing crashes.
Comment 34 Chris Dumez 2013-05-22 00:38:10 PDT
(In reply to comment #32)
> (In reply to comment #30)
> > (From update of attachment 202500 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202500&action=review
> > 
> > >> Source/JavaScriptCore/heap/HeapTimer.cpp:174
> > >> +    ecore_init();
> > > 
> > > Why do you need to call ecore_init() again though it is already called during the webprocess initialization ?
> > 
> > There's a binary to run in shell. That binary runs without ecore_init but ecore_timer is in GCActivityCallback.
> 
> If so, how about adding it to main() of Jsc.cpp ? I don't agree to call ecore_init() / ecore_shutdown() again here. I heard that there may be problems inside EFL when we call it twice.

if you ecore_init() more than once, it should merely increase a ref count and not do any processing.
Comment 35 Hojong Han 2013-05-22 00:38:11 PDT
Comment on attachment 202500 [details]
Patch

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

>>>> Source/JavaScriptCore/heap/HeapTimer.cpp:174
>>>> +    ecore_init();
>>> 
>>> Why do you need to call ecore_init() again though it is already called during the webprocess initialization ?
>> 
>> There's a binary to run in shell. That binary runs without ecore_init but ecore_timer is in GCActivityCallback.
> 
> If so, how about adding it to main() of Jsc.cpp ? I don't agree to call ecore_init() / ecore_shutdown() again here. I heard that there may be problems inside EFL when we call it twice.

Good to move those in Jsc.cpp even if there isn't any specific problem when calling it more than twice as I know of :)

>> Source/JavaScriptCore/heap/HeapTimer.cpp:180
>> +    ecore_shutdown();
> 
> ditto.

ditto.
Comment 36 Hojong Han 2013-05-22 00:58:29 PDT
Created attachment 202505 [details]
Patch
Comment 37 Chris Dumez 2013-05-22 01:09:57 PDT
Comment on attachment 202505 [details]
Patch

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

> Source/JavaScriptCore/heap/HeapTimer.cpp:175
> +{

Please see my earlier comment about adding a thread check here.

> Source/JavaScriptCore/jsc.cpp:805
> +    ShellEnvironment env;

Why add this to jscmain instead of main() like other ports?
Comment 38 Gyuyoung Kim 2013-05-22 01:31:30 PDT
Comment on attachment 202505 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Implements the activity triggered garbage collector.

It looks this description is insufficient for this patch. Please add more detailed description.
Comment 39 Hojong Han 2013-05-22 01:33:37 PDT
Comment on attachment 202505 [details]
Patch

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

>> Source/JavaScriptCore/heap/HeapTimer.cpp:175
>> +{
> 
> Please see my earlier comment about adding a thread check here.

HeapTimer can be created in other than main thread, but it does not make m_timer run because other threads might not have an ecore loop.
In this vein, ecore_init() in constructor and ecore_shutdown() in destructor are necessary.

>> Source/JavaScriptCore/jsc.cpp:805
>> +    ShellEnvironment env;
> 
> Why add this to jscmain instead of main() like other ports?

I'll remove this if my comment upper is acceptable
Comment 40 Hojong Han 2013-05-22 05:04:53 PDT
Created attachment 202524 [details]
Patch

GCActivityCallback works only in main thread
Comment 41 WebKit Commit Bot 2013-05-22 05:07:22 PDT
Attachment 202524 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/PlatformEfl.cmake', u'Source/JavaScriptCore/heap/HeapTimer.cpp', u'Source/JavaScriptCore/heap/HeapTimer.h', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/runtime/GCActivityCallback.cpp', u'Source/JavaScriptCore/runtime/GCActivityCallback.h']" exit_code: 1
Source/JavaScriptCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 42 Hojong Han 2013-05-22 05:13:56 PDT
Created attachment 202526 [details]
Patch

style error fixed
Comment 43 Gyuyoung Kim 2013-05-22 19:29:41 PDT
Comment on attachment 202526 [details]
Patch

Looks good to me for EFL port. However, I'd like to know how does JS reviewer think about this patch. Geoffrey, what do you think about this ?
Comment 44 Geoffrey Garen 2013-06-03 17:12:13 PDT
Comment on attachment 202526 [details]
Patch

Please use #endif followed by #if instead of #elif. For platform #ifdefs, that makes it easier to edit code that's not related to your particular port.
Comment 45 Hojong Han 2013-06-03 18:16:25 PDT
(In reply to comment #44)
> (From update of attachment 202526 [details])
> Please use #endif followed by #if instead of #elif. For platform #ifdefs, that makes it easier to edit code that's not related to your particular port.
Thanks for review, Geoffrey.
But one thing not clear is which #elifs you meant. #elifs for QT and BlackBerry had been used so that I complied with those.
Do you want every #elif, on this patch, to follow your guide or just several parts such as including header file?
Comment 46 Geoffrey Garen 2013-06-03 19:59:51 PDT
Ah, I didn't realize. I guess we can save the #if/#elif cleanup for a follow-up patch.
Comment 47 WebKit Commit Bot 2013-06-03 20:20:30 PDT
Comment on attachment 202526 [details]
Patch

Clearing flags on attachment: 202526

Committed r151149: <http://trac.webkit.org/changeset/151149>
Comment 48 WebKit Commit Bot 2013-06-03 20:20:38 PDT
All reviewed patches have been landed.  Closing bug.