Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

[Android] Fix XWALK unable to support download behavior #2590

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

js0701
Copy link
Contributor

@js0701 js0701 commented Nov 7, 2014

@crosswalk-trybot
Copy link

Testing patch series with js0701/crosswalk@85afe79 as its head.

Bot Status
Crosswalk Tizen 3 Common [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/1988)
Crosswalk Android-X86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/2398)
Crosswalk Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/2398)
Crosswalk Tizen IVI [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen IVI/builds/2391)

@rakuco
Copy link
Member

rakuco commented Nov 7, 2014

[BUG]=https://crosswalk-project.org/jira/browse/XWALK-1079

BUG, not [BUG]

@js0701
Copy link
Contributor Author

js0701 commented Nov 7, 2014

@xingnan @wang16 Please review. The PR is result after debug issue 1079 to support download to native filesystem behavior from XWALK

@rakuco
Copy link
Member

rakuco commented Nov 7, 2014

Your commit message is extremely laconic. How was it broken? How did you fix it?

@crosswalk-trybot
Copy link

Testing patch series with js0701/crosswalk@07c5115 as its head.

Bot Status
Crosswalk Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/2399)
Crosswalk Tizen IVI [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen IVI/builds/2392)
Crosswalk Android-X86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/2399)
Crosswalk Tizen 3 Common [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/1989)

@js0701
Copy link
Contributor Author

js0701 commented Nov 7, 2014

@rakuco
The change trivials

  1. Content use render_frame_id= MSG_ROUTING_NONE for download request, xwalk failed to change the IO_THREAD data
  2. onDownloadStart in XWalkContentsClientBridge was not implemented
  3. Suggested name was NULL for Android, fixed

@rakuco
Copy link
Member

rakuco commented Nov 7, 2014

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 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent

@crosswalk-trybot
Copy link

Testing patch series with js0701/crosswalk@cff355d as its head.

Bot Status
Crosswalk Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/2408)
Crosswalk Tizen IVI [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen IVI/builds/2401)
Crosswalk Android-X86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/2408)
Crosswalk Tizen 3 Common [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/1998)

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
@js0701
Copy link
Contributor Author

js0701 commented Nov 10, 2014

@rakuco @wang16 PTAL Thanks

while (iterator != rfh_to_io_thread_client_.end()) {
if (iterator->first.first == rfh_id.first)
break;
iterator++;
Copy link
Contributor

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?

Copy link
Contributor Author

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..

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

@aaccurso
Copy link

aaccurso commented Jan 2, 2015

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;
Copy link
Contributor

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.

@DanielJoyce
Copy link

Big problem for our app as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants