Summary: | [EFL] Add "return" statement corresponding to abnormal condition on _ewk_frame_smart_add. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | KwangHyuk <hyuki.kim> | ||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | antognolli+webkit, aroben, gyuyoung.kim, leandro, lucas.de.marchi, rakuco, ryuan.choi, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | Linux | ||||||||
Attachments: |
|
Description
KwangHyuk
2011-07-29 22:50:05 PDT
Created attachment 102427 [details]
Patch
Comment on attachment 102427 [details]
Patch
LGTM
Please adjust the smart_add in ewk_view too. > Please adjust the smart_add in ewk_view too.
I could find the patch for ewk_view in our local repository that other engineer created.
As a result, It will be created as another patch, and of course, I will ask him to create new bug for ewk_view.
> Please adjust the smart_add in ewk_view too. ewk_view's patch was created as new bug. :-) Refer the https://bugs.webkit.org/show_bug.cgi?id=65853 Hi Kubo and Leandro, Shall I get your review opinion for this patch ? :-) I'm closing this bug as duplicate, since the patch in 65853 already fixes this. *** This bug has been marked as a duplicate of bug 65853 *** > I'm closing this bug as duplicate, since the patch in 65853 already fixes this.
Hi, Leandro,
But, there was no ewk_frame related change on 65853.
So, This is not duplicated.
I have pulled change set from 65853.
* ewk/ewk_view.cpp:
(_ewk_view_smart_add):
(_ewk_view_smart_resize):
(_ewk_view_smart_move):
(_ewk_view_smart_show):
(_ewk_view_smart_hide):
* ewk/ewk_view_single.c:
(_ewk_view_single_smart_add):
(_ewk_view_single_smart_resize):
* ewk/ewk_view_tiled.c:
(_ewk_view_tiled_smart_add):
Reopening bug, as I mistook ewk_frame with ewk_view. Comment on attachment 102427 [details]
Patch
Informal r+.
Comment on attachment 102427 [details]
Patch
LGTM.
Comment on attachment 102427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102427&action=review > Source/WebKit/efl/ChangeLog:8 > + In order to avoid abnormal reference of variable "sd", > + it must be returned if it can't be allocated because variable "sd" can be referenced > + regardless of success of allocation. I think this could be stated a little more clearly. Something like: Bail out when we fail to allocate an Ewk_Frame_Smart_Data object rather than continuing on as if the allocation had succeeded. > Source/WebKit/efl/ewk/ewk_frame.cpp:184 > sd = (Ewk_Frame_Smart_Data*)calloc(1, sizeof(Ewk_Frame_Smart_Data)); > - if (!sd) > + if (!sd) { > CRITICAL("could not allocate Ewk_Frame_Smart_Data"); > - else > - evas_object_smart_data_set(o, sd); > + return; > + } Is this really a condition we expect to occur? Will we really continue to function correctly even if we return early from this function? We don't check for allocation failures in most of WebKit/WebCore, so I assume that if we're really in an out-of-memory situation we'll just crash a little later. > I think this could be stated a little more clearly. Something like: Bail out when we fail to allocate an Ewk_Frame_Smart_Data object rather than continuing on as if the allocation had succeeded. Great sentence and I will update it according to your idea. :-) > Is this really a condition we expect to occur? Will we really continue to function correctly even if we return early from this function? We don't check for allocation failures in most of WebKit/WebCore, so I assume that if we're really in an out-of-memory situation we'll just crash a little later. I agree with you that crash may be the better way to detect the error condition faster. But, I believe that we will be able to notice the critical error message as soon as it will occur and this patch will give more time which browser can be alive little longer. :-) Created attachment 104504 [details]
Patch.
Comment on attachment 104504 [details] Patch. Clearing flags on attachment: 104504 Committed r93410: <http://trac.webkit.org/changeset/93410> All reviewed patches have been landed. Closing bug. |