Resolve #206: Add ability to download files from the browser. #1289

iccub · 2019-07-24T09:50:03Z

All of file download logic and Downloads panel is taken from Firefox.
What was implemented by me:

download prompt is new
new menu entry for downloads panel

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
My patch or PR title has a standard commit message that looks like Fix #123: This fixes the shattered coffee cup! (or No Bug: <message> if no relevant ticket)
Unit Tests are updated to cover new or changed functionality
User-facing strings use NSLocalizableString()
New files have MPL-2.0 license header.

Test Plan:

Screenshots:

Reviewer Checklist:

PR is linked to an issue via Zenhub.
Issues are assigned to at least one epic.
Issues include necessary QA labels:
- QA/(Yes|No)
- release-notes/(include|exclude)
- bug / enhancement
Necessary security reviews have taken place.
Adequate test coverage exists to prevent regressions.
Adequate test plan exists for QA to validate (if applicable)

jumde · 2019-08-09T16:53:20Z

Running into a crash

(lldb) bt 5
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
    frame #0: 0x000000021a8990dc libsystem_kernel.dylib`__pthread_kill + 8
    frame #1: 0x000000021a912094 libsystem_pthread.dylib`pthread_kill$VARIANT$mp + 380
    frame #2: 0x000000021a7f2ea8 libsystem_c.dylib`abort + 140
    frame #3: 0x0000000219ebf788 libc++abi.dylib`abort_message + 132
    frame #4: 0x0000000219ebf934 libc++abi.dylib`default_terminate_handler() + 308

STR:

Navigate to http://cyberside.net.ee/ripping/
Click on 1click-bluray-copy_v1.0.0.6beta.exe

jumde · 2019-08-09T18:22:27Z

Downloads in the private-tab are visible in the download panel in the normal tab. I think its okay to keep the files, but we should remove the reference from the normal ( mimics the behavior on desktop )

jumde · 2019-08-09T19:53:42Z

Clear Private Data does not clear downloads.

jhreis · 2019-08-13T03:39:01Z

I am blocking this beyond security review. Please DO NOT MERGE, until talking with me. There are some timing mechanics that need to be worked out.

jhreis · 2019-08-21T16:54:38Z

Client/Info.plist

@@ -152,5 +152,7 @@
 			<string></string>
 		</dict>
 	</array>
+    <key>UIFileSharingEnabled</key>
+    <string>YES</string>


This needs to be a bool <true/>, will probably need to edit the plaintext, as I see Xcode has it stuck as a string type.

Client/Frontend/Browser/DownloadQueue.swift

jhreis · 2019-08-21T17:20:16Z

BraveShared/BraveStrings.swift

@@ -27,6 +27,8 @@ extension Strings {
    public static let OpenNewPrivateTabButtonTitle = NSLocalizedString("OpenNewPrivateTabButtonTitle", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Open in New Private Tab", comment: "Context menu option for opening a link in a new private tab")
    public static let DeleteLoginButtonTitle = NSLocalizedString("DeleteLoginButtonTitle", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Delete", comment: "Label for the button used to delete the current login.")
    public static let SaveButtonTitle = NSLocalizedString("SaveButtonTitle", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Save", comment: "Label for the button used to save data")
+    public static let Share = NSLocalizedString("CommonShare", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Share", comment: "Text for share action")


I think this comment could be a bit more clear, it is for translators, so something more like:

Text to select sharing something (example: image, video, URL)

jhreis · 2019-08-21T17:20:57Z

BraveShared/BraveStrings.swift

@@ -27,6 +27,8 @@ extension Strings {
    public static let OpenNewPrivateTabButtonTitle = NSLocalizedString("OpenNewPrivateTabButtonTitle", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Open in New Private Tab", comment: "Context menu option for opening a link in a new private tab")
    public static let DeleteLoginButtonTitle = NSLocalizedString("DeleteLoginButtonTitle", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Delete", comment: "Label for the button used to delete the current login.")
    public static let SaveButtonTitle = NSLocalizedString("SaveButtonTitle", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Save", comment: "Label for the button used to save data")
+    public static let Share = NSLocalizedString("CommonShare", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Share", comment: "Text for share action")
+    public static let Download = NSLocalizedString("CommonDownload", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Download", comment: "Text for download action")


Same as above, maybe:

Text to choose for downloading a file (example: saving an image to phone)

jhreis · 2019-08-21T17:21:56Z

BraveShared/BraveStrings.swift

@@ -512,6 +514,8 @@ extension Strings {
    public static let SettingsMenuItem = NSLocalizedString("SettingsMenuItem", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Settings", comment: "Title for settings menu item")
    public static let AddToMenuItem = NSLocalizedString("AddToMenuItem", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Add to...", comment: "Title for adding new bookmark menu item")
    public static let ShareWithMenuItem = NSLocalizedString("ShareWithMenuItem", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Share with...", comment: "Title for sharing url menu item")
+    public static let DownloadsMenuItem = NSLocalizedString("DownloadsMenuItem", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Downloads", comment: "Title for downloads menu item")
+    public static let DownloadsPanelEmptyStateTitle = NSLocalizedString("DownloadsPanelEmptyStateTitle", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Downloaded files will show up here.", comment: "Title for the Downloads screen empty state.")


comment maybe:

Title for when a user has nothing downloaded onto their device, and the list is empty.

Shared/DateGroupedTableData.swift

jhreis · 2019-08-22T00:44:00Z

Talked with @jumde, we can add a new check list item in Clear Private Data specifically for "downloads".

Client/Frontend/Browser/BrowserViewController.swift

Client/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift

jhreis · 2019-08-23T02:16:53Z

We are still blocked on:

security review
new copy for private browsing disclaimer text
cc'ing @tomlowenthal @jumde @anthonypkeane

Downloaded files will go straight to documents directory

jumde · 2019-08-26T17:50:24Z

Updated text:

Private Tabs aren’t saved in Brave, but they don’t make you anonymous online. Sites you visit in a private tab won’t show up in your history and their cookies always vanish when you close them — there won’t be any trace of them left in Brave. However, downloads will be saved.

Your mobile carrier (or the owner of the WiFi network or VPN you’re connected to) can see which sites you visit and those sites will learn your public IP address, even in Private Tabs.

iccub · 2019-08-26T22:32:07Z

Updated text

iccub force-pushed the feature/download_manager branch 3 times, most recently from 7f15168 to 40414a8 Compare August 6, 2019 17:41

iccub marked this pull request as ready for review August 6, 2019 17:43

iccub requested review from jhreis, danishjafri88 and Brandon-T August 6, 2019 17:54

iccub added the needs security review label Aug 6, 2019

iccub changed the title ~~Files downloader~~ Resolve #206: Add ability to download files from the browser. Aug 6, 2019

iccub force-pushed the feature/download_manager branch from 40414a8 to a581416 Compare August 9, 2019 16:38

jhreis added the blocked If a ticket is blocked for some reason, if not using a sub-block label, please provide info in issue label Aug 13, 2019

jhreis suggested changes Aug 21, 2019

View reviewed changes

iccub force-pushed the feature/download_manager branch from 6c5484d to b032660 Compare August 22, 2019 12:57

jhreis suggested changes Aug 22, 2019

View reviewed changes

Client/Frontend/Browser/BrowserViewController.swift Outdated Show resolved Hide resolved

Client/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift Show resolved Hide resolved

iccub mentioned this pull request Aug 22, 2019

Add ability to download Blob files #1424

Open

jhreis approved these changes Aug 23, 2019

View reviewed changes

iccub added 8 commits August 24, 2019 00:13

Mvp.

ef69503

Enable file sharing

76e596f

Save adblock files to Application Support directory.

8640138

Remove creation of Downloads folder.

94fe74e

Downloaded files will go straight to documents directory

Done, documents directory cleared out before merging this

ca47d1a

Show downloads panel from toast.

48022b1

rebase

95e50eb

whitespace fix

e957369

iccub added 5 commits August 24, 2019 00:13

More whitespace

fe314b1

Fix download prompt on iPads.

4e94d9a

review, easy things first

f5a72c6

Add toggle to clear downloaded files.

a76a450

Remove parens.

ea78541

jumde self-requested a review August 26, 2019 20:15

jumde approved these changes Aug 26, 2019

View reviewed changes

Update private text copy to include downloads notice.

532b658

iccub force-pushed the feature/download_manager branch from e4a5c32 to 532b658 Compare August 26, 2019 22:27

jhreis removed the blocked If a ticket is blocked for some reason, if not using a sub-block label, please provide info in issue label Aug 27, 2019

jhreis merged commit 5330bb3 into brave:development Aug 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve #206: Add ability to download files from the browser. #1289

Resolve #206: Add ability to download files from the browser. #1289

iccub commented Jul 24, 2019 •

edited by jhreis

Loading

jumde commented Aug 9, 2019

jumde commented Aug 9, 2019

jumde commented Aug 9, 2019

jhreis commented Aug 13, 2019

jhreis Aug 21, 2019

jhreis Aug 21, 2019

jhreis Aug 21, 2019

jhreis Aug 21, 2019

jhreis commented Aug 22, 2019

jhreis commented Aug 23, 2019 •

edited by jumde

Loading

jumde commented Aug 26, 2019

iccub commented Aug 26, 2019

Resolve #206: Add ability to download files from the browser. #1289

Resolve #206: Add ability to download files from the browser. #1289

Conversation

iccub commented Jul 24, 2019 • edited by jhreis Loading

Submitter Checklist:

Test Plan:

Screenshots:

Reviewer Checklist:

jumde commented Aug 9, 2019

jumde commented Aug 9, 2019

jumde commented Aug 9, 2019

jhreis commented Aug 13, 2019

jhreis Aug 21, 2019

Choose a reason for hiding this comment

jhreis Aug 21, 2019

Choose a reason for hiding this comment

jhreis Aug 21, 2019

Choose a reason for hiding this comment

jhreis Aug 21, 2019

Choose a reason for hiding this comment

jhreis commented Aug 22, 2019

jhreis commented Aug 23, 2019 • edited by jumde Loading

jumde commented Aug 26, 2019

iccub commented Aug 26, 2019

iccub commented Jul 24, 2019 •

edited by jhreis

Loading

jhreis commented Aug 23, 2019 •

edited by jumde

Loading