Bug 86841 - [EFL] EFL's DRT should print the number of MessagePorts for new each new intent
Summary: [EFL] EFL's DRT should print the number of MessagePorts for new each new intent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 87245
Blocks: 86868 87118
  Show dependency treegraph
 
Reported: 2012-05-18 04:32 PDT by Chris Dumez
Modified: 2012-05-30 14:46 PDT (History)
7 users (show)

See Also:


Attachments
Patch (7.92 KB, patch)
2012-05-29 23:27 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (8.09 KB, patch)
2012-05-30 09:23 PDT, Chris Dumez
abarth: review+
Details | Formatted Diff | Diff
Patch for landing (8.37 KB, patch)
2012-05-30 14:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 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
Comment 1 Chris Dumez 2012-05-29 23:27:04 PDT
Created attachment 144717 [details]
Patch
Comment 2 Greg Billock 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?
Comment 3 Chris Dumez 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.
Comment 4 Chris Dumez 2012-05-30 09:23:23 PDT
Created attachment 144837 [details]
Patch
Comment 5 Greg Billock 2012-05-30 09:56:41 PDT
Looks good to me
Comment 6 Adam Barth 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.
Comment 7 Chris Dumez 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.
Comment 8 Raphael Kubo da Costa (:rakuco) 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.
Comment 9 Chris Dumez 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.
Comment 10 Chris Dumez 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.
Comment 11 Raphael Kubo da Costa (:rakuco) 2012-05-30 14:12:35 PDT
Comment on attachment 144919 [details]
Patch for landing

Looks great, thanks.
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-05-30 14:45:27 PDT
Committed r118977: <http://trac.webkit.org/changeset/118977>