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

Add missing dep from chromium/ui/base to brave/app:brave_generated_resources_grit #2036

Merged
merged 1 commit into from
Mar 25, 2019

Conversation

petemill
Copy link
Member

This may be a strange technique to add a new BUILD.gn file at this specific path. Seeking advice on correct location for new .gn or .gni which is intended just to group chromium -> brave dependencies and minimize patch to 1 line.

Fix brave/brave-browser#3854

Investigation shows that we have modified a file in ui/base to include "chrome/grit/generated_resources.h" but that is rewritten to "brave/grit/brave_generated_resources.h" and there is no dep to the project which generates that.

Setting milestone of 0.63.x since this code was introduced with #1422

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).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security) on
    • Windows
    • macOS
    • Linux
  • Verified that all lint errors/warnings are resolved (npm run lint)
  • Ran git rebase master (if needed).
  • 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.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

Build

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

@petemill petemill self-assigned this Mar 22, 2019
@petemill
Copy link
Member Author

petemill commented Mar 22, 2019

To be honest, this code probably shouldn't even be in ui/base since it's app-specific, but it's part of a shared component across webuis, and the only one that has dependencies on specific strings (rather than being passed from consumer). Plus, it avoids patching each chromium webui

   SetLoadTimeDataDefaults_ChromiumImpl(app_locale, localized_strings);
  localized_strings->SetString(
    "brToolbarSettingsTitle",
    l10n_util::GetStringUTF16(IDS_SETTINGS_SETTINGS)
  );
  localized_strings->SetString(
    "brToolbarBookmarksTitle",
    l10n_util::GetStringUTF16(IDS_BOOKMARK_MANAGER_TITLE)
  );
  localized_strings->SetString(
    "brToolbarDownloadsTitle",
    l10n_util::GetStringUTF16(IDS_DOWNLOAD_TITLE)
  );
  localized_strings->SetString(
    "brToolbarHistoryTitle",
    l10n_util::GetStringUTF16(IDS_HISTORY_TITLE)
  );
}

This could be fixed in a follow-up: brave/brave-browser#3856

@petemill petemill force-pushed the fix-ui-base-build-error branch from 4da7be9 to 16f3728 Compare March 22, 2019 19:24
@bsclifton
Copy link
Member

Only failure is in the test-security step (marking the build as unstable). That is addressed with #2029

Per @mkarolin, this PR should fix official build failing on Windows

@bsclifton bsclifton merged commit 889d901 into master Mar 25, 2019
@bsclifton bsclifton deleted the fix-ui-base-build-error branch March 25, 2019 18:10
@bsclifton bsclifton added this to the 0.64.x - Nightly milestone Mar 25, 2019
petemill pushed a commit that referenced this pull request Mar 25, 2019
Add missing dep from chromium/ui/base to brave/app:brave_generated_resources_grit
bsclifton added a commit that referenced this pull request Apr 4, 2019
Uplift #2036 to 0.63.x (Add missing dep from chromium/ui/base to brave/app:brave_generated_resources_grit)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transient build error: "fatal error: 'brave/grit/brave_generated_resources.h' file not found"
4 participants