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
Created attachment 133803 [details] Proposed patch
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.
Created attachment 133979 [details] Improved patch based on Raphael Kubo da Costa's review
Comment on attachment 133979 [details] Improved patch based on Raphael Kubo da Costa's review Looks fine before.
(In reply to comment #4) > (From update of attachment 133979 [details]) > Looks fine before. Oops. I mean looks better than before. :-)
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.
(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.
Alright then, thanks.
Created attachment 134106 [details] Improved patch based on Raphael Kubo da Costa's review (#2) Removed the "useless" return statement.
@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)?
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.
(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.
Created attachment 134512 [details] Second patch proposal (listening for signal on the view) Forgot to update the Changelogs in the patch.
Created attachment 134521 [details] Second patch proposal (listening for signal on the view) Forgot to update the ewk_view signals documentation accordingly.
(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 ?
(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.
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.
Created attachment 135306 [details] Proposed patch Updated the Changelogs in the patch.
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.
Created attachment 135315 [details] Proposed patch Updated based on informal review.
Comment on attachment 135315 [details] Proposed patch LGTM.
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
Created attachment 135570 [details] Proposed patch Update to take Gustavo Noronha's comment into consideration.
Created attachment 135577 [details] Proposed patch Rebase on master since it does not apply anymore.
Comment on attachment 135577 [details] Proposed patch Rejecting attachment 135577 [details] from commit-queue. rakuco@webkit.org 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.
Created attachment 135645 [details] Proposed patch Updated reviewer name.
Comment on attachment 135645 [details] Proposed patch Clearing flags on attachment: 135645 Committed r113224: <http://trac.webkit.org/changeset/113224>
All reviewed patches have been landed. Closing bug.