Summary: | [EFL] EFL's DRT should print the number of MessagePorts for new each new intent | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | WebKit EFL | Assignee: | 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
Chris Dumez
2012-05-18 04:32:03 PDT
Created attachment 144717 [details]
Patch
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? (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. Created attachment 144837 [details]
Patch
Looks good to me 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. (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. 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. Comment on attachment 144837 [details]
Patch
Clearing cq flag until I update the patch based on rakuco's input.
Created attachment 144919 [details]
Patch for landing
Take Rakuco's feedback into consideration and rebase on master so that the patch applies.
Comment on attachment 144919 [details]
Patch for landing
Looks great, thanks.
Committed r118977: <http://trac.webkit.org/changeset/118977> |