Bug 85006 - [EFL][DRT] EFL's DRT navigation_policy_decision implementation
Summary: [EFL][DRT] EFL's DRT navigation_policy_decision implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
: 84842 (view as bug list)
Depends on: 85048
Blocks: 84842
  Show dependency treegraph
 
Reported: 2012-04-26 14:33 PDT by Mikhail Pozdnyakov
Modified: 2012-05-31 10:15 PDT (History)
5 users (show)

See Also:


Attachments
navigation_policy_decision + LayoutTestController::setCustomPolicyDelegate (10.71 KB, patch)
2012-04-27 05:51 PDT, Mikhail Pozdnyakov
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
navigation_policy_decision + LayoutTestController::setCustomPolicyDelegate (10.80 KB, patch)
2012-05-04 00:59 PDT, Mikhail Pozdnyakov
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
patch v3 (rebased) (10.88 KB, patch)
2012-05-21 01:38 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v4 (fixed comments from Gyuyoung) (10.88 KB, patch)
2012-05-21 05:26 PDT, Mikhail Pozdnyakov
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
patch v4 (resubmitted) (10.88 KB, patch)
2012-05-23 01:20 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v5(rebased) (10.86 KB, patch)
2012-05-23 01:36 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v6 (Skipped list clean up) (11.30 KB, patch)
2012-05-23 02:31 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v7 (Review comments are met. Thanks Chris) (11.02 KB, patch)
2012-05-31 07:49 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2012-04-26 14:33:20 PDT
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
Comment 1 Mikhail Pozdnyakov 2012-04-27 05:51:30 PDT
Created attachment 139177 [details]
navigation_policy_decision + LayoutTestController::setCustomPolicyDelegate
Comment 2 Gyuyoung Kim 2012-04-30 05:55:31 PDT
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
Comment 3 Mikhail Pozdnyakov 2012-05-02 00:46:46 PDT
(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.
Comment 4 Gyuyoung Kim 2012-05-03 04:28:10 PDT
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() ?
Comment 5 Mikhail Pozdnyakov 2012-05-04 00:59:24 PDT
Created attachment 140175 [details]
navigation_policy_decision + LayoutTestController::setCustomPolicyDelegate

use makeString
Comment 6 Mikhail Pozdnyakov 2012-05-04 01:00:42 PDT
(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.
Comment 7 Gyuyoung Kim 2012-05-04 01:05:01 PDT
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
Comment 8 Mikhail Pozdnyakov 2012-05-21 01:38:50 PDT
Created attachment 142968 [details]
patch v3 (rebased)
Comment 9 Gyuyoung Kim 2012-05-21 02:01:12 PDT
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?
Comment 10 Mikhail Pozdnyakov 2012-05-21 05:23:45 PDT
(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.
Comment 11 Mikhail Pozdnyakov 2012-05-21 05:26:46 PDT
Created attachment 143008 [details]
patch v4 (fixed comments from Gyuyoung)
Comment 12 Early Warning System Bot 2012-05-21 05:46:39 PDT
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
Comment 13 Csaba Osztrogonác 2012-05-21 06:07:36 PDT
(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.
Comment 14 Mikhail Pozdnyakov 2012-05-21 07:06:57 PDT
*** Bug 84842 has been marked as a duplicate of this bug. ***
Comment 15 Gyuyoung Kim 2012-05-23 01:17:49 PDT
It looks qt-wk2 ews had some problems. I think you need to submit patch again. Let's see ews result again.
Comment 16 Mikhail Pozdnyakov 2012-05-23 01:20:11 PDT
Created attachment 143495 [details]
patch v4 (resubmitted)
Comment 17 Mikhail Pozdnyakov 2012-05-23 01:36:08 PDT
Created attachment 143497 [details]
patch v5(rebased)
Comment 18 WebKit Review Bot 2012-05-23 01:41:29 PDT
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.
Comment 19 Mikhail Pozdnyakov 2012-05-23 02:31:22 PDT
Created attachment 143509 [details]
patch v6 (Skipped list clean up)
Comment 20 Chris Dumez 2012-05-29 08:43:20 PDT
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.
Comment 21 Mikhail Pozdnyakov 2012-05-31 07:49:18 PDT
Created attachment 145091 [details]
patch v7 (Review comments are met. Thanks Chris)
Comment 22 Chris Dumez 2012-05-31 07:53:50 PDT
Comment on attachment 145091 [details]
patch v7 (Review comments are met. Thanks Chris)

LGTM. Thanks.
Comment 23 Gyuyoung Kim 2012-05-31 08:06:06 PDT
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 ?
Comment 24 Chris Dumez 2012-05-31 08:13:33 PDT
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.
Comment 25 WebKit Review Bot 2012-05-31 10:15:37 PDT
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>
Comment 26 WebKit Review Bot 2012-05-31 10:15:46 PDT
All reviewed patches have been landed.  Closing bug.