-
Notifications
You must be signed in to change notification settings - Fork 653
[Android] Fix XWALK unable to support download behavior #2590
base: master
Are you sure you want to change the base?
Conversation
Testing patch series with js0701/crosswalk@85afe79 as its head.
|
BUG, not [BUG] |
Your commit message is extremely laconic. How was it broken? How did you fix it? |
Testing patch series with js0701/crosswalk@07c5115 as its head.
|
@rakuco
|
It's not trivial for the poor person who's going to look at this commit 2 years from now and wonder why things were done they way they were. Please update your commit message with more information. You have to answer why things are being done. If you say it's a fix, you also have to mention if it's always been broken or what commit broke it otherwise. |
@@ -373,12 +373,16 @@ public void onDownloadStart(String url, | |||
String contentDisposition, | |||
String mimeType, | |||
long contentLength) { | |||
if (mDownloadListener != null) { | |||
mDownloadListener.onDownloadStart(url, userAgent, | |||
contentDisposition, mimeType, contentLength ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent
Testing patch series with js0701/crosswalk@cff355d as its head.
|
Several bugs reported that Xwalk not able to support "download" attribute from anchor since XWALK4 Changes in this commit 1) Implement onDownloadStart method in XWalkContentsClientBridge to call download listener method 2) Ignore render_frame_id which is set to MSG_ROUTING_NONE from content for download request 3) In runtime_download_manager_deledate, return the suggested path to content 4) In runtime_resource_dispatcher_host_delegate_android, don't cancel request, because content will use it later on to futher operation(rename, trunk..etc) BUG=https://crosswalk-project.org/jira/browse/XWALK-1079
while (iterator != rfh_to_io_thread_client_.end()) { | ||
if (iterator->first.first == rfh_id.first) | ||
break; | ||
iterator++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change coming from upstream? Any reference commit from upstream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wang16 The change is not from upstream, I debugged and found the solution by myself.
The story is Content is using MSG_ROUTING_NONE for render_frame_id. Previous implementation fails to find IoThreadClientData for it. Current change will ignore MSG_ROUTING_NONE..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And It seems both content shell and Android Webview falls to download by my test, so I am wondering where to find reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is this an upstream bug? will this change apply to upstream as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to investigate. But I think we can commit in XWALK first, as in the bug, customer is waiting for the fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wang16
https://code.google.com/p/chromium/issues/detail?id=432414&thanks=432414&ts=1415773645
This is the bug link, seems no response yet.
There are 2 implementions from upstream, while Crosswalk's implementation is almost the same with Androidwebview for the part which support download. AndroidWebview does not support this download too...The other implementation is from Chrome Shell, which is very different from XWalk's
Hi, what's the status of this bug? I really need support for downloading files to the device for my next project. Thanks! |
@@ -164,7 +164,7 @@ void RuntimeDownloadManagerDelegate::ChooseDownloadPath( | |||
if (GetSaveFileName(&save_as)) | |||
result = base::FilePath(std::wstring(save_as.lpstrFile)); | |||
#else | |||
NOTIMPLEMENTED(); | |||
result = suggested_path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will change the Crosswalk behavior in many other platforms beside the Android.
Big problem for our app as well. |
BUG=https://crosswalk-project.org/jira/browse/XWALK-1079