Bug 44500 – [EFL] API for Dump Render Tree application

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Krzysztof Czech]

New API that are used by Dump Render Tree application

[Krzysztof Czech]

Created attachment 65240 [details]
Enhancements for WebKit-EFL port, that are used by Dump Render Tree application

[Gyuyoung Kim]

Hello Krzysztof,

I think you need to fill doxyzen for new methods.

[Krzysztof Czech]

Created attachment 65258 [details]
Enhancements for WebKit-EFL port, that are used by Dump Render Tree application

[Lucas De Marchi]

Before reviewing, just a side note: when you submit patches, clear the flags on previous patch and mark it as obsolete (if it's meant to replace the previous one).

If you use WebKitTools/Scripts/webkit-patch to send patches, this will be done automatically for you and your life will be easier ;-)

[Antonio Gomes]

First, I would like to hear from the other EFL guys how the additions like, API-wise.

[Lucas De Marchi]

(In reply to comment #5)
> First, I would like to hear from the other EFL guys how the additions like, API-wise.

I like it, but there are some problems that I'll address in my review (as for example the function names)

[Lucas De Marchi]

Comment on attachment 65258 [details]
Enhancements for WebKit-EFL port, that are used by Dump Render Tree application

> diff --git a/WebKit/efl/ChangeLog b/WebKit/efl/ChangeLog
> index de65885..8ea8baa 100644
> --- a/WebKit/efl/ChangeLog
> +++ b/WebKit/efl/ChangeLog
> @@ -1,3 +1,36 @@
> [phone]  Krzysztof Czech  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [EFL] WebKit API for Dump Render Tree application
> +        https://bugs.webkit.org/show_bug.cgi?id=44500
> +
> +        Enhancements for WebKit-EFL port, that are used
> +        by Dump Render Tree application 
> +
> +        * ewk/ewk_frame.cpp:
> +        (ewk_frame_get_inner_text):
> +        (ewk_frame_get_dump_render_tree):
> +        (ewk_frame_get_children):
> +        (ewk_frame_page_number_for_element_by_id):
> +        (ewk_frame_page_number_of_pages):
> +        (ewk_frame_counter_value_for_element_by_id):
> +        (ewk_frame_web_layout):
> +        (ewk_frame_pause_animation):
> +        (ewk_frame_pause_transition):
> +        (ewk_frame_pause_svg_animation):
> +        (ewk_frame_number_of_active_animations):
> +        (ewk_frame_get_response_mime_type):
> +        (ewk_gc_collect_javascript_objects):
> +        (ewk_gc_collect_javascript_objects_on_alternate_thread):
> +        (ewk_gc_count_javascript_objects):
> +        (ewk_worker_thread_count):

We use the verb as the last word in function names (with a few exceptions). Look in other functions in this same file; for example:
It's ewk_frame_inner_text_get, not ewk_frame_inner_text_get


> +        * ewk/ewk_frame.h:
> +        * ewk/ewk_view.cpp:
> +        (ewk_view_load_started):
> +        (ewk_view_window_object_cleared):
These are ok.


> diff --git a/WebKit/efl/ewk/ewk_frame.cpp b/WebKit/efl/ewk/ewk_frame.cpp
> index 296c261..d86692c 100644
> --- a/WebKit/efl/ewk/ewk_frame.cpp
> +++ b/WebKit/efl/ewk/ewk_frame.cpp

> @@ -1950,3 +1959,272 @@ WTF::PassRefPtr<WebCore::Widget> ewk_frame_plugin_create(Evas_Object* o, const W
>  {
>      return 0;
>  }
> +/**
> + * Dumps webpage's layout as a plain text
> + *
> + * @param o Frame
> + *
> + * @return returns textual representation of dumped page
> +*/
> +char* ewk_frame_get_inner_text(Evas_Object* o)
const Evas_Object* o

> +{
> +    EWK_FRAME_SD_GET_OR_RETURN(o, sd, 0);
> +
> +    WebCore::FrameView* frameView = sd->frame->view();
> +    if (frameView && frameView->layoutPending())
> +        frameView->layout();
> +
> +    WebCore::Element* docElement = sd->frame->document()->documentElement();
> +    WTF::String innerText = docElement->innerText();
> +
> +    return strdup(innerText.utf8().data());
When you return a struct that you dynamically allocate or, as in this case, strdup, you have to document it in doxygen. For example, in this file ewk_frame_selection_get() document it as:
@return newly allocated string or @c NULL if nothing is selected or failure.

Otherwise user would have to check the implementation in order to know it must free the returned var.

> +}
> +/**
> + * Dumps webpage's layout as a tree representation
> + *
> + * @param o Frame
> + *
> + * @return returns textual representation of dumped page
> + */
> +char* ewk_frame_get_dump_render_tree(Evas_Object* o)
cont

> +{
> +    EWK_FRAME_SD_GET_OR_RETURN(o, sd, 0);
> +
> +    WebCore::FrameView* frameView = sd->frame->view();
> +    if (frameView && frameView->layoutPending())
> +        frameView->layout();
> +
> +    WTF::String renderText = WebCore::externalRepresentation(sd->frame);
> +
> +    return strdup(renderText.utf8().data());
same as above.

> +}
> +/**
> + * Returns child frames if exists
> + *
> + * @param o Frame
> + *
> + * @return returns list of child frames, or @c NULL empty list
document the type (Evas_Object) and that the user must free the list nodes (and *not* data) after use.


> +/**
> + * Returns number of page element
> + *
> + * @param o Frame
> + * @param element_id element id
> + * @param pageWidth page width
> + * @param pageHeight page height
> + *
> + * @return number of page element or @c 0
> + */
> +int ewk_frame_page_number_for_element_by_id(Evas_Object* o, char* element_id, float pageWidth, float pageHeight)
const Evas_Object*, const char*



> +/**
> + * Returns number of pages
> + *
> + * @param o Frame
> + * @param pageWidth page width
> + * @param pageHeight page height
> + *
> + * @return returns number of pages
> + */
> +int ewk_frame_page_number_of_pages(Evas_Object* o, float pageWidth, float pageHeight)
const..

> +/**
> + * Returns counter value for element
> + *
> + * @param o Frame
> + * @param element_id element id
> + *
> + * @return returns textual value or @c NULL
> + */
> +char* ewk_frame_counter_value_for_element_by_id(Evas_Object* o, char* element_id)

consts + document strdup.


> +/**
> + * Returns true if frame layout
> + * 
> + * @param o Frame
> + * 
> + * @return return @c TRUE if frame's layout, or @c FALSE if something went wrong
> + */
> +Eina_Bool ewk_frame_web_layout(Evas_Object* o)

I didn't get what this function does. Are you sure you meant "frame *is* layout" ?

> +{
> +    EWK_FRAME_SD_GET_OR_RETURN(o, sd, 0);
> +
> +    WebCore::FrameView* frameView = sd->frame->view();
> +
> +    if (!frameView)
> +        return false;

You are just returning false if frame has a view and true otherwise.


> +/**
> + * Set pause on frame's animation
> + *
> + * @param name name
> + * @param element_id element id
> + * @double time time
> + *
> + * @return returns @c TRUE pause is set, @c FALSE pause is not set
> + */
> +Eina_Bool ewk_frame_pause_animation(Evas_Object* o, const char* name, const char* elementId, double time)
> +{
> +    EWK_FRAME_SD_GET_OR_RETURN(o, sd, 0);
> +
> +    WebCore::Element* element = sd->frame->document()->getElementById(WTF::AtomicString(elementId));
> +    if (!element || !element->renderer())
> +        return false;
> +
> +    return sd->frame->animation()->pauseAnimationAtTime(element->renderer(), WTF::AtomicString(name), time);
> +}

Missing space between functions here.

> +/**
> + * Pause transition
> + *
> + * @param o Frame
> + * @param name
> + * @param elementId
> + * @param time
> + *
> + * @return returns @c TRUE pause is set, or @c FALSE pause is not set
Also in other doxygens: we don't need "@return returns", use just 1


> + */
> +Eina_Bool ewk_frame_pause_transition(Evas_Object* o, const char* name, const char* elementId, double time)
> +{
> +    EWK_FRAME_SD_GET_OR_RETURN(o, sd, 0);
> +
> +    WebCore::Element* element = sd->frame->document()->getElementById(WTF::AtomicString(elementId));
> +    if (!element || !element->renderer())
> +        return false;
> +
> +    return sd->frame->animation()->pauseTransitionAtTime(element->renderer(), WTF::AtomicString(name), time);
> +}
> +/**
> + * Pause svg animation
> + * 
> + * @param o Frame
> + * @param animationId
> + * @param elementId
> + * @param time
> + *
> + * @return returns @c TRUE pause is set or @c FALSE pause is not set
> + */
> +Eina_Bool ewk_frame_pause_svg_animation(Evas_Object* o, const char* animationId, const char* elementId, double time)
> +{
> +    EWK_FRAME_SD_GET_OR_RETURN(o, sd, 0);
> +
> +    WebCore::Document* document = sd->frame->document();
> +    if (!document && !document->svgExtensions())
> +        return false;
> +
> +    WebCore::Element* element = document->getElementById(WTF::AtomicString(animationId));
> +    if (!element && !WebCore::SVGSMILElement::isSMILElement(element))
> +        return false;
> +
> +    return document->accessSVGExtensions()->sampleAnimationAtTime(elementId, static_cast<WebCore::SVGSMILElement*>(element), time);
> +}
> +/**
> + * Returns number of active animations
> + * 
> + * @param o Frame
> + *
> + * @return returns number of active animations, or @c 0 if something went wrong
> + */
> +unsigned ewk_frame_number_of_active_animations(Evas_Object* o)
> +{
> +    EWK_FRAME_SD_GET_OR_RETURN(o, sd, 0);
> +
> +    WebCore::AnimationController* aController = sd->frame->animation();
> +    if (!aController)
> +        return false;
> +
> +    return aController->numberOfActiveAnimations();
> +}
> +/**
> + * Gets response mi type
typo here.

> + *
> + * @param o Frame
> + *
> + * @return returns mime type or @c NULL if something went wrong
> + */
> +char* ewk_frame_get_response_mime_type(Evas_Object* o)
const + doc about strdup

[...]
> +        return strdup(mimeType.utf8().data());
> +
> +    return 0;
> +}
missing blank line

> +/**
> + * Collec javascript objects
> + */
> +void ewk_gc_collect_javascript_objects()
> +{
> +    WebCore::gcController().garbageCollectNow();
> +}
> +/**
> + * Collect javascript object on alternate thread
> + *
> + * @param waitUntilDone
> + */
> +void ewk_gc_collect_javascript_objects_on_alternate_thread(Eina_Bool waitUntilDone)
https://bugs.webkit.org/attachment.cgi?id=65258





> diff --git a/WebKit/efl/ewk/ewk_view.cpp b/WebKit/efl/ewk/ewk_view.cpp
> index 7a98bac..c55c2d1 100644
> --- a/WebKit/efl/ewk/ewk_view.cpp
> +++ b/WebKit/efl/ewk/ewk_view.cpp
> @@ -3139,6 +3139,18 @@ void ewk_view_load_started(Evas_Object* o)
>  {
>      DBG("o=%p", o);
>      evas_object_smart_callback_call(o, "load,started", 0);
> +
> +    ewk_view_window_object_cleared(o);
Why do you need this here? I think this is part of another patch, not this one.

> +}
> +
> +/**
> + * @param o View
> + * Used by layout tests to initialize some JavaScript properties
> + * Emits,signal: "window,object,cleared" woth no parameters]
typo

> + */
> +void ewk_view_window_object_cleared(Evas_Object* o)
> +{
> +    evas_object_smart_callback_call(o, "window,object,cleared", 0);
It's only used on the same file. If you really want this callback, you could put this function in ewk_view_load_started(), though I'm  not sure if this is the right place.

>  }
>  
>  /**
> diff --git a/WebKit/efl/ewk/ewk_view.h b/WebKit/efl/ewk/ewk_view.h
> index 8dd6178..aa07926 100644
> --- a/WebKit/efl/ewk/ewk_view.h
> +++ b/WebKit/efl/ewk/ewk_view.h
> @@ -480,6 +480,8 @@ EAPI float ewk_view_zoom_range_max_get(Evas_Object* o);
>  EAPI void ewk_view_user_scalable_set(Evas_Object* o, Eina_Bool user_scalable);
>  EAPI Eina_Bool ewk_view_user_scalable_get(Evas_Object* o);
>  
> +EAPI void ewk_view_window_object_cleared(Evas_Object* o);

Why is this an exported function?

[Adam Barth]

Comment on attachment 65258 [details]
Enhancements for WebKit-EFL port, that are used by Dump Render Tree application

Please address the informal review comments above and submit a new patch.

[Gyuyoung Kim]

Hello Krzysztof,

I wonder when you report new patch again.

[Krzysztof Czech]

> + */
> +void ewk_view_window_object_cleared(Evas_Object* o)
> +{
> +    evas_object_smart_callback_call(o, "window,object,cleared", 0);

Yes, this callback could be moved to the ewk_view_load_started().

> +EAPI void ewk_view_window_object_cleared(Evas_Object* o);
Yes, this should not be exported. This function calls only one callback so I moved its body to the ewk_view_load_started(). Previously this function was called from ewk_view_load_started().

[Gyuyoung Kim]

EFL port should have DRT for LayoutTest. So, we should land this patch. Krzysztof, if you are busy, I think it is better to make this patch by other developer. How do you think about it ?

[Krzysztof Czech]

(In reply to comment #11)
> EFL port should have DRT for LayoutTest. So, we should land this patch. Krzysztof, if you are busy, I think it is better to make this patch by other developer. How do you think about it ?

Hello Gyuyoung,
Indeed I am pretty busy, but recently I have started updating this patch. I am in a middle of a work, so as soon as I finish it, I will provide a patch.

[Gyuyoung Kim]

(In reply to comment #12)
> (In reply to comment #11)
> > EFL port should have DRT for LayoutTest. So, we should land this patch. Krzysztof, if you are busy, I think it is better to make this patch by other developer. How do you think about it ?
> 
> Hello Gyuyoung,
> Indeed I am pretty busy, but recently I have started updating this patch. I am in a middle of a work, so as soon as I finish it, I will provide a patch.

Good. I finish to prepare to buy servers for EFL layout test. If DRT is run on EFL, I will start to setup EFL layout test server. Please hurry up. :-)

[Leandro Pereira]

Closing bug as FIXED: feature is being implemented in a different way.