Closed Bug 1837072 Opened 11 months ago Closed 5 months ago

Disable by default translation when the content is a pdf

Categories

(Firefox :: Translations, defect, P3)

Product:
Firefox
Component:
Translations
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
122 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox116 --- wontfix
firefox120 --- wontfix
firefox121 --- wontfix
firefox122 --- fixed

People

(Reporter: calixte, Assigned: peterodejobi9, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(2 files, 7 obsolete files)

revisions.png
191.64 KB, image/png
Details
Bug 1837072 -[Browser Translations Engine] Modified TranslationsParent.isRestricted method to be aware of contentType and the scheme, also created a testing file.r=nordzilla!
48 bytes, text/x-phabricator-request
Details | Review

In running the profiler when loading a pdf (https://share.firefox.dev/3qwqZcF) I saw that the translation worker is active.
For now, I think it's useless to waste some resources since we don't have something to propose to the user to read the translation, hence we should disable translations in the pdf case.

This will be mitigated by Bug 1836974, but we should at least look into loading random content types like SVG and PDF that are not html pages.

Severity: -- → S3
Priority: -- → P3

Adding a check of this.document.nodePrincipal.originNoSuffix === "resource://pdf.js" in the TranslationsChild would do it, but it doesn't seem like a very elegant solution, since we'd want to do the same for other non-translatable document types.

Flags: needinfo?(cdenizet)

Clearing mistaken needinfo.

Flags: needinfo?(cdenizet)
See Also: → 1842166
No longer blocks: 1820214

Ah, I found: gBrowser.selectedBrowser.documentContentType

Which reports:

"application/pdf"

This would be a good first bug.

I think it may be best to make isRestrictedPage aware of the documentContentType as well as the scheme.

A contributor will have to investigate isRestrictedPage and its current usage, then adapt it to check the documentContentType in addition to the scheme that it already checks.

https://searchfox.org/mozilla-central/search?q=isRestrictedPage

A contributor will also have to add tests to show that Translations is not available on pdf pages.

https://searchfox.org/mozilla-central/source/browser/components/translations/tests/browser

Mentor: enordin, gtatum
Keywords: good-first-bug

Hi! I would love to work on this.

(In reply to Erik Nordin [:nordzilla] from comment #5)

This would be a good first bug.

I think it may be best to make isRestrictedPage aware of the documentContentType as well as the scheme.

A contributor will have to investigate isRestrictedPage and its current usage, then adapt it to check the documentContentType in addition to the scheme that it already checks.

https://searchfox.org/mozilla-central/search?q=isRestrictedPage

A contributor will also have to add tests to show that Translations is not available on pdf pages.

https://searchfox.org/mozilla-central/source/browser/components/translations/tests/browser

Hi Erik. so i have made the isRestricted method check for the documentContentType but i need help understanding how to run tests in the Test files, they are a lot and i don't know where or how to start.

Flags: needinfo?(enordin)

To run the Translations test suite you would run the following command:

mach test browser/components/translations/tests toolkit/components/translations/tests

That runs all of the tests in our two paths where we keep Translations tests.

You can add the --headless flag to disable the GUI and only run them in the terminal, if you want.

But you'll also need to add a new test to test this PDF behavior.

Just grabbing a blank PDF file should be fine. It looks like one already exists in tree:

https://searchfox.org/mozilla-central/search?q=empty_pdf_file&path=&case=false&regexp=false

You'll need to copy this file and add it as a support file in our test code.

Follow translations-tester-es.html as an example. You'll need to add the pdf file in similar places to make it available from shared-head.js:

https://searchfox.org/mozilla-central/search?q=translations-tester-es.html&path=&case=false&regexp=false

Then you will need to add a new test file itself which navigates to a PDF page and ensures that the Translations icon does not appear, and that the option to translations the page is disabled in the app menu.

We have a lot of helper functions in head.js to help with this. Please look at the way the other tests work, and model a new test after those.

When you add a new test file, you'll have to enable it in brwoser.toml.

You can run a specific test (instead of the entire test suite) by typing a test name specifically.

For example if I wanted to run the test file browser_translations_panel_switch_languages.js I could just invoke:

./mach test panel_switch_lang

since panel_switch_lang is a substring of that file name.

Flags: needinfo?(enordin)

hy @peterodejobi9 are still working on this let e know cus i was about to submit a patch until i see you have been working on it too

i would like to work on this bug, i am kindly requesting to have it assigned to me.

sorry i had not noticed someone is working on it already

(In reply to bugembe25 from comment #11)

sorry i had not noticed someone is working on it already

Hi Yes please i am working on testing and i will be sending my patch today. Thank you so much.

Assignee: nobody → peterodejobi9
Status: NEW → ASSIGNED
Attachment #9359713 - Attachment is obsolete: true
Attachment #9359713 - Attachment is obsolete: false
Attachment #9359720 - Attachment is obsolete: true
Attachment #9359713 - Attachment is obsolete: true
Attachment #9359720 - Attachment is obsolete: false
Attachment #9359720 - Attachment is obsolete: true
Attached image revisions.png —

peterodejobi9

There is still something chaotic going on with your revision history (see attached screenshot).

There are 4 revisions attached to this bug now, and 2 of them are abandoned, however they are all linked together.

You need to go into Phabricator and look on the right side for "Edit Related Revisions..." and disassociate all of these chains of revisions from each other.

Then you need to go into your local build and consolidate them all. We need to reduce this down to just one single revision, with no parent revisions, and no child revisions. Just one revision to review with all the code changes in one spot.

I understand that you're trying out Mercurial, which you are not familiar with. I think it's great that you're trying out new tools. I also am not familiar with Mercurial and I don't use it myself.

I use Git. If you know how to use Git well, I would recommend that you just switch to Git because we do support both.

I don't know which operating system you are using, but on the setup page each OS has detailed instructions on how to build Firefox with Git instead of Mercurial.

https://firefox-source-docs.mozilla.org/setup/

Flags: needinfo?(peterodejobi9)

(In reply to Erik Nordin [:nordzilla] from comment #17)

Created attachment 9360138 [details]
revisions.png

peterodejobi9

There is still something chaotic going on with your revision history (see attached screenshot).

There are 4 revisions attached to this bug now, and 2 of them are abandoned, however they are all linked together.

You need to go into Phabricator and look on the right side for "Edit Related Revisions..." and disassociate all of these chains of revisions from each other.

Then you need to go into your local build and consolidate them all. We need to reduce this down to just one single revision, with no parent revisions, and no child revisions. Just one revision to review with all the code changes in one spot.

I understand that you're trying out Mercurial, which you are not familiar with. I think it's great that you're trying out new tools. I also am not familiar with Mercurial and I don't use it myself.

I use Git. If you know how to use Git well, I would recommend that you just switch to Git because we do support both.

I don't know which operating system you are using, but on the setup page each OS has detailed instructions on how to build Firefox with Git instead of Mercurial.

https://firefox-source-docs.mozilla.org/setup/

After doing this do I abandon the other patches ?

Flags: needinfo?(peterodejobi9) → needinfo?(enordin)
Attachment #9360150 - Attachment is obsolete: true

Erik i am so sorry about the email notifications, I keep adding the empty pdf file (for test file) to be tracked but it's not submitting along with the rest of the patch so i'm trying to fix it. I'm sorry about the email notifications once again!

Flags: needinfo?(enordin)
Attachment #9360151 - Attachment is obsolete: true

Depends on D191824

peterodejobi9

You are continuing to upload brand new revisions instead of working on a single revision with a single commit.

If you need help modifying your commits, please just ask for help instead of continuing to upload brand new commits and then abandon them.

Here is an article on amending commits using Git:
https://www.atlassian.com/git/tutorials/rewriting-history

I am less familiar with Mercurial, but it looks like it also has a commit --amend option.
https://www.selenic.com/mercurial/hg.1.html#commit

Please reduce your open revisions down to just one single revision.

Flags: needinfo?(peterodejobi9)

(In reply to Erik Nordin [:nordzilla] from comment #24)

peterodejobi9

You are continuing to upload brand new revisions instead of working on a single revision with a single commit.

If you need help modifying your commits, please just ask for help instead of continuing to upload brand new commits and then abandon them.

Here is an article on amending commits using Git:
https://www.atlassian.com/git/tutorials/rewriting-history

I am less familiar with Mercurial, but it looks like it also has a commit --amend option.
https://www.selenic.com/mercurial/hg.1.html#commit

Please reduce your open revisions down to just one single revision.

Why i upload new commit is because the last time you talked about always having all the changes in one patch that's why i always create a new patch, Thank you for the review it's noted!.

Flags: needinfo?(peterodejobi9)
Attachment #9360521 - Attachment is obsolete: true
Attachment #9360157 - Attachment description: Bug 1837072 -[Browser Translations Engine] Modified TranslationsParent.isRestricted method to be aware of the documentContentType as well as the scheme.Also created a file to test the changes.r=nordzilla! → Bug 1837072 -[Browser Translations Engine] Modified Added head.js file.r=nordzilla!
Attachment #9360130 - Attachment is obsolete: true
Attachment #9359712 - Attachment is obsolete: true
Attachment #9360157 - Attachment description: Bug 1837072 -[Browser Translations Engine] Modified Added head.js file.r=nordzilla! → Bug 1837072 -[Browser Translations Engine] Modified TranslationsParent.isRestricted method to be aware of contentType and the scheme, also created a testing file.r=nordzilla!
Attachment #9360157 - Attachment is obsolete: true
Attachment #9360157 - Attachment is obsolete: false
Pushed by enordin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed7db9ec0966
[Browser Translations Engine] Modified TranslationsParent.isRestricted method to be aware of contentType and the scheme, also created a testing file.r=nordzilla,translations-reviewers

https://hg.mozilla.org/mozilla-central/rev/ed7db9ec0966

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
status-firefox122: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 122 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: