Bug 120360

Created attachment 209767 [details] Patch

Comment on attachment 209767 [details] Patch Attachment 209767 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1579316 New failing tests: fast/workers/termination-with-port-messages.html

Created attachment 209774 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

I didn't have time to look at the whole patch, but ewk_frame_coreFrame_set looks like a weird function name. Why not add setCoreFrame to EWKPrivate (just like we already have coreFrame() there) and call it instead?

Comment on attachment 209767 [details] Patch Overall, I found the new location of the calls more confusing than before. Can't we get our behavior more inline with GTK+'s? It basically has most of the code present in ewk_view_frame_create() in FrameLoaderClientGtk::createFrame() itself.

Created attachment 209823 [details] Follow kubo's comment

(In reply to comment #4) > I didn't have time to look at the whole patch, but ewk_frame_coreFrame_set looks like a weird function name. Why not add setCoreFrame to EWKPrivate (just like we already have coreFrame() there) and call it instead? Fixed. (In reply to comment #5) > (From update of attachment 209767 [details]) > Overall, I found the new location of the calls more confusing than before. Can't we get our behavior more inline with GTK+'s? It basically has most of the code present in ewk_view_frame_create() in FrameLoaderClientGtk::createFrame() itself. Thank you for good suggestion. I thought that ewk_view_frame_create is bad way. I moved most of the code to FrameLoaderClientEfl::createFrame().

Comment on attachment 209823 [details] Follow kubo's comment Attachment 209823 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1624087 New failing tests: fast/workers/termination-with-port-messages.html

Created attachment 209828 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.4

Comment on attachment 209823 [details] Follow kubo's comment View in context: https://bugs.webkit.org/attachment.cgi?id=209823&action=review > Source/WebKit/efl/ChangeLog:3 > + [EFL] Crash after r154616 It looks this patch has meaningful change current title is unable to represent. It would move this title to description. Then, please change title more meaningful thing. > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:380 > + if (!coreSubFrame->page()) { EWKPrivate::coreFrame may return 0. Don't you need to check if coreSubFrame is null ? WebCore::Frame* coreFrame(const Evas_Object* ewkFrame) { EWK_FRAME_SD_GET_OR_RETURN(ewkFrame, smartData, 0); return smartData->frame; } > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:381 > + evas_object_del(subFrame); It would be nice to add a message. > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:391 > + evas_object_del(subFrame); ditto.

Created attachment 209833 [details] Patch

(In reply to comment #10) > (From update of attachment 209823 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209823&action=review > > > Source/WebKit/efl/ChangeLog:3 > > + [EFL] Crash after r154616 > > It looks this patch has meaningful change current title is unable to represent. It would move this title to description. Then, please change title more meaningful thing. > OK, I changed like other ports did. > > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:380 > > + if (!coreSubFrame->page()) { > > EWKPrivate::coreFrame may return 0. Don't you need to check if coreSubFrame is null ? > > > WebCore::Frame* coreFrame(const Evas_Object* ewkFrame) > { > EWK_FRAME_SD_GET_OR_RETURN(ewkFrame, smartData, 0); > return smartData->frame; > } > Added ASSERT. I think that it can't be NULL logically. > > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:381 > > + evas_object_del(subFrame); > > It would be nice to add a message. > > > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:391 > > + evas_object_del(subFrame); > > ditto. I think that this is normal case. So we don't need to print error message.

Comment on attachment 209833 [details] Patch Darin set r+ instead of me. It looks there is no critical problem now. I land this patch in order to fix EFL WK1 buildbot as soon as possible.

Comment on attachment 209833 [details] Patch Clearing flags on attachment: 209833 Committed r154798: <http://trac.webkit.org/changeset/154798>

All reviewed patches have been landed. Closing bug.