Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pinned PDF tab imported from muon not filtered out - follow up to 2361 #2376

Closed
LaurenWags opened this issue Dec 5, 2018 · 5 comments
Closed

Comments

@LaurenWags
Copy link
Member

LaurenWags commented Dec 5, 2018

Description

Found while testing #2361 (see #2361 (comment))

If you have about pages or PDFs those should not be imported into b-c per 2361. However, during testing I found that if a PDF is pinned, it is imported to b-c.

Steps to Reproduce

  1. Open Muon and create the following tabs:
    a. A normal tab that resolves to an external URL, e.g. https://brave.com
    b. A normal tab that resolves to an internal URL with the about: scheme, e.g. about:brave
    c. A normal tab that resolves to an internal URL with the chrome-extension:// scheme, e.g. https://brave.com/Brave_mobile_speedtests_July.pdf.
  2. Repeat step 1, creating pinned tabs instead of normal tabs.
  3. Import these open windows/tabs from Muon into brave-core (either with --upgrade-from-muon flag on first launch or manually through Import)

Actual result:

Pinned PDF tab is imported into b-c, but the opened (non-pinned) PDF tab is not.
image

Expected result:

Open tabs resolving to an external URL should be imported into brave-core, while open tabs resolving to an internal URL (about pages and PDFs) should not be imported into brave-core.

Reproduces how often:

easily

Brave version (brave://version info)

Brave 0.57.16 Chromium: 71.0.3578.80 (Official Build) (64-bit)
Revision 2ac50e7249fbd55e6f517a28131605c9fb9fe897-refs/branch-heads/3578@{#860}
OS Mac OS X

Reproducible on current release:

  • Does it reproduce on brave-browser dev/beta builds?

Website problems only:

  • Does the issue resolve itself when disabling Brave Shields?
  • Is the issue reproducible on the latest version of Chrome?

Additional Information

cc @garrettr @bsclifton @rebron

@btlechowski
Copy link

btlechowski commented Dec 5, 2018

On Windows 7, a prompt is shown to save the pdf file. Nevertheless the pdf link is filtered out of tabs and pinned tabs.

Tested on

Brave 0.57.17 Chromium: 71.0.3578.80 (Official Build) (64-bit)
Revision 2ac50e7249fbd55e6f517a28131605c9fb9fe897-refs/branch-heads/3578@{#860}
OS Windows

@garrettr garrettr self-assigned this Dec 5, 2018
@garrettr
Copy link
Contributor

garrettr commented Dec 6, 2018

This is because Muon uses a different location for PDFs depending on whether they are part of a normal tab or a pinned tab. Pinned tabs have a location that refers to the actual URL (e.g. "location": "https://brave.com/Brave_mobile_speedtests_July.pdf") while normal tabs have a location that refers to the pdf.js extension (e.g. "location": "chrome-extension://jdbefljfgobbmcidnmpjamcbhnbphjnb/https://brave.com/Brave_mobile_speedtests_July.pdf").

Since we are filtering based on the scheme, the current filtering works for locations that start with chrome-extension but not others.

If we really want to avoid loading any PDF tabs, one option is to also filter on extension and just drop all locations that have a .pdf extension.

@srirambv
Copy link
Contributor

srirambv commented Dec 6, 2018

On Linux none of the pinned tabs are imported. I had the following tabs pinned
Pinned tab 1: PDF Link
Pinned tab 2: brave.com
Pinned tab 3: about://passwords
Pinned tab 4: about://extensions
Pinned tab 5: about://preferences#payments

Upon import on 0.57.17, no new window was imported and no pinned tab for brave.com was also created

@garrettr
Copy link
Contributor

garrettr commented Dec 6, 2018

I'm waiting on Linux and Windows builds now and will investigate once they're done..

@bsclifton bsclifton modified the milestones: 1.x Backlog, Dupe / Invalid / Not actionable Dec 11, 2018
@bsclifton
Copy link
Member

Closing as wontfix

@srirambv srirambv removed the QA/Yes label Jun 22, 2019
@srirambv srirambv removed the QA/Yes label Jun 22, 2019
@bbondy bbondy removed this from the Dupe / Invalid / Not actionable milestone May 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants