96610 2012-09-13 00:06:14 -0700 [EFL][WK2] Regression (r128163) 2012-09-17 22:17:49 -0700 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) Other Linux RESOLVED FIXED P2 Normal --- 0 heejin.r.chung webkit-unassigned cdumez gyuyoung.kim lucas.de.marchi rakuco webkit.review.bot oldest_to_newest 719126 0 heejin.r.chung 2012-09-13 00:06:14 -0700 While removing compile warnings r128163 changed the logic of code for entering accelerated compositing mode, resulting in never being able to enter it. 719146 1 163800 heejin.r.chung 2012-09-13 00:27:37 -0700 Created attachment 163800 Patch 719178 2 163800 heejin.r.chung 2012-09-13 00:54:34 -0700 Comment on attachment 163800 Patch Fixes regression introduced by r128163 while removing compile warnings as intended in r128163. 721765 3 163800 gyuyoung.kim 2012-09-17 02:17:55 -0700 Comment on attachment 163800 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163800&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:501 > + EINA_SAFETY_ON_FALSE_RETURN_VAL(!priv->evasGl, false); Is below code is more readable instead of above one? EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); 721788 4 heejin.r.chung 2012-09-17 03:31:07 -0700 (In reply to comment #3) > (From update of attachment 163800 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163800&action=review > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:501 > > + EINA_SAFETY_ON_FALSE_RETURN_VAL(!priv->evasGl, false); > Is below code is more readable instead of above one? > EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); Using EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); doesn't compile. If we want to use EINA_SAFETY_ON_TRUE_RETURN_VAL(), we have to put !! in front of priv->evasGl. Which one should I use? EINA_SAFETY_ON_FALSE_RETURN_VAL(!priv->evasGl, false); or EINA_SAFETY_ON_TRUE_RETURN_VAL(!!priv->evasGl, false); 721796 5 gyuyoung.kim 2012-09-17 04:26:45 -0700 (In reply to comment #4) > Using EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); doesn't compile. > If we want to use EINA_SAFETY_ON_TRUE_RETURN_VAL(), we have to put !! in front of priv->evasGl. > Which one should I use? > EINA_SAFETY_ON_FALSE_RETURN_VAL(!priv->evasGl, false); or > EINA_SAFETY_ON_TRUE_RETURN_VAL(!!priv->evasGl, false); Something is strange. I succeed to compile with below code in my linux box. EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); Could you check this again ? 722413 6 heejin.r.chung 2012-09-17 18:49:01 -0700 (In reply to comment #5) > (In reply to comment #4) > > Using EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); doesn't compile. > > If we want to use EINA_SAFETY_ON_TRUE_RETURN_VAL(), we have to put !! in front of priv->evasGl. > > Which one should I use? > > EINA_SAFETY_ON_FALSE_RETURN_VAL(!priv->evasGl, false); or > > EINA_SAFETY_ON_TRUE_RETURN_VAL(!!priv->evasGl, false); > Something is strange. I succeed to compile with below code in my linux box. > EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); > Could you check this again ? You have to build with --tiled-backing-store option to include that code :) 722422 7 163800 gyuyoung.kim 2012-09-17 19:20:05 -0700 Comment on attachment 163800 Patch Frankly, EINA_SAFETY_ON_NULL_RETURN_VAL is correct I think. But, it was changed due to compilation warning. So, if you need to check if priv->evasGl is null, why don't you use *if* condition explicitly ? if (priv->evasGl) { EINA_LOG_DOM_WARN(_ewk_log_dom, "bla bla"); return false; } 722429 8 164479 heejin.r.chung 2012-09-17 19:36:54 -0700 Created attachment 164479 Patch 722430 9 heejin.r.chung 2012-09-17 19:37:17 -0700 (In reply to comment #7) > (From update of attachment 163800 [details]) > Frankly, EINA_SAFETY_ON_NULL_RETURN_VAL is correct I think. But, it was changed due to compilation warning. So, if you need to check if priv->evasGl is null, why don't you use *if* condition explicitly ? > if (priv->evasGl) { > EINA_LOG_DOM_WARN(_ewk_log_dom, "bla bla"); > return false; > } I agree that EINA_SAFETY_ON_NULL_RETURN_VAL is correct. Changed the code as you suggested. 722438 10 164479 webkit.review.bot 2012-09-17 20:03:49 -0700 Comment on attachment 164479 Patch Clearing flags on attachment: 164479 Committed r128844: <http://trac.webkit.org/changeset/128844> 722439 11 webkit.review.bot 2012-09-17 20:03:53 -0700 All reviewed patches have been landed. Closing bug. 722472 12 164479 cdumez 2012-09-17 21:48:26 -0700 Comment on attachment 164479 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164479&action=review > Source/WebKit2/ChangeLog:3 > + [EFL][WK2] Regression (r128163) Please add a title after Regression (r128163): > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:-501 > - EINA_SAFETY_ON_NULL_RETURN_VAL(priv->evasGl, false); EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); ? 722478 13 gyuyoung.kim 2012-09-17 21:57:49 -0700 (In reply to comment #12) > (From update of attachment 164479 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164479&action=review > > > Source/WebKit2/ChangeLog:3 > > + [EFL][WK2] Regression (r128163) > > Please add a title after Regression (r128163): > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:-501 > > - EINA_SAFETY_ON_NULL_RETURN_VAL(priv->evasGl, false); > > EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); ? As previous comments mentioned, EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false) makes a below build break with --tiled-backing-store option. /home/gyuyoung/webkit/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:501:5: error: invalid conversion from ‘Evas_GL* {aka _Evas_GL*}’ to ‘long int’ [-fpermissive] 722485 14 cdumez 2012-09-17 22:11:18 -0700 (In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 164479 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=164479&action=review > > > > > Source/WebKit2/ChangeLog:3 > > > + [EFL][WK2] Regression (r128163) > > > > Please add a title after Regression (r128163): > > > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:-501 > > > - EINA_SAFETY_ON_NULL_RETURN_VAL(priv->evasGl, false); > > > > EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); ? > > As previous comments mentioned, EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false) makes a below build break with --tiled-backing-store option. > > /home/gyuyoung/webkit/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:501:5: error: invalid conversion from ‘Evas_GL* {aka _Evas_GL*}’ to ‘long int’ [-fpermissive] Maybe EINA_SAFETY_ON_TRUE_RETURN_VAL(!!priv->evasGl, false); would have worked then? 722489 15 gyuyoung.kim 2012-09-17 22:17:49 -0700 (In reply to comment #14) /home/gyuyoung/webkit/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:501:5: error: invalid conversion from ‘Evas_GL* {aka _Evas_GL*}’ to ‘long int’ [-fpermissive] > > Maybe EINA_SAFETY_ON_TRUE_RETURN_VAL(!!priv->evasGl, false); would have worked then? As I said comment #7, I think EINA_SAFETY_ON_NULL_RETURN_VAL is more proper because this is for checking if priv->evasGL is not null. Generally, it looks EINA_SAFETY_ON_TRUE | FALSE_RETURN_VAL() are used for checking specific condition rather than checking if null. So, I thought that explicit *if* condition is better than EINA_SAFETY_ON_TRUE | FALSE_RETURN_VAL for null checking. 163800 2012-09-13 00:27:37 -0700 2012-09-17 19:36:48 -0700 Patch regression-r128163.patch text/plain 1570 heejin.r.chung SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDEyODQxMikKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDEyLTA5LTEyICBSZWdpbmEg Q2h1bmcgIDxoZWVqaW4uci5jaHVuZ0BzYW1zdW5nLmNvbT4KKworICAgICAgICBbRUZMXVtXSzJd IFJlZ3Jlc3Npb24gKHIxMjgxNjMpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD05NjYxMAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIFdoaWxlIHJlbW92aW5nIGNvbXBpbGUgd2FybmluZ3MgcjEyODE2MyBjaGFu Z2VkIHRoZSBsb2dpYyBvZiBjb2RlIGZvciBlbnRlcmluZyAKKyAgICAgICAgYWNjZWxlcmF0ZWQg Y29tcG9zaXRpbmcgbW9kZSwgcmVzdWx0aW5nIGluIG5ldmVyIGJlaW5nIGFibGUgdG8gZW50ZXIg aXQuCisgICAgICAgIENoYW5nZWQgYmFjayB0byB0aGUgY29ycmVjdCBjb2RlIGFuZCBmaXhlZCB0 aGUgY29tcGlsZSB3YXJuaW5nIGJ5IHVzaW5nIGFuCisgICAgICAgIGFwcHJvcHJpYXRlIEVJTkEg bWFjcm8uCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQVBJL2VmbC9ld2tfdmlldy5jcHA6CisgICAg ICAgIChld2tfdmlld19hY2NlbGVyYXRlZF9jb21wb3NpdGluZ19tb2RlX2VudGVyKTogQ2hhbmdl ZCBFSU5BX1NBRkVUWV9PTl9OVUxMX1JFVFVSTl9WQUwgdG8gRUlOQV9TQUZFVFlfT05fRkFMU0Vf UkVUVVJOX1ZBTC4KKwogMjAxMi0wOS0xMiAgU2hlcmlmZiBCb3QgIDx3ZWJraXQucmV2aWV3LmJv dEBnbWFpbC5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgcm9sbGluZyBvdXQgcjEyNzg3Ni4K SW5kZXg6IFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL2V3a192aWV3LmNwcAo9PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2VmbC9ld2tfdmlldy5jcHAJ KHJldmlzaW9uIDEyODQxMikKKysrIFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL2V3 a192aWV3LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNDk4LDcgKzQ5OCw3IEBACiAgICAgRVdLX1ZJ RVdfU0RfR0VUX09SX1JFVFVSTihld2tWaWV3LCBzbWFydERhdGEsIGZhbHNlKTsKICAgICBFV0tf VklFV19QUklWX0dFVF9PUl9SRVRVUk4oc21hcnREYXRhLCBwcml2LCBmYWxzZSk7CiAKLSAgICBF SU5BX1NBRkVUWV9PTl9OVUxMX1JFVFVSTl9WQUwocHJpdi0+ZXZhc0dsLCBmYWxzZSk7CisgICAg RUlOQV9TQUZFVFlfT05fRkFMU0VfUkVUVVJOX1ZBTCghcHJpdi0+ZXZhc0dsLCBmYWxzZSk7CiAK ICAgICBFdmFzKiBldmFzID0gZXZhc19vYmplY3RfZXZhc19nZXQoZXdrVmlldyk7CiAgICAgcHJp di0+ZXZhc0dsID0gZXZhc19nbF9uZXcoZXZhcyk7Cg== 164479 2012-09-17 19:36:54 -0700 2012-09-17 21:48:26 -0700 Patch bug-96610-20120918113654.patch text/plain 1749 heejin.r.chung SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDEyODczNCkKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDEyLTA5LTE3ICBSZWdpbmEg Q2h1bmcgIDxoZWVqaW4uci5jaHVuZ0BzYW1zdW5nLmNvbT4KKworICAgICAgICBbRUZMXVtXSzJd IFJlZ3Jlc3Npb24gKHIxMjgxNjMpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD05NjYxMAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIFdoaWxlIHJlbW92aW5nIGNvbXBpbGUgd2FybmluZ3MgcjEyODE2MyBjaGFu Z2VkIHRoZSBsb2dpYyBvZiBjb2RlIGZvciBlbnRlcmluZyAKKyAgICAgICAgYWNjZWxlcmF0ZWQg Y29tcG9zaXRpbmcgbW9kZSwgcmVzdWx0aW5nIGluIG5ldmVyIGJlaW5nIGFibGUgdG8gZW50ZXIg aXQuCisgICAgICAgIENoYW5nZWQgYmFjayB0byB0aGUgY29ycmVjdCBjb2RlIGFuZCBmaXhlZCB0 aGUgY29tcGlsZSB3YXJuaW5nIGJ5IHVzaW5nIGFuCisgICAgICAgIGFwcHJvcHJpYXRlIEVJTkEg bWFjcm8uCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQVBJL2VmbC9ld2tfdmlldy5jcHA6CisgICAg ICAgIChld2tfdmlld19hY2NlbGVyYXRlZF9jb21wb3NpdGluZ19tb2RlX2VudGVyKTogQ2hhbmdl ZCBFSU5BX1NBRkVUWV9PTl9OVUxMX1JFVFVSTl9WQUwgdG8gKmlmKiBjb25kaXRpb24gc3RhdGVt ZW50LgorCiAyMDEyLTA5LTE1ICBWaXZlayBHYWxhdGFnZSAgPHZpdmVrZ2FsYXRhZ2VAZ21haWwu Y29tPgogCiAgICAgICAgIFdpbkNhaXJvIGJ1aWxkIGZpeCBkdWUgdG8gbWlzc2luZyBzeW1ib2xz IGZvciBzZXRJbWFnZXNFbmFibGVkIGFuZCBzZXRTdG9yYWdlQmxvY2tpbmdQb2xpY3kKSW5kZXg6 IFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL2V3a192aWV3LmNwcAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBTb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2VmbC9ld2tfdmlldy5jcHAJKHJldmlz aW9uIDEyODczNCkKKysrIFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL2V3a192aWV3 LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNDk4LDcgKzQ5OCwxMCBAQCBib29sIGV3a192aWV3X2Fj Y2VsZXJhdGVkX2NvbXBvc2l0aW5nX21vCiAgICAgRVdLX1ZJRVdfU0RfR0VUX09SX1JFVFVSTihl d2tWaWV3LCBzbWFydERhdGEsIGZhbHNlKTsKICAgICBFV0tfVklFV19QUklWX0dFVF9PUl9SRVRV Uk4oc21hcnREYXRhLCBwcml2LCBmYWxzZSk7CiAKLSAgICBFSU5BX1NBRkVUWV9PTl9OVUxMX1JF VFVSTl9WQUwocHJpdi0+ZXZhc0dsLCBmYWxzZSk7CisgICAgaWYgKHByaXYtPmV2YXNHbCkgewor ICAgICAgICBFSU5BX0xPR19ET01fV0FSTihfZXdrX2xvZ19kb20sICJBY2NlbGVyYXRlZCBjb21w b3NpdGluZyBtb2RlIGFscmVhZHkgZW50ZXJlZC4iKTsKKyAgICAgICAgcmV0dXJuIGZhbHNlOwor ICAgIH0KIAogICAgIEV2YXMqIGV2YXMgPSBldmFzX29iamVjdF9ldmFzX2dldChld2tWaWV3KTsK ICAgICBwcml2LT5ldmFzR2wgPSBldmFzX2dsX25ldyhldmFzKTsK