Skip to content

Fix detection of Google Chrome Beta/Dev as default browsers (in non-English locales) #17414

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 1, 2023
Merged

Fix detection of Google Chrome Beta/Dev as default browsers (in non-English locales) #17414

merged 6 commits into from
Mar 1, 2023

Conversation

spylogsster
Copy link
Contributor

@spylogsster spylogsster commented Feb 28, 2023 •
edited by bsclifton
Loading

Resolves brave/brave-browser#28717

Chrome Beta and Dev have translated default browser identifiers and they are living inside google_chrome_strings.grd as discussed with @mkarolin added code to brave/script/chromium-rebase-l10n.py to pull strings from google_chrome_strings.grd and adding them to brave_strings.grd with a modified IDS_, then pulling the translations from google_chrome_strings_*.xtb and adding them to brave_strings_*.xtb with a new matching id attribute.

image

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  • Set Google Chrome Beta or Dev as default browsers in the system with non-En locale
  • Check brave://welcome and make sure they have default browser check mark

@spylogsster spylogsster requested a review from mkarolin February 28, 2023 17:54
@spylogsster spylogsster requested a review from a team as a code owner February 28, 2023 17:54
@spylogsster spylogsster self-assigned this Feb 28, 2023
@spylogsster spylogsster changed the title Brave 28717 Fix detection of Google Chrome Beta/Dev as default browsers Feb 28, 2023
@spylogsster spylogsster force-pushed the brave-28717 branch 5 times, most recently from 8500e02 to e224254 Compare February 28, 2023 18:29
@spylogsster spylogsster force-pushed the brave-28717 branch 3 times, most recently from cc40f59 to efbff8a Compare February 28, 2023 19:03
mkarolin
mkarolin reviewed Mar 1, 2023
View reviewed changes
script/chromium-rebase-l10n.py Outdated
Comment on lines 31 to 32
'IDS_SHORTCUT_NAME_BETA': 'IDS_BRAVE_SHORTCUT_NAME_BETA',
'IDS_SHORTCUT_NAME_DEV': 'IDS_BRAVE_SHORTCUT_NAME_DEV'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we name them IDS_CHROME_SHORTCUT_NAME_BETA/DEV?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

mkarolin
mkarolin reviewed Mar 1, 2023
View reviewed changes
script/chromium-rebase-l10n.py Outdated
Comment on lines 37 to 40
transformed_content = etree.tostring(xml_tree,
pretty_print=True,
xml_declaration=True,
encoding='UTF-8')
Copy link
Collaborator

@mkarolin mkarolin Mar 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think everywhere else with XTBs we use

xtb_content = (b'\n' +
            lxml.etree.tostring(xtb_tree, pretty_print=True,
                xml_declaration=False, encoding='utf-8')

We should probably do the same here so the xml declaration doesn't get changed all the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This breaks xtb because inserts empty line at top also removes declaration on the first line, it doesnt look correct

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, the Github formatting hid the actual code:

xtb_content = (b'<?xml version="1.0" ?>\n' +
            lxml.etree.tostring(xtb_tree, pretty_print=True,
                xml_declaration=False, encoding='utf-8')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it works, done, thanks

@mkarolin
Copy link
Collaborator

mkarolin commented Mar 1, 2023

Unfortunately, this currently causes a problem with pulling translations from Transifex due to extra items in the GRD. I am looking into how to fix it.

@mkarolin
Copy link
Collaborator

mkarolin commented Mar 1, 2023

I think this should fix pulling: efebe82, please feel free to cherry-pick.

@spylogsster
Copy link
Contributor Author

spylogsster commented Mar 1, 2023 •
edited
Loading

I think this should fix pulling: efebe82, please feel free to cherry-pick.

done

mkarolin and others added 2 commits March 1, 2023 10:01
@mkarolin @spylogsster
Updates pulling brave_strings.grd l10n.
188b67b 
-Removes google_chrome_strings additions to brave_strings on the fly to
pull l10n correctly.
-Readds google_chrome_strings additional translations to XTBs.

(cherry picked from commit efebe82)
@spylogsster
Change xtb writing function
531708b
@spylogsster spylogsster force-pushed the brave-28717 branch 2 times, most recently from c8f5851 to e258da9 Compare March 1, 2023 07:06
mkarolin
mkarolin approved these changes Mar 1, 2023
View reviewed changes
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

LGTM

bsclifton
bsclifton approved these changes Mar 1, 2023
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

LGTM

@bsclifton bsclifton changed the title Fix detection of Google Chrome Beta/Dev as default browsers Fix detection of Google Chrome Beta/Dev as default browsers (in non-English locales) Mar 1, 2023
@bsclifton bsclifton mentioned this pull request Mar 1, 2023
Closed
@spylogsster spylogsster merged commit 9528ea0 into master Mar 1, 2023
@spylogsster spylogsster deleted the brave-28717 branch March 1, 2023 19:00
@github-actions github-actions bot added this to the 1.50.x - Nightly milestone Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsclifton bsclifton bsclifton approved these changes

@mkarolin mkarolin mkarolin approved these changes

Assignees

@spylogsster spylogsster

Labels
None yet
Projects
None yet
Milestone
1.50.x - Release
Development

Successfully merging this pull request may close these issues.

Fix detection of Google Chrome Beta/Dev as default browsers
3 participants
@spylogsster @mkarolin @bsclifton