New API that are used by Dump Render Tree application
Created attachment 65240 [details] Enhancements for WebKit-EFL port, that are used by Dump Render Tree application
Hello Krzysztof, I think you need to fill doxyzen for new methods.
Created attachment 65258 [details] Enhancements for WebKit-EFL port, that are used by Dump Render Tree application
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 ;-)
First, I would like to hear from the other EFL guys how the additions like, API-wise.
(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)
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 @@ > +2010-08-24 Krzysztof Czech <k.czech@samsung.com> > + > + 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?
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.
Hello Krzysztof, I wonder when you report new patch again.
> + */ > +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().
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 ?
(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.
(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. :-)
Closing bug as FIXED: feature is being implemented in a different way.