Bug 86841

Summary: [EFL] EFL's DRT should print the number of MessagePorts for new each new intent
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit EFLAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, gbillock, gyuyoung.kim, haraken, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 87245    
Bug Blocks: 86868, 87118    
Attachments:
Description Flags
Patch
none
Patch
abarth: review+
Patch for landing none

Chris Dumez
Reported 2012-05-18 04:32:03 PDT
EFL's Ewk_Intent does not expose WebCore::Intent::messagePorts(). As a consequence, the following test case cannot be unskipped: webintents/web-intents-invoke-port.html
Attachments
Patch (7.92 KB, patch)
2012-05-29 23:27 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (8.09 KB, patch)
2012-05-30 09:23 PDT, Chris Dumez 		abarth: review+
DetailsFormatted DiffDiff
Patch for landing (8.37 KB, patch)
2012-05-30 14:03 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2012-05-29 23:27:04 PDT
Created attachment 144717 [details] Patch
Greg Billock
Comment 2 2012-05-30 08:05:33 PDT
Comment on attachment 144717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144717&action=review > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:673 > + const WebCore::MessagePortChannelArray* messagePorts = ewk_intent_message_ports_get(intent); Would it make sense to inline this in DumpRenderTreeChrome.cpp?
Chris Dumez
Comment 3 2012-05-30 08:36:44 PDT
(In reply to comment #2) > (From update of attachment 144717 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144717&action=review > > > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:673 > > + const WebCore::MessagePortChannelArray* messagePorts = ewk_intent_message_ports_get(intent); > > Would it make sense to inline this in DumpRenderTreeChrome.cpp? I was trying to avoid exposing a WebCore type (MessagePortChannelArray) to DumpRenderTree but you're right: It seems a few methods in DumpRenderTreeSupportEfl expose WebCore types and it is more extensible this way. I'll fix the patch, thanks.
Chris Dumez
Comment 4 2012-05-30 09:23:23 PDT
Created attachment 144837 [details] Patch
Greg Billock
Comment 5 2012-05-30 09:56:41 PDT
Looks good to me
Adam Barth
Comment 6 2012-05-30 11:36:03 PDT
Comment on attachment 144837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144837&action=review > Source/WebKit/efl/ewk/ewk_intent_private.h:39 > +WebCore::MessagePortChannelArray* ewk_intent_message_ports_get(const Ewk_Intent* intent); Is it ok to return WebCore types in this API? I guess we're doing that a bunch in this header already.
Chris Dumez
Comment 7 2012-05-30 11:43:05 PDT
(In reply to comment #6) > (From update of attachment 144837 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144837&action=review > > > Source/WebKit/efl/ewk/ewk_intent_private.h:39 > > +WebCore::MessagePortChannelArray* ewk_intent_message_ports_get(const Ewk_Intent* intent); > > Is it ok to return WebCore types in this API? I guess we're doing that a bunch in this header already. It is not uncommon in private Ewk headers.
Raphael Kubo da Costa (:rakuco)
Comment 8 2012-05-30 12:57:09 PDT
As we were discussing on IRC, these days we tend to add a function to the EWKPrivate namespace to extract a WebCore type wrapped inside an ewk type (we have EWKPrivate::corePage() and EWKPrivate::corePageClient(), for example). While Chris' approach isn't wrong, since we started writing our DumpRenderTree implementation different reviewers have suggested that cleaner approach I mentioned above, so we're slowly moving away from these small extractor functions to leaving it up to the caller code to do whatever is needed with a core type.
Chris Dumez
Comment 9 2012-05-30 13:04:07 PDT
Comment on attachment 144837 [details] Patch Clearing cq flag until I update the patch based on rakuco's input.
Chris Dumez
Comment 10 2012-05-30 14:03:04 PDT
Created attachment 144919 [details] Patch for landing Take Rakuco's feedback into consideration and rebase on master so that the patch applies.
Raphael Kubo da Costa (:rakuco)
Comment 11 2012-05-30 14:12:35 PDT
Comment on attachment 144919 [details] Patch for landing Looks great, thanks.
Raphael Kubo da Costa (:rakuco)
Comment 12 2012-05-30 14:45:27 PDT
Committed r118977: <http://trac.webkit.org/changeset/118977>
Note You need to log in before you can comment on or make changes to this bug.