CEF3: Move LoadRequest to the browser process, use data: URL for LoadString

GoogleCodeExporter commented 9 years ago

What steps will reproduce the problem?
Replace calls to CefFrame::LoadURL with CefFrame::LoadRequest. This can be 
accomplished by creating a CefRequest object, calling SetURL with a given URL, 
and passing said object to CefFrame::LoadRequest. A convenient place to do this 
is in the cefclient_win.cpp file used for the cefclient test app.

What is the expected output? What do you see instead?
If the steps above are followed, we should navigate to a different page when a 
different URL is entered on the address bar. Instead, nothing happens.

What version of the product are you using? On what operating system?
CEF3, revision 588. Windows 7.

Please provide any additional information below.
This was first discovered through resolving 
http://magpcss.org/ceforum/viewtopic.php?f=6&t=784

I believe the problem is somewhere in CefBrowserHostImpl::LoadRequest, but I'm 
not sure where the problem is, exactly.

Original issue reported on code.google.com by nesf...@gmail.com on 24 Apr 2012 at 10:20

GoogleCodeExporter commented 9 years ago

You need to load at least one URL before you can use LoadRequest. For example, 
you can pass "about:blank" as the |url| parameter to 
CefBrowserHost::CreateBrowser. Once the browser has been created you can then 
call LoadRequest(). See RequestSendRecvTestHandler in 
tests/unittests/request_unittest.cc for an example.

Original comment by magreenb...@gmail.com on 22 May 2012 at 3:43

Changed state: Accepted

GoogleCodeExporter commented 9 years ago

Issue 727 has been merged into this issue.

Original comment by magreenb...@gmail.com on 24 Sep 2012 at 3:40

GoogleCodeExporter commented 9 years ago

There is currently no unit test for the LoadString case but it's likely the 
same problem.

Original comment by magreenb...@gmail.com on 24 Sep 2012 at 3:41

Changed title: CEF3: LoadRequest and LoadString don't work until URL is loaded
Added labels: CEF3

GoogleCodeExporter commented 9 years ago

Issue 797 has been merged into this issue.

Original comment by magreenb...@gmail.com on 26 Nov 2012 at 6:06

GoogleCodeExporter commented 9 years ago

Issue 797 has been merged into this issue.

Original comment by magreenb...@gmail.com on 16 Oct 2013 at 2:35

GoogleCodeExporter commented 9 years ago

I experience this problem too.

Original comment by alervd...@gmail.com on 20 Nov 2013 at 4:36

GoogleCodeExporter commented 9 years ago

In CEF3 branch 1650 the following problem is reproducible : 
1. Navigate to a page say google.com
2. Navigate to a non-existing address, ex: 
http://insert-random-junk-chars-here.com
3. cefclient enters infinite loop where OnLoadError is called due to previous 
frame->LoadString() in client_handler.cpp ClientHandler::OnLoadError

Original comment by nvine...@gmail.com on 28 Jan 2014 at 12:58

GoogleCodeExporter commented 9 years ago

Issue 1277 has been merged into this issue.

Original comment by magreenb...@gmail.com on 20 May 2014 at 3:46

GoogleCodeExporter commented 9 years ago

LoadRequest can be implemented from the browser process using 
NavigationController::LoadURLWithParams. The LoadURLParams structure supports 
extra headers, post data and target frame.

Original comment by magreenb...@gmail.com on 20 May 2014 at 3:48

Changed title: CEF3: Move LoadRequest to the browser process, and remove LoadString

GoogleCodeExporter commented 9 years ago

Issue 641 has been merged into this issue.

Original comment by magreenb...@gmail.com on 20 May 2014 at 3:50

GoogleCodeExporter commented 9 years ago

@comment#10: Also set should_clear_history_list via LoadRequest from the 
browser process.

Original comment by magreenb...@gmail.com on 20 May 2014 at 3:51

GoogleCodeExporter commented 9 years ago

I have a patch for LoadString and LoadRequest to go through 
NavigationController::LoadURLWithParams.  I ran into a some problems supporting 
all the existing options for LoadRequest:

1. No support for first_party_for_cookies
2. No support for load_flags
3. LoadURLWithParams expects a vector<char> for the body, so no support for 
multi-part body

To support LoadString() I converted the string content into a data: url

Original comment by gber...@gmail.com on 27 Jun 2014 at 4:03

Attachments:

cef_patch.txt

GoogleCodeExporter commented 9 years ago

@#12: Thanks for the patch.

> I ran into a some problems supporting all the existing options for 
LoadRequest:
> 
> 1. No support for first_party_for_cookies
> 2. No support for load_flags
> 3. LoadURLWithParams expects a vector<char> for the body, so no support for 
multi-part body

These should be fine. |load_flags| is primarily intended for use with 
CefURLRequest in any case.

> To support LoadString() I converted the string content into a data: url

This is a nice solution.

Some comments about your patch:

1. Please add a unit test for LoadString.

2. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=579&aid=5790
012000&name=cef_patch.txt&token=ABZ6GAdoKLA17EoEMbvKRfoYk1YW885PaQ%3A14038857607
61#488

+    IPC_MESSAGE_HANDLER(CefMsg_LoadRequest, LoadRequestParams)

A. Rename CefMsg_LoadRequest to CefHostMsg_LoadRequest because it is now sent 
from the render process to the browser process.
B. Remove the CefMsg_LoadRequest handler from CefBrowserImpl because it's no 
longer used.
C. Remove members from CefHostMsg_LoadRequest that are no longer supported.

3. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=579&aid=5790
012000&name=cef_patch.txt&token=ABZ6GAdoKLA17EoEMbvKRfoYk1YW885PaQ%3A14038857607
61#166

+    content::NavigationController::LoadURLParams params(
+      GetGURL(url));

Indent the 2nd+ lines 4 spaces instead of 2. Same with other locations.

4. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=579&aid=5790
012000&name=cef_patch.txt&token=ABZ6GAdoKLA17EoEMbvKRfoYk1YW885PaQ%3A14038857607
61#295

+};
 }  // namespace

Add a space between end of class and end of namespace.

Original comment by magreenb...@gmail.com on 27 Jun 2014 at 4:39

GoogleCodeExporter commented 9 years ago

@13:  Thanks for the feedback.  I'll work on your suggestions.  Quick comment 
on:
> 2B. Remove the CefMsg_LoadRequest handler from CefBrowserImpl because it's no 
longer used.

This is still used for navigating from the render process in frame_impl.cc

Original comment by gber...@gmail.com on 27 Jun 2014 at 4:56

GoogleCodeExporter commented 9 years ago

@#14: But the message is no longer sent from the browser process to the render 
process, right? So you don't need to handle the message in 
CefBrowserImpl::OnMessageReceived.

Original comment by magreenb...@gmail.com on 27 Jun 2014 at 4:58

GoogleCodeExporter commented 9 years ago

@#15: Yup, I understand now. Thanks

Original comment by gber...@gmail.com on 27 Jun 2014 at 5:01

GoogleCodeExporter commented 9 years ago

@#12: I'm having some trouble writing up a unit test for LoadString.

1. None of the CefRequestHandler methods (OnBeforeResourceLoad, 
GetResourceHandler, etc...) seem to be triggered by LoadString.  So I can't 
compare the CefRequest to the expected data url (similar to the SendRecv test 
in request_test.cc)

2. I've tried using CefFrame::GetSource during OnLoadEnd, but the 
CefStringVisitor::Visit method doesn't get triggered either.  I'm not sure if 
there is a timing issue causing this since the visitor is meant to be called 
asynchronously.  I can see the content getting loading the browser window so it 
is there but perhaps not during OnLoadEnd.

Do you have any suggestions on now to implement a unit test for LoadString?

Original comment by gber...@gmail.com on 30 Jun 2014 at 4:36

GoogleCodeExporter commented 9 years ago

@#17: For 2, are you waiting for the VIsit method to be called asynchronously?

Original comment by magreenb...@gmail.com on 30 Jun 2014 at 4:48

GoogleCodeExporter commented 9 years ago

@#17: I'm not sure how to do that.  As an aside, is there a way to run an 
individual test without running through the entire test suite?

Thanks,

Original comment by gber...@gmail.com on 30 Jun 2014 at 4:52

GoogleCodeExporter commented 9 years ago

@#19:

> I'm not sure how to do that.

Create a TestHandler that also implements CefStringVisitor. Call 
CefFrame::GetSource from inside OnLoadEnd using |this| as the visitor. Call 
DestroyTest from the Visit implementation after verifying that the contents are 
correct.

> As an aside, is there a way to run an individual test without running through 
the entire test suite?

Pass --gtest_filter=TestType.TestName on the command-line.

Original comment by magreenb...@gmail.com on 30 Jun 2014 at 4:58

GoogleCodeExporter commented 9 years ago

@#20:  Excellent... I got this working but my impl is a bit hacky. See attached.

I couldn't create a TestHandler that implements CefStringVisitor directly, 
because CefFrame::GetSource takes in a CefRefPtr.  So I can't pass in |this| 
into GetSource.  

Is there a CEF equivalent to boost::enable_shared_from_this I could use?  Or do 
you have any other idea on how to clean this up?

Original comment by gber...@gmail.com on 30 Jun 2014 at 6:02

Attachments:

load_string_test.txt

GoogleCodeExporter commented 9 years ago

Here's the actual patch for the load string test

Original comment by gber...@gmail.com on 30 Jun 2014 at 6:35

Attachments:

load_string_test.txt

GoogleCodeExporter commented 9 years ago

@#21: You should be able to assign |this| to a CefRefPtr, and then pass in the 
CefRefPtr. For example: CefRefPtr<CefStringVisitor> visitor(this);

Original comment by magreenb...@gmail.com on 30 Jun 2014 at 7:09

GoogleCodeExporter commented 9 years ago

@#23: That works. Here's the updated patch for LoadString. Thanks again for the 
feedback.

Original comment by gber...@gmail.com on 30 Jun 2014 at 7:35

Attachments:

cef_patch.txt

GoogleCodeExporter commented 9 years ago

@#24: Looks good, thanks.

Original comment by magreenb...@gmail.com on 30 Jun 2014 at 8:25

Added labels: NextRelease

GoogleCodeExporter commented 9 years ago

Any chance this can be patched into the 1916 branch?

Original comment by gber...@gmail.com on 1 Jul 2014 at 8:46

Attachments:

cef_patch_1916.txt

GoogleCodeExporter commented 9 years ago

@#26: Yes, it will be merged to 1916 as well.

Original comment by magreenb...@gmail.com on 2 Jul 2014 at 9:46

GoogleCodeExporter commented 9 years ago

Issue 763 has been merged into this issue.

Original comment by magreenb...@gmail.com on 2 Jul 2014 at 10:25

GoogleCodeExporter commented 9 years ago

Original comment by magreenb...@gmail.com on 2 Jul 2014 at 10:26

Changed title: CEF3: Move LoadRequest to the browser process, use data: URL for LoadString

GoogleCodeExporter commented 9 years ago

Issue 1185 has been merged into this issue.

Original comment by magreenb...@gmail.com on 2 Jul 2014 at 10:58

GoogleCodeExporter commented 9 years ago

@#24: Added in trunk revision 1765 and 1916 branch revision 1766 with minor 
changes.

Original comment by magreenb...@gmail.com on 10 Jul 2014 at 6:24

Changed state: Fixed

GoogleCodeExporter commented 9 years ago

@#31: The changes will be reverted due to the following problems (see also 
issue #1339):

1. WebContentsImpl::NavigateToPendingEntry ignores the specified 
frame_tree_node_id unless the "--site-per-process" flag is set. This mode is 
still experimental and not fully implemented so we can't use it yet.

2. The frame_id value (which comes from RenderFrame routing ID) is not the same 
as frame_tree_node_id. NavigationHelper will need to convert from frame_id to 
frame_tree_node_id, perhaps using FrameTree::FindByRoutingID. Otherwise, 
|frame_tree_.FindByID| in WebContentsImpl::NavigateToPendingEntry returns NULL.

3. Need to add unit tests for the sub-frame loading use case.

Original comment by magreenb...@gmail.com on 16 Jul 2014 at 9:05

Changed state: Accepted
Removed labels: NextRelease

GoogleCodeExporter commented 9 years ago

Issue 1339 has been merged into this issue.

Original comment by magreenb...@gmail.com on 16 Jul 2014 at 9:06

GoogleCodeExporter commented 9 years ago

@#32: Reverted in trunk revision 1780 and 1916 branch revision 1781.

Also:

4. When re-applying the changes to trunk remove the FilePathStringToWebString 
function from browser_impl.cc since it is no longer used.

Original comment by magreenb...@gmail.com on 16 Jul 2014 at 9:29

GoogleCodeExporter commented 9 years ago

Trunk revision 2028 and 2272 branch revision 2029 update cefclient to use a 
data: URI instead of LoadString for loading error messages. This also adds 
base64 and URI encoding/decoding functions to cef_url.h.

Original comment by magreenb...@gmail.com on 11 Feb 2015 at 6:17

GoogleCodeExporter commented 9 years ago

CEF is transitioning from Google Code to Bitbucket project hosting. If you 
would like to continue receiving notifications on this issue please add 
yourself as a Watcher at the new location: 
https://bitbucket.org/chromiumembedded/cef/issue/579

Original comment by magreenb...@gmail.com on 14 Mar 2015 at 3:22

alderlopez / chromiumembedded

CEF3: Move LoadRequest to the browser process, use data: URL for LoadString #579