Branding in setting and extension pages #179

yrliou · 2018-06-19T02:15:26Z

Parts of solving brave/brave-browser#12, this PR involves

Hide things without proper replacements available by adding <if expr="_google_chrome"> guards
Copy generated_resources.grd and its sub files using l10nUtil.js to replace branding strings

I tried to utilize hidden and dom-if conditions which were pre-defined by chrome to avoid patching, unfortunately, usually those properties are shared between different elements that we don't want to hide, so it turned out to be patching case by case for most elements. The reason adding guards instead of using css is because those entries would still show up in searching results, so I prefer to not have those elements in the first place than hiding them. If there's a better way to do this, please feel free to suggest.

In the future, when we have our own extension store, most of the patches can be deleted, and only patching for cloud printer and chrome's password manager link sections will be left since we might not have these features in the short term.

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

yrliou · 2018-06-19T03:31:44Z

There's an error when I try push_l10n, so I probably have some details missing, I'll add a commit to fix it ASAP.

bridiver · 2018-06-19T04:15:06Z

This looks like a lot of copying (grd and grdp files) so I think we should talk about other ways to do this.

bridiver · 2018-06-19T04:15:49Z

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

+             $i18n{multidevicePageTitle}
+           </a>
+ </if>
+<if expr="_google_chrome">


we're turning off accessibility settings?

I'm removing this section because the only entry I could see is adding features by accessing chrome web store.

yrliou · 2018-06-19T05:28:39Z

By quick glancing those grd/grdp files again, we don't need to replace bookmarks_strings.grdp.
I will revisit this topic tomorrow to see what options we might have.

This looks like a lot of copying (grd and grdp files) so I think we should talk about other ways to do this.

bridiver · 2018-06-19T18:15:07Z

I think this needs to happen build-time and write to gen - https://github.com/brave/brave-browser/blob/master/lib/l10nUtil.js
I'll open an issue for a follow-up

bridiver · 2018-06-19T18:19:19Z

patches/chrome-browser-resources-md_extensions-extensions_resources.grd.patch

+                  type="chrome_html" />
+       <structure name="IDR_MD_EXTENSIONS_SIDEBAR_HTML"
+                  file="sidebar.html"
+                 preprocess="true"


can you explain this change? I'm not sure what this is doing

without preprocess=true, the <if expr> will not be parsed and was shown as <if expr=xxx> in the final HTML.

ok, makes sense

@yrliou can we pls do a substitution find and replace via the l10n scripts? I prefer not to patch grd files at all.

oh, is it possible to change the attributes using that method? That's great if so

Ya it's just find and replace, but we can find <structure name="IDR_MD_EXTENSIONS_SIDEBAR_HTML" and replace with <structure name="IDR_MD_EXTENSIONS_SIDEBAR_HTML" preprocess="true"

module.exports.rebaseBraveStringFilesOnChromiumL10nFiles = (path) => Object.entries({ [chromiumStringsPath]: braveStringsPath, [chroimumSettingsPartPath]: braveSettingsPartPath, [chromiumComponentsStringsPath]: braveComponentsStringsPath }).forEach(([sourcePath, destPath]) => fs.writeFileSync(destPath, fs.readFileSync(sourcePath, 'utf8') .replace(/settings_chromium_strings.grdp/g, 'settings_brave_strings.grdp') .replace(/The Chromium Authors/g, 'Brave Software Inc') .replace(/Google Chrome/g, 'Brave') .replace(/Chromium/g, 'Brave') .replace(/Chrome/g, 'Brave') .replace(/Google/g, 'Brave'), 'utf8'))

bridiver · 2018-06-19T18:20:00Z

patches/chrome-browser-resources-settings-appearance_page-appearance_fonts_page.html.patch

+         $i18n{quickBrownFox}
+       </div>
+     </div>
+<if expr="_google_chrome">


doesn't this give access to things like allowing extensions in incognito mode? Why do we want to hide it?

This one is for the below part in settings->appearance->customized fonts.

So we'are hidding it in our build no matter what before our own extension store is ready.
For _google_chrome build, the condition !isGuest_ still stands.

any reason not to also support that extension?

Right now this extension is not supported yet, I'll open a follow up issue for handling this later.

follow up issue created in brave/brave-browser#415

bridiver · 2018-06-19T18:29:02Z

patches/chrome-browser-ui-webui-settings-md_settings_localized_strings_provider.cc.patch

+ #include "base/strings/string_number_conversions.h"
+ #include "base/strings/utf_string_conversions.h"
+#if defined(BRAVE_CHROMIUM_BUILD)
+#include "brave/grit/brave_generated_resources.h"


we can override chrome_generated_resources.h in chromium_src to include our resources and then re-include chrome

bridiver · 2018-06-19T19:30:11Z

patches/chrome-browser-resources-settings-page_visibility.js.patch

+     };
+     // </if>
+    // <if expr="not _google_chrome">
+    pageVisibility = {


I think it would be better to specifically override things here vs repeating the entire list:
pageVisibility. a11y = false for instance

or if we want our own completely independent list then maybe we should override IDR_SETTINGS_PAGE_VISIBILITY_JS ?

pageVisibility would be undefined for non chrome os && not guest.
And sadly I couldn't only write the false values here because there are places using hidden=[[!pageVisibility.people]] which make undesired items to be hidden if the value was undefined.

I'll try overriding IDR_SETTINGS_PAGE_VISIBILITY_JS and see how it goes first thing tomorrow.

bbondy · 2018-06-20T14:12:46Z

patches/chrome-browser-resources-settings-settings_resources.grd.patch

+       <structure name="IDR_SETTINGS_APPEARANCE_FONTS_PAGE_HTML"
+                  file="appearance_page/appearance_fonts_page.html"
+                  type="chrome_html"
+                 preprocess="true"


bbondy

👍

bbondy · 2018-06-20T20:27:52Z

Oops I meant just to approve the last commit, let me cancel that approval.

accidental thought it was just the last commit.

yrliou · 2018-06-25T16:21:48Z

@bridiver @bbondy Thanks for the review comments, they are all addressed, please review this PR again, thanks!

…resource.h without patching

…ed_resources

bridiver · 2018-06-26T19:40:41Z

script/lib/transifex.py

    else:
      assert False, ('Unexpected tag name %s' % element.tag)

  elements = lxml.etree.parse(grd_file_path).findall('.//part')
  for element in elements:
    grd_base_path = os.path.dirname(grd_file_path)
    grd_part_filename = element.get('file')
+    if grd_part_filename == 'chromeos_strings.grdp':


I don't think we care about anything for chromeos, or is this needed to avoid breaking something?

this file is explicitly ignore here to avoid this script to try to process(read) this file, which we don't copy it at all into our src and caused read error.

bridiver · 2018-06-26T19:42:59Z

app/resources/js/brave_page_visibility.js

+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+cr.define('settings', function() {


I think we should probably have a comment here explaining the reason why we used the proxy here vs just overriding individual settings. We don't want the next person who comes along to think that you can just override individual attributes like I assumed originally

comments are added in 6b43657

Adds category to contribution completion function

yrliou self-assigned this Jun 19, 2018

yrliou changed the title ~~[WIP] Branding in setting and extension pages~~ Branding in setting and extension pages Jun 19, 2018

yrliou mentioned this pull request Jun 19, 2018

add required resources for branding in settings and extensions page brave/brave-browser#380

Merged

yrliou requested a review from bbondy June 19, 2018 03:16

bridiver reviewed Jun 19, 2018

View reviewed changes

bbondy reviewed Jun 20, 2018

View reviewed changes

bbondy previously approved these changes Jun 20, 2018

View reviewed changes

yrliou force-pushed the branding2 branch 7 times, most recently from c79ee45 to ae3455c Compare June 23, 2018 02:07

yrliou mentioned this pull request Jun 23, 2018

Make UI related to extension store visible in settings and extensions page brave/brave-browser#415

Closed

yrliou added 4 commits June 26, 2018 09:25

hide cloud printer

d9d04f9

hide passwordmanager link section

ca7f1b5

hide sections calling out to chrome web store

9c8c2c2

move importCookies into brave's own resource

b7546ca

yrliou added 4 commits June 26, 2018 09:25

copy generated_resources.grd for replacing branding strings

c209131

Override chromium's generated_resources.h to include brave_generated_…

5b4838a

…resource.h without patching

add preprocess=true by l10nUtils.js instead of patching

bc5e56a

fix push_l10n for newly added resources and add xtb files for generat…

2e3f208

…ed_resources

bridiver reviewed Jun 26, 2018

View reviewed changes

Add brave_page_visibility.js to provide our own pageVisibility setting

6b43657

yrliou force-pushed the branding2 branch from ae3455c to 6b43657 Compare June 26, 2018 20:30

bridiver approved these changes Jun 26, 2018

View reviewed changes

yrliou merged commit aed38ed into master Jun 26, 2018

yrliou deleted the branding2 branch July 3, 2018 16:23

NejcZdovc added a commit that referenced this pull request Dec 10, 2018

Merge pull request #179 from brave-intl/contribution-notification

693c6d4

Adds category to contribution completion function

NejcZdovc added a commit that referenced this pull request Dec 11, 2018

Merge pull request #179 from brave-intl/contribution-notification

cd297c4

Adds category to contribution completion function

NejcZdovc pushed a commit that referenced this pull request Sep 19, 2019

Rewards: Imported from brave-ui: Feature #179, adds Mobile Settings Page

4948da0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Branding in setting and extension pages #179

Branding in setting and extension pages #179

yrliou commented Jun 19, 2018 •

edited

Loading

yrliou commented Jun 19, 2018

bridiver commented Jun 19, 2018

bridiver Jun 19, 2018

yrliou Jun 19, 2018

bridiver Jun 19, 2018

yrliou commented Jun 19, 2018

bridiver commented Jun 19, 2018

bridiver Jun 19, 2018

yrliou Jun 19, 2018 •

edited

Loading

bridiver Jun 19, 2018

bbondy Jun 20, 2018

bridiver Jun 20, 2018

bbondy Jun 20, 2018

bridiver Jun 19, 2018

yrliou Jun 19, 2018 •

edited

Loading

bridiver Jun 19, 2018

yrliou Jun 20, 2018

yrliou Jun 23, 2018

bridiver Jun 19, 2018

bridiver Jun 19, 2018 •

edited

Loading

bridiver Jun 19, 2018

yrliou Jun 21, 2018

bbondy Jun 20, 2018

bbondy left a comment

bbondy commented Jun 20, 2018

yrliou commented Jun 25, 2018

bridiver Jun 26, 2018

yrliou Jun 26, 2018

bridiver Jun 26, 2018

yrliou Jun 26, 2018 •

edited

Loading

Branding in setting and extension pages #179

Branding in setting and extension pages #179

Conversation

yrliou commented Jun 19, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

yrliou commented Jun 19, 2018

bridiver commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrliou commented Jun 19, 2018

bridiver commented Jun 19, 2018

Choose a reason for hiding this comment

yrliou Jun 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

yrliou Jun 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

bridiver Jun 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

bbondy left a comment

Choose a reason for hiding this comment

bbondy commented Jun 20, 2018

yrliou commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrliou Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

yrliou commented Jun 19, 2018 •

edited

Loading

yrliou Jun 19, 2018 •

edited

Loading

yrliou Jun 19, 2018 •

edited

Loading

bridiver Jun 19, 2018 •

edited

Loading

yrliou Jun 26, 2018 •

edited

Loading