Bug 95923

Summary:

[EFL] Implement GCActivityCallback

Product:

WebKit

Reporter:

Hojong Han <hojong.han>

Component:

JavaScriptCore

Assignee:

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:

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

Hojong Han

Reported 2012-09-05 18:51:17 PDT

to add a GCActivityCallbackEfl.cpp, and implement GCActivityCallback with platform timer

Attachments
Patch (5.92 KB, patch) 2012-09-05 18:59 PDT, Hojong Han	no flags	Details Formatted Diff Diff
Patch (5.88 KB, patch) 2012-09-06 19:37 PDT, Hojong Han	no flags	Details Formatted Diff Diff
Patch (6.04 KB, patch) 2012-12-02 20:37 PST, Hojong Han	no flags	Details Formatted Diff Diff
Patch (5.42 KB, patch) 2013-05-21 22:08 PDT, Hojong Han	no flags	Details Formatted Diff Diff
Patch (5.14 KB, patch) 2013-05-21 23:03 PDT, Hojong Han	no flags	Details Formatted Diff Diff
Patch (5.46 KB, patch) 2013-05-22 00:20 PDT, Hojong Han	no flags	Details Formatted Diff Diff
Patch (6.54 KB, patch) 2013-05-22 00:58 PDT, Hojong Han	no flags	Details Formatted Diff Diff
Patch (7.53 KB, patch) 2013-05-22 05:04 PDT, Hojong Han	no flags	Details Formatted Diff Diff
Patch (7.53 KB, patch) 2013-05-22 05:13 PDT, Hojong Han	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Hojong Han

Comment 1 2012-09-05 18:59:37 PDT

Created attachment 162393 [details] Patch

Raphael Kubo da Costa (:rakuco)

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

Gyuyoung Kim

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

Hojong Han

Comment 4 2012-09-06 19:37:17 PDT

Created attachment 162655 [details] Patch

Hojong Han

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

Simon Hausmann

Comment 6 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? :)

Kenneth Rohde Christiansen

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

Gyuyoung Kim

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

Hojong Han

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

Gyuyoung Kim

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

Hojong Han

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

Gyuyoung Kim

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

Chris Dumez

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

Hojong Han

Comment 14 2012-12-02 20:37:01 PST

Created attachment 177172 [details] Patch

Hojong Han

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

Gyuyoung Kim

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

Simon Hausmann

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

Allan Sandfeld Jensen

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

Gyuyoung Kim

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

Hojong Han

Comment 20 2013-05-21 22:08:39 PDT

Created attachment 202494 [details] Patch

Gyuyoung Kim

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

Hojong Han

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

Hojong Han

Comment 23 2013-05-21 23:03:05 PDT

Created attachment 202500 [details] Patch

Chris Dumez

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

Hojong Han

Comment 25 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().

Chris Dumez

Comment 26 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 :)

Gyuyoung Kim

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

Hojong Han

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

Chris Dumez

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

Hojong Han

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

Hojong Han

Comment 31 2013-05-22 00:20:39 PDT

Created attachment 202503 [details] Patch

Gyuyoung Kim

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

Chris Dumez

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

Chris Dumez

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

Hojong Han

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

Hojong Han

Comment 36 2013-05-22 00:58:29 PDT

Created attachment 202505 [details] Patch

Chris Dumez

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

Gyuyoung Kim

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

Hojong Han

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

Hojong Han

Comment 40 2013-05-22 05:04:53 PDT

Created attachment 202524 [details] Patch GCActivityCallback works only in main thread

WebKit Commit Bot

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

Hojong Han

Comment 42 2013-05-22 05:13:56 PDT

Created attachment 202526 [details] Patch style error fixed

Gyuyoung Kim

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

Geoffrey Garen

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

Hojong Han

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

Geoffrey Garen

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

WebKit Commit Bot

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

WebKit Commit Bot

Comment 48 2013-06-03 20:20:38 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.