Bug 82192 – [EFL][DRT] Catch the "resource,request,willsend" signal

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

EFL's DRT should catch the "resource,request,willsend" signal in order to implement the "Blocked access to external URL" behavior.

This allows the following test cases to be removed from the skip list:
    fast/ruby/before-block-doesnt-crash.html
    fast/ruby/before-table-doesnt-crash.html
    fast/ruby/generated-before-counter-doesnt-crash.html
    fast/workers/worker-crash-with-invalid-location.html

[Chris Dumez]

Created attachment 133803 [details]
Proposed patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 133803 [details]
Proposed patch

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

> Source/WebKit/efl/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

Style-wise, the "Reviewed by" line usually goes right after the bug URL.

> Source/WebKit/efl/ChangeLog:14
> +        * WebCoreSupport/FrameLoaderClientEfl.cpp:
> +        (WebCore::FrameLoaderClientEfl::dispatchWillSendRequest):

It looks like this part of the patch is missing.

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:310
> +

Style nitpick: extra empty line here.

[Chris Dumez]

Created attachment 133979 [details]
Improved patch based on Raphael Kubo da Costa's review

[Gyuyoung Kim]

Comment on attachment 133979 [details]
Improved patch based on Raphael Kubo da Costa's review

Looks fine before.

[Gyuyoung Kim]

(In reply to comment #4)
> (From update of attachment 133979 [details])
> Looks fine before.
Oops. I mean looks better than before. :-)

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 133979 [details]
Improved patch based on Raphael Kubo da Costa's review

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

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:102
> +    Evas_Object* mainFrame = ewk_view_frame_main_get(view);
> +    if (mainFrame)

Sorry for not noticing this earlier. Some time ago I added code which emitted "resource,request,willsend" on the view when a request was sent to its main frame (see FrameLoaderClientEfl::dispatchWillSendRequest). Have you checked if it works in this case?

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:308
> +        return;

This return statement is unnecessary.

[Chris Dumez]

(In reply to comment #6)
> (From update of attachment 133979 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133979&action=review
> 
> > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:102
> > +    Evas_Object* mainFrame = ewk_view_frame_main_get(view);
> > +    if (mainFrame)
> 
> Sorry for not noticing this earlier. Some time ago I added code which emitted "resource,request,willsend" on the view when a request was sent to its main frame (see FrameLoaderClientEfl::dispatchWillSendRequest). Have you checked if it works in this case?

My initial attempt was to listen for the "resource,request,willsend" signal on the view (instead of on the main frame). However, this did not work because my callback would only get called when the main frame is loaded, but not when each subcomponent of the main frame gets loaded. The tests check for subcomponents in the frame.

[Raphael Kubo da Costa (:rakuco)]

Alright then, thanks.

[Chris Dumez]

Created attachment 134106 [details]
Improved patch based on Raphael Kubo da Costa's review (#2)

Removed the "useless" return statement.

[Chris Dumez]

@Raphael Kubo da Costa: I ran more test cases and figured out that I'm not getting the "resource,request,willsend" signal for resources in a iframe (e.g. "LayoutTests/http/tests/misc/window-dot-stop.html").

I believe this is a side effect on me listening for the signal on the main frame instead of the view.

However, unlike the GTK port equivalent, the FrameLoaderClientEfl dispatchWillSendRequest() will emit the "resource,request,willsend" signal on the view, only for the main frame:

************
    ewk_frame_request_will_send(m_frame, &messages);
    // We want to distinguish between a request for a document to be loaded into
    // the main frame, a sub-frame, or the sub-objects in that document (via Chromium).
    if (loader) {
        const FrameLoader* frameLoader = loader->frameLoader();
        const bool isMainFrameRequest = (loader == frameLoader->provisionalDocumentLoader() && frameLoader->isLoadingMainFrame());
        if (isMainFrameRequest)
            evas_object_smart_callback_call(m_view, "resource,request,willsend", &messages);
    }
************

Is this really required or can I remove the check and emit the signal on the view for all resources (like in GTK port)?

[Chris Dumez]

Created attachment 134293 [details]
Second patch proposal (listening for signal on the view)

Here is a second proposal where DRT is listening for "resource,request,willsend" signal on the view (not the main frame).
To support this, I'm now emitting the signal on the view for all resources (not just the main frame).

Also, to help the browser identify the frame (seems like it was the needed use-case), I added a frame property to the Ewk_Frame_Resource_Request struct.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #10) 
> Is this really required or can I remove the check and emit the signal on the view for all resources (like in GTK port)?

I imported this code from Chromium some time ago while working on a downstream project; as I don't have access to the project's code anymore, I'll let glima (CC'ed) answer more properly.

In short, from what I remember, the idea was to be able to differentiate requests to the main frame from other types of requests, so that SSL requests to servers with invalid SSL certificates would be made only if they were to the main frame and the user had accepted that, and dropped automatically otherwise.

[Chris Dumez]

Created attachment 134512 [details]
Second patch proposal (listening for signal on the view)

Forgot to update the Changelogs in the patch.

[Chris Dumez]

Created attachment 134521 [details]
Second patch proposal (listening for signal on the view)

Forgot to update the ewk_view signals documentation accordingly.

[Gyuyoung Kim]

(In reply to comment #12)

> In short, from what I remember, the idea was to be able to differentiate requests to the main frame from other types of requests, so that SSL requests to servers with invalid SSL certificates would be made only if they were to the main frame and the user had accepted that, and dropped automatically otherwise.

I have not used this function yet. Are there any problems when we fire a signal on frame ?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #15)
> (In reply to comment #12)
> 
> > In short, from what I remember, the idea was to be able to differentiate requests to the main frame from other types of requests, so that SSL requests to servers with invalid SSL certificates would be made only if they were to the main frame and the user had accepted that, and dropped automatically otherwise.
> 
> I have not used this function yet. Are there any problems when we fire a signal on frame ?

No, there aren't -- the frame signal is still being emitted. It's just that listening on the frame signal cannot do what I mentioned above.

[Chris Dumez]

Created attachment 135296 [details]
Proposed patch

Updated the patch so that the Ewk_Frame_Resource_Request now contains an "is_main_frame_request" boolean flag.
The browser needs to distinguish requests that are made for the main frame from those made for sub-resources.

[Chris Dumez]

Created attachment 135306 [details]
Proposed patch

Updated the Changelogs in the patch.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 135306 [details]
Proposed patch

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

Thanks for the work, looks almost good to me.

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:-176
> -    // We want to distinguish between a request for a document to be loaded into
> -    // the main frame, a sub-frame, or the sub-objects in that document (via Chromium).

Please keep the comment in the code for the chunk it's related to to be easier to understand.

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:303
> +        return;

`return' not needed.

[Chris Dumez]

Created attachment 135315 [details]
Proposed patch

Updated based on informal review.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 135315 [details]
Proposed patch

LGTM.

[Gustavo Noronha (kov)]

Comment on attachment 135315 [details]
Proposed patch

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

cq- because of the comments

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:169
> +    // the main frame, a sub-frame, or the sub-objects in that document (via Chromium).

I know this line was still here, but via Chromium here means what? you are doing the same chromium does? or the author misspelled Chrome(Client)?

> Source/WebKit/efl/ewk/ewk_frame.h:110
> +    Eina_Bool is_main_frame_request; /** < indicates is the request is for the main frame */

is the request -> if the request

[Chris Dumez]

Created attachment 135570 [details]
Proposed patch

Update to take Gustavo Noronha's comment into consideration.

[Chris Dumez]

Created attachment 135577 [details]
Proposed patch

Rebase on master since it does not apply anymore.

[Kristóf Kosztyó]

Comment on attachment 135577 [details]
Proposed patch

Rejecting attachment 135577 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Chris Dumez]

Created attachment 135645 [details]
Proposed patch

Updated reviewer name.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 135645 [details]
Proposed patch

Clearing flags on attachment: 135645

Committed r113224: <http://trac.webkit.org/changeset/113224>

[Raphael Kubo da Costa (:rakuco)]

All reviewed patches have been landed.  Closing bug.