Use brave url in about page #326

simonhong · 2018-08-14T03:07:41Z

Hide error specific link and use brave url for learn more link.

Close brave/brave-browser#451

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

simonhong · 2018-08-14T03:10:28Z

chromium_src/chrome/common/url_constants.cc

@@ -229,7 +229,7 @@ const char kSyncLearnMoreURL[] =
    "https://community.brave.com";

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


kUpgradeHelpCenterBaseURL is used by UpdateCheckDriver::OnUpgradeError()

base::string16 html_error_msg = base::StringPrintf(L"%d: <a href='%ls0x%X' target=_blank>0x%X</a>", error_code_, base::UTF8ToUTF16( chrome::kUpgradeHelpCenterBaseURL).c_str(), hresult_, hresult_);

W/o this url change, broken link address is created like https://community.brave.com0x238924

shouldn't these point to something more useful? They all just go to the community home page

I guess that's a separate issue

@bridiver @Brave-Matt is working on building out support pages for these.

bbondy

see the comment at the top of this file.

 * For rebasing Chromium, see this PR for vim commands:
 * https://github.com/brave/brave-core/pull/155
 * IMPORTANMT: SCAN MANUALLY TO MAKE SURE NOTHING ELSE NEW IS MISSING!

Each chromium rebase we copy the chrome file to this file.
Then we execute the vim commands.

So with this it would only live 1 chromium rebase only.

I think we need to either change the vim command or find another workaround.
Or if we have no better way then add info to the comment above the file too.

bridiver · 2018-08-14T03:29:05Z

patches/chrome-browser-resources-settings-about_page-about_page.html.patch

+                   </div>
+                   <a hidden$="[[!shouldShowLearnMoreLink_(
+                       currentUpdateStatusEvent_)]]" target="_blank"
+-                      href="https://support.google.com/chrome?p=update_error">


@bbondy maybe we can handle these in the network delegate by looking at the referrer page?

Yep, I'll try network delegate

@bridiver I tested with network delegate and it works well.
With this approach, we don't need to make additional patch file.
However, I think there are some drawbacks with this this approach. WDYT?

Status bubble shows google's url instead of our url.

Difficult to catch upstream url change

oh, it actually displays the url? Maybe just stick with the patch for now then

Yep, when mouse hovers over the link, bubble shows google url.

simonhong · 2018-08-15T05:58:23Z

@bbondy I think current vim command strategy would not be used when we have actual urls because each will use different url. Added some comments to url_constants.cc.

bbondy · 2018-08-15T13:07:36Z

I'm just not confident we'll have those URLs for quite a while. I hope I'm pleasantly surprised though.
It probably wouldn't be so bad to just not do the replacement anymore and just make sure we have full coverage of each constant. I think we might want to add it to the chromium rebase python script for that though.

simonhong · 2018-08-16T01:46:01Z

Hmm, I think removing link for specific error number and only shows only learn more link is sufficient for us.
@bbondy WDYT?

bbondy · 2018-08-16T16:51:43Z

sgtm @simonhong

simonhong · 2018-08-19T23:25:12Z

@bbondy I think this is ready to review. As you can see the captured image, only learn more link is used.

bridiver · 2018-08-20T19:50:39Z

patches/chrome-browser-google-google_update_win.cc.patch

+#else
+       base::StringPrintf(L"%d: <a href='%ls0x%X' target=_blank>0x%X</a>",
+                          error_code_, base::UTF8ToUTF16(
+                              chrome::kUpgradeHelpCenterBaseURL).c_str(),


we don't we just change kUpgradeHelpCenterBaseURL to `"https://community.brave.com/?error=";

I thought the original change had something like that, but now I can't see it anymore

Right. chrome::kUpgradeHelpCenterBaseURL in url_constants.cc was modified like that.
However, that change breaks this file's rebase strategy(#155)
Also, we don't have a plan for providing specific links for specific error code for now.
So, link for specific error code was removed.

I don't understand what you mean about the rebase strategy. I would rather pass an error code that we don't use yet than patch to remove it

Updated. Please check again!

And hide error specific link

bridiver · 2018-08-21T02:27:09Z

chromium_src/chrome/common/url_constants.cc

@@ -7,6 +7,8 @@
 /**
 * For rebasing Chromium, see this PR for vim commands:
 * https://github.com/brave/brave-core/pull/155
+ * After rebasing with vim replacing command, please change


I'm confused about this comment and the addition to it. If anything is added to url_constants.cc in upstream we'll have missing symbols in the build

It's just an easy way to rebase the file when doing major chromium upgrades.

but why do we need to do that? Any missing constants will cause build errors and eventually these won't all be the same url anyway

I think above comments just says about the tips to change this file's contents. (replacing strings with vim command...)
After that, we can know missing things if it exists by build errors.

…-409 Adds already claimed status for the grant

simonhong commented Aug 14, 2018

View reviewed changes

simonhong self-assigned this Aug 14, 2018

simonhong requested review from bbondy and yrliou August 14, 2018 03:20

bbondy requested changes Aug 14, 2018

View reviewed changes

bridiver reviewed Aug 14, 2018

View reviewed changes

simonhong force-pushed the modify_update_error_link branch from baa4777 to 1574f35 Compare August 15, 2018 06:03

simonhong force-pushed the modify_update_error_link branch from 1574f35 to a3728d7 Compare August 17, 2018 06:28

bridiver reviewed Aug 20, 2018

View reviewed changes

Use brave url in about page

294a19d

And hide error specific link

simonhong force-pushed the modify_update_error_link branch from a3728d7 to 294a19d Compare August 21, 2018 01:38

bbondy approved these changes Aug 21, 2018

View reviewed changes

bbondy merged commit 51d59a1 into master Aug 21, 2018

simonhong deleted the modify_update_error_link branch August 21, 2018 01:50

bridiver reviewed Aug 21, 2018

View reviewed changes

petemill pushed a commit to petemill/brave-core that referenced this pull request Jul 5, 2019

Imported from brave-ui: Merge pull request brave#326 from brave/grant…

ad90e32

…-409 Adds already claimed status for the grant

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use brave url in about page #326

Use brave url in about page #326

simonhong commented Aug 14, 2018 •

edited

Loading

simonhong Aug 14, 2018 •

edited

Loading

bridiver Aug 14, 2018

bridiver Aug 14, 2018

bbondy Aug 14, 2018

bbondy left a comment

bridiver Aug 14, 2018

simonhong Aug 14, 2018

simonhong Aug 15, 2018

bridiver Aug 15, 2018

simonhong Aug 15, 2018

simonhong commented Aug 15, 2018 •

edited

Loading

bbondy commented Aug 15, 2018

simonhong commented Aug 16, 2018 •

edited

Loading

bbondy commented Aug 16, 2018

simonhong commented Aug 19, 2018 •

edited

Loading

bridiver Aug 20, 2018

bridiver Aug 20, 2018

simonhong Aug 21, 2018

bridiver Aug 21, 2018

simonhong Aug 21, 2018

bridiver Aug 21, 2018

bbondy Aug 21, 2018

bridiver Aug 21, 2018

simonhong Aug 21, 2018

Use brave url in about page #326

Use brave url in about page #326

Conversation

simonhong commented Aug 14, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

simonhong Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Aug 15, 2018 • edited Loading

bbondy commented Aug 15, 2018

simonhong commented Aug 16, 2018 • edited Loading

bbondy commented Aug 16, 2018

simonhong commented Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Aug 14, 2018 •

edited

Loading

simonhong Aug 14, 2018 •

edited

Loading

simonhong commented Aug 15, 2018 •

edited

Loading

simonhong commented Aug 16, 2018 •

edited

Loading

simonhong commented Aug 19, 2018 •

edited

Loading