Bug 85006 – [EFL][DRT] EFL's DRT navigation_policy_decision implementation

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

[Mikhail Pozdnyakov]

EFL's DRT (DumpRenderTreeView) needs to implement navigation_policy_decision in order to unskip
fast/encoding/mailto-always-utf-8.html

fast/forms/mailto/advanced-get.html
fast/forms/mailto/advanced-put.html
fast/forms/mailto/formenctype-attribute-button-html.html
fast/forms/mailto/formenctype-attribute-input-html.html
fast/forms/mailto/get-multiple-items-text-plain.html
fast/forms/mailto/get-multiple-items-x-www-form-urlencoded.html
fast/forms/mailto/get-multiple-items.html
fast/forms/mailto/get-non-ascii-always-utf-8.html
fast/forms/mailto/get-non-ascii-text-plain-latin-1.html
fast/forms/mailto/get-non-ascii-text-plain.html
fast/forms/mailto/get-non-ascii.html
fast/forms/mailto/get-overwrite-query.html
fast/forms/mailto/post-append-query.html
fast/forms/mailto/post-multiple-items-multipart-form-data.html
fast/forms/mailto/post-multiple-items-text-plain.html
fast/forms/mailto/post-multiple-items-x-www-form-urlencoded.html
fast/forms/mailto/post-multiple-items.html
fast/forms/mailto/post-text-plain-with-accept-charset.html
fast/forms/mailto/post-text-plain.html

[Mikhail Pozdnyakov]

Created attachment 139177 [details]
navigation_policy_decision + LayoutTestController::setCustomPolicyDelegate

[Gyuyoung Kim]

Comment on attachment 139177 [details]
navigation_policy_decision + LayoutTestController::setCustomPolicyDelegate

Attachment 139177 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12584241

[Mikhail Pozdnyakov]

(In reply to comment #2)
> (From update of attachment 139177 [details])
> Attachment 139177 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/12584241

This patch depends on other patch from bug85048.

[Gyuyoung Kim]

Comment on attachment 139177 [details]
navigation_policy_decision + LayoutTestController::setCustomPolicyDelegate

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

> Tools/DumpRenderTree/efl/DumpRenderTreeView.cpp:160
> +    String output = String("Policy delegate: attempt to load ") +  urlSuitableForTestResult(request->url) +

Why don't use use makeString() ?

[Mikhail Pozdnyakov]

Created attachment 140175 [details]
navigation_policy_decision + LayoutTestController::setCustomPolicyDelegate

use makeString

[Mikhail Pozdnyakov]

(In reply to comment #4)
> (From update of attachment 139177 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139177&action=review
> 
> > Tools/DumpRenderTree/efl/DumpRenderTreeView.cpp:160
> > +    String output = String("Policy delegate: attempt to load ") +  urlSuitableForTestResult(request->url) +
> 
> Why don't use use makeString() ?

Thanks for the hint! Done.

[Gyuyoung Kim]

Comment on attachment 140175 [details]
navigation_policy_decision + LayoutTestController::setCustomPolicyDelegate

Attachment 140175 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12627070

[Mikhail Pozdnyakov]

Created attachment 142968 [details]
patch v3 (rebased)

[Gyuyoung Kim]

Comment on attachment 142968 [details]
patch v3 (rebased)

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

> Tools/DumpRenderTree/efl/DumpRenderTreeView.cpp:176
> +static Eina_Bool onNavigationPolicyDecision(Ewk_View_Smart_Data*, Ewk_Frame_Resource_Request* request, Ewk_Navigation_Type navigationType )

Style nit : remove a space at the end of this line.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:213
> +    setCustomPolicyDelegate(true, true);

Does these test cases assumes policy delegation and permissive are always enabled?

[Mikhail Pozdnyakov]

(In reply to comment #9)
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:213
> > +    setCustomPolicyDelegate(true, true);
> 
> Does these test cases assumes policy delegation and permissive are always enabled?

LayoutTestController::waitForPolicyDelegate is used in 
fast/forms/mailto/*  (which are unskipped by this patch) and 
http/tests/download/inherited-encoding.html
http/tests/download/inherited-encoding-form-submission-result.html
http/tests/download/default-encoding.html

For all of the mentioned tests policy delegation enabled flag is crucial as affects logging, permissive flag is not crucial and tests can pass either with or without this flag. However Qt port for instance keeps permissive flag as false so we can do the same keeping consistency.

[Mikhail Pozdnyakov]

Created attachment 143008 [details]
patch v4 (fixed comments from Gyuyoung)

[Early Warning System Bot]

Comment on attachment 143008 [details]
patch v4 (fixed comments from Gyuyoung)

Attachment 143008 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12748015

[Csaba Osztrogonác]

(In reply to comment #12)
> (From update of attachment 143008 [details])
> Attachment 143008 [details] did not pass qt-wk2-ews (qt):
> Output: http://queues.webkit.org/results/12748015

Sorry, it was false positive alarm.

[Mikhail Pozdnyakov]

*** Bug 84842 has been marked as a duplicate of this bug. ***

[Gyuyoung Kim]

It looks qt-wk2 ews had some problems. I think you need to submit patch again. Let's see ews result again.

[Mikhail Pozdnyakov]

Created attachment 143495 [details]
patch v4 (resubmitted)

[Mikhail Pozdnyakov]

Created attachment 143497 [details]
patch v5(rebased)

[WebKit Review Bot]

Attachment 143497 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/platform/efl/test_expectations.txt:305:  fast/loader/policy-delegate-action-hit-test-zoomed.html is also in a Skipped file.  [test/expectations] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mikhail Pozdnyakov]

Created attachment 143509 [details]
patch v6 (Skipped list clean up)

[Chris Dumez]

Comment on attachment 143509 [details]
patch v6 (Skipped list clean up)

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

> LayoutTests/platform/efl/test_expectations.txt:301
> +// Needs custom policy delegate enhancement to log downloads - EFL port is missing api for getting header fields from HTTP responses. 

It would be nice to open bug reports for those. We try to avoid using BUGWKEFL in test_expectations.txt

> LayoutTests/platform/efl/test_expectations.txt:304
> +// Needs custom policy delegate enhancement to log DOM node info - EFL port is missing DOM node abstraction

Ditto.

> Tools/DumpRenderTree/efl/DumpRenderTreeView.cpp:181
> +    const String& output = makeString("Policy delegate: attempt to load ", urlSuitableForTestResult(request->url),

Why construct a WTF::String here? You could just put all that as printf arguments.

[Mikhail Pozdnyakov]

Created attachment 145091 [details]
patch v7 (Review comments are met. Thanks Chris)

[Chris Dumez]

Comment on attachment 145091 [details]
patch v7 (Review comments are met. Thanks Chris)

LGTM. Thanks.

[Gyuyoung Kim]

Comment on attachment 145091 [details]
patch v7 (Review comments are met. Thanks Chris)

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

Looks good to me.

> LayoutTests/platform/efl/test_expectations.txt:356
> +// Needs custom policy delegate enhancement to log downloads - EFL port is missing api for getting header fields from HTTP responses. 

Does *api* needs uppercase ?

[Chris Dumez]

Comment on attachment 145091 [details]
patch v7 (Review comments are met. Thanks Chris)

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

>> LayoutTests/platform/efl/test_expectations.txt:356
>> +// Needs custom policy delegate enhancement to log downloads - EFL port is missing api for getting header fields from HTTP responses. 
> 
> Does *api* needs uppercase ?

Right.

[WebKit Review Bot]

Comment on attachment 145091 [details]
patch v7 (Review comments are met. Thanks Chris)

Clearing flags on attachment: 145091

Committed r119116: <http://trac.webkit.org/changeset/119116>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.