Bug 86841 – [EFL] EFL's DRT should print the number of MessagePorts for new each new intent

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

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

[Chris Dumez]

Created attachment 144717 [details]
Patch

[Greg Billock]

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]

(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]

Created attachment 144837 [details]
Patch

[Greg Billock]

Looks good to me

[Adam Barth]

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]

(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)]

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 on attachment 144837 [details]
Patch

Clearing cq flag until I update the patch based on rakuco's input.

[Chris Dumez]

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 on attachment 144919 [details]
Patch for landing

Looks great, thanks.

[Raphael Kubo da Costa (:rakuco)]

Committed r118977: <http://trac.webkit.org/changeset/118977>