Bug 82838 – [EFL] EFL's LayoutTestController keepWebHistory implementation

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Mikhail Pozdnyakov]

EFL's LayoutTestController needs keepWebHistory implementation in order to 
unskip 
fast/history/visited-generated-content-test.html
fast/history/visited-link-background-color.html
fast/history/window-open.html

LayoutTestController  keepWebHistory implementation requires shared history support.

[Mikhail Pozdnyakov]

Created attachment 135059 [details]
ewk_history is extended  to support shared history feature + dummy implementation of FrameLoaderClientEfl::updateGlobalHistory.

[WebKit Review Bot]

Comment on attachment 135059 [details]
ewk_history is extended  to support shared history feature + dummy implementation of FrameLoaderClientEfl::updateGlobalHistory.

Rejecting attachment 135059 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
offset 12 lines).
patching file Source/WebKit/efl/ewk/ewk_history.cpp
patching file Source/WebKit/efl/ewk/ewk_history.h
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp
Hunk #1 succeeded at 108 with fuzz 1.
Hunk #2 succeeded at 161 (offset 7 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Nate Chapin']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12477015

[Raphael Kubo da Costa (:rakuco)]

Please don't cq+ it yet, I'd like to take a look at it before.

[Gyuyoung Kim]

Comment on attachment 135059 [details]
ewk_history is extended  to support shared history feature + dummy implementation of FrameLoaderClientEfl::updateGlobalHistory.

View in context: https://bugs.webkit.org/attachment.cgi?id=135059&action=review

> LayoutTests/ChangeLog:8
> +        * platform/efl/Skipped:

You don't mention any description for this change.

> Source/WebKit/efl/ewk/ewk_history.h:69
> +EAPI void ewk_history_free(Ewk_History* history);

Move * to variable name side. 

(Ewk_History *history);

> Tools/ChangeLog:7
> +

Missing description too.

[Mikhail Pozdnyakov]

Created attachment 140184 [details]
rebased + fixed changelog description

[Mikhail Pozdnyakov]

(In reply to comment #4)
> (From update of attachment 135059 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135059&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        * platform/efl/Skipped:
> 
> You don't mention any description for this change.
> 
> > Source/WebKit/efl/ewk/ewk_history.h:69
> > +EAPI void ewk_history_free(Ewk_History* history);
> 
> Move * to variable name side. 
> 
> (Ewk_History *history);
> 
> > Tools/ChangeLog:7
> > +
> 
> Missing description too.
Fixed.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 140184 [details]
rebased + fixed changelog description

View in context: https://bugs.webkit.org/attachment.cgi?id=140184&action=review

The API does not look very good to me yet:

 - "optional_shared_history" is too clunky. I still don't know if it makes sense to make the shared history an Ewk_History at all, or go the Qt route and have a separate object for that.
 - There seems to be no way to destroy the shared history object correctly (it's not done in DRT, and Qt uses qAddPostRoutine for that).

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:961
> +    // FIXME: primitive behaviour yet for enabling Layout tests. See mac or win ports implementation.

Can you elaborate on what these ports do differently from us?

> Source/WebKit/efl/ewk/ewk_history.cpp:215
> +int ewk_history_all_item_count(const Ewk_History *history)
> +{
> +    EWK_HISTORY_CORE_GET_OR_RETURN(history, core, 0);
> +    return static_cast<int>(core->entries().size());
> +}

I wonder if there's really a need for this method -- it's the same as back_list_length + forward_list_length.

> Tools/ChangeLog:9
> +        LayoutTestController::keepWebHistory() implementation added.
> +        LayoutTestController::webHistoryItemCount() returns count of all history items including the current one. 

These can be moved to the method list below.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:112
> +    if (!ewk_history_optional_shared_history_get())
> +        ewk_history_optional_shared_history_set(ewk_history_new());

Don't you need to reset this after each test?

[Eric Seidel (no email)]

Comment on attachment 135059 [details]
ewk_history is extended  to support shared history feature + dummy implementation of FrameLoaderClientEfl::updateGlobalHistory.

Cleared Nate Chapin's review+ from obsolete attachment 135059 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Gyuyoung Kim]

Comment on attachment 140184 [details]
rebased + fixed changelog description

View in context: https://bugs.webkit.org/attachment.cgi?id=140184&action=review

Set cq- because of Kubo comment and doxyzen errors.

> Source/WebKit/efl/ewk/ewk_history.h:56
> + * @return newly allocated history instance or @c NULL on errors. You must

Nit : We don't use '.' at the end of *@XXXX* sentence. In addition, new line is placed just below at the beginning of description.

> Source/WebKit/efl/ewk/ewk_history.h:74
> + * @return shared history instance or @c NULL if shared history is not set.

ditto.

> Source/WebKit/efl/ewk/ewk_history.h:81
> + * @note when a new history instance is set the previous one is not freed.

ditto.

> Source/WebKit/efl/ewk/ewk_history.h:83
> + * @param history the history to use for the shared history.

ditto.

> Source/WebKit/efl/ewk/ewk_history.h:200
> + * @param history which history instance to query.

ditto.

> Source/WebKit/efl/ewk/ewk_history.h:202
> + * @return number of elements in whole list.

ditto.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.