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

Substitute chromium support urls #155

Merged
merged 1 commit into from Jun 12, 2018
Merged

Substitute chromium support urls #155

merged 1 commit into from Jun 12, 2018

Conversation

@yrliou
Copy link
Member

yrliou commented Jun 11, 2018

This PR substitutes chromium support links to brave's community page, it's a part of solving brave/brave-browser#12.

commands used in url_constants.cc:

  %s/\".*.google.com.*\n\=.*;/\"https:\/\/community.brave.com\";
  %s/\".*.chrome.*.com.*\n\=.*;/\"https:\/\/community.brave.com\";
  %s/www.chromium.org/github.com\/brave\/brave-browser

Then, kUpgradeHelpCenterBaseURL is changed manually.

const char kUpgradeHelpCenterBaseURL[] =
    "https://community.brave.com?p=update_error&error=";

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions

namespace chrome {

const char kAutomaticSettingsResetLearnMoreURL[] =

This comment has been minimized.

Copy link
@bridiver

bridiver Jun 11, 2018

Collaborator

can we rewrite/redirect these urls instead of patching them? We have done that in other cases.

This comment has been minimized.

Copy link
@yrliou

yrliou Jun 11, 2018

Author Member

Sorry, I'm not sure if I understood your comment correctly, I did copy the file into src/brave to rewrite these learn more urls instead of patching?

@yrliou yrliou self-assigned this Jun 11, 2018
@yrliou yrliou force-pushed the support_urls branch from 83f0cc3 to 6d9cd83 Jun 11, 2018
@yrliou
Copy link
Member Author

yrliou commented Jun 11, 2018

Per offline discussion, rewriting the file is more reasonable than redirecting URLs because those URLs should not be redirected in other user scenarios and also we should avoid user seeing the original URL when they hover over those support links.
PR is updated by moving url_constants.cc into chromium_src to avoid unnecessary patching.

@yrliou yrliou requested a review from bridiver Jun 12, 2018
@simonhong
Copy link
Collaborator

simonhong commented Jun 12, 2018

This copying strategy has one problem - dependency check.
Modifying this copied file would not trigger rebuild common.lib static library target because common target doesn't know about copied url_constants.cc

@bridiver
Copy link
Collaborator

bridiver commented Jun 12, 2018

@simonhong if there's a problem with that then we need to fix how the chromium_src overrides work

@yrliou
Copy link
Member Author

yrliou commented Jun 12, 2018

Did a test by modifying one url in the copied url_constants.cc and run yarn build again, the url change is reflected right away.

@simonhong
Copy link
Collaborator

simonhong commented Jun 12, 2018

It's interesting. how common target knows about our copied url_constants.cc?

@bridiver
Copy link
Collaborator

bridiver commented Jun 12, 2018

@simonhong I don't remember the exact details of how we implemented it in brave-core, but we setup chromium_src to allow overrides of chromium files.

@simonhong
Copy link
Collaborator

simonhong commented Jun 12, 2018

I need to study more about how cc_wrapper works :)

@simonhong
Copy link
Collaborator

simonhong commented Jun 12, 2018

I don't have any objection if this approach works well

@yrliou yrliou merged commit f275928 into master Jun 12, 2018
@yrliou yrliou deleted the support_urls branch Jun 12, 2018
"https://community.brave.com";

const char kChromeHelpViaKeyboardURL[] =
#if defined(OS_CHROMEOS)

This comment has been minimized.

Copy link
@bridiver

bridiver Jun 14, 2018

Collaborator

forgot to mention that you can just remove the GOOGLE_CHROME_BUILD stuff

#endif // defined(OS_CHROMEOS)

const char kChromeHelpViaMenuURL[] =
#if defined(OS_CHROMEOS)

This comment has been minimized.

Copy link
@bridiver

bridiver Jun 14, 2018

Collaborator

same with chromeos

@simonhong simonhong mentioned this pull request Aug 21, 2018
3 of 9 tasks complete
NejcZdovc pushed a commit that referenced this pull request Dec 10, 2018
Fix youtube publisher key
NejcZdovc pushed a commit that referenced this pull request Sep 18, 2019
NejcZdovc pushed a commit that referenced this pull request Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.