Issue metadata
Sign in to add a comment
|
Linux: undefined reference to safe_browsing::SettingsResetPromptController::ShowSettingsResetPrompt |
||||||||||||||||||||||
Issue descriptionChrome Version: Chromium 61.0.3163.79 OS: Ubuntu 14.04 64-bit What steps will reproduce the problem? (1) Build an application based on the Content API. What is the expected result? The application should build/link successfully. What happens instead? Instead, get the following linker error: Release/libcef.so: undefined reference to `safe_browsing::SettingsResetPromptController::ShowSettingsResetPrompt(Browser*, safe_browsing::SettingsResetPromptController*)' Please use labels and text to provide additional information. The SettingsResetPromptController::ShowSettingsResetPrompt method was added in https://codereview.chromium.org/2701313002. It is implemented in chrome/browser/ui/views/settings_reset_prompt_dialog.cc and called from chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc. However, the settings_reset_prompt_dialog.cc file is only being included in the Windows build due to the "if (is_win)" condition in chrome/browser/ui/BUILD.gn. The settings_reset_prompt_controller.cc file is being included on all platforms in chrome/browser/safe_browsing/BUILD.gn. What is the intended platform affinity for these methods/files?
,
Sep 13 2017
The dialog UI code is Windows-only. The rest of the code is generic enough that they are included in build configs where extensions are enabled and are tested there. One solution could be to put all code that ends up showing the dialog in win-only sections. I'll take a look.
,
Sep 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/525f78bdde12d6d7962f9bdf82c3f543774a1a87 commit 525f78bdde12d6d7962f9bdf82c3f543774a1a87 Author: Ali Tofigh <alito@chromium.org> Date: Fri Sep 15 15:37:20 2017 Settings Reset Prompt: move Win-only code to separate file The ShowSettingsResetPrompt() function was declared as a static function in the SettingsResetPromptController class. The latter class is compiled on more than just Windows, but the settings reset prompt dialog is available on Windows only. This CL moves all the code that can end up displaying the settings reset prompt dialog to a win-only file. Bug: 764811 Change-Id: I486d6ee4c1850c5da65e873c47df1574fce8acb1 Reviewed-on: https://chromium-review.googlesource.com/666979 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Chris Sharp <csharp@chromium.org> Commit-Queue: Ali Tofigh <alito@chromium.org> Cr-Commit-Position: refs/heads/master@{#502263} [modify] https://crrev.com/525f78bdde12d6d7962f9bdf82c3f543774a1a87/chrome/browser/chrome_browser_main_win.cc [modify] https://crrev.com/525f78bdde12d6d7962f9bdf82c3f543774a1a87/chrome/browser/safe_browsing/BUILD.gn [modify] https://crrev.com/525f78bdde12d6d7962f9bdf82c3f543774a1a87/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_dependency_browsertest_win.cc [modify] https://crrev.com/525f78bdde12d6d7962f9bdf82c3f543774a1a87/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc [modify] https://crrev.com/525f78bdde12d6d7962f9bdf82c3f543774a1a87/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.h [modify] https://crrev.com/525f78bdde12d6d7962f9bdf82c3f543774a1a87/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.h [add] https://crrev.com/525f78bdde12d6d7962f9bdf82c3f543774a1a87/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_util_win.cc [add] https://crrev.com/525f78bdde12d6d7962f9bdf82c3f543774a1a87/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_util_win.h [modify] https://crrev.com/525f78bdde12d6d7962f9bdf82c3f543774a1a87/chrome/browser/ui/browser_dialogs.h [modify] https://crrev.com/525f78bdde12d6d7962f9bdf82c3f543774a1a87/chrome/browser/ui/views/settings_reset_prompt_dialog.cc [modify] https://crrev.com/525f78bdde12d6d7962f9bdf82c3f543774a1a87/chrome/browser/ui/views/settings_reset_prompt_dialog_browsertest.cc
,
Sep 15 2017
The patch in #3 should fix the issue on trunk. Feel free to update this bug if there are other issues related to this.
,
Sep 15 2017
Thanks for the quick fix :) Can this also be merged to M62?
,
Sep 16 2017
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 18 2017
Thanks for the fix. Can you please confirm why this is required for M62? What are the issues if we wait until M63? Since we're past branch point, we'd only like to take fixes that are critical and necessary.
,
Sep 19 2017
@abdulsyed This fixes a build error that also impacts M62. It does not include functional changes.
,
Sep 19 2017
Approving merge to M62. Branch: 3202
,
Sep 19 2017
Just fyi, I'm going to try to do merge soon.
,
Sep 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/75cd8b6327e3b002adb48fbd7223d251e952eb5c commit 75cd8b6327e3b002adb48fbd7223d251e952eb5c Author: Ali Tofigh <alito@chromium.org> Date: Tue Sep 19 19:25:42 2017 [Merge to M62] Settings Reset Prompt: move Win-only code to separate file The ShowSettingsResetPrompt() function was declared as a static function in the SettingsResetPromptController class. The latter class is compiled on more than just Windows, but the settings reset prompt dialog is available on Windows only. This CL moves all the code that can end up displaying the settings reset prompt dialog to a win-only file. (cherry picked from commit 525f78bdde12d6d7962f9bdf82c3f543774a1a87) Bug: 764811 Change-Id: I486d6ee4c1850c5da65e873c47df1574fce8acb1 Reviewed-on: https://chromium-review.googlesource.com/666979 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Chris Sharp <csharp@chromium.org> Commit-Queue: Ali Tofigh <alito@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#502263} Reviewed-on: https://chromium-review.googlesource.com/673132 Reviewed-by: Ali Tofigh <alito@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#329} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/75cd8b6327e3b002adb48fbd7223d251e952eb5c/chrome/browser/chrome_browser_main_win.cc [modify] https://crrev.com/75cd8b6327e3b002adb48fbd7223d251e952eb5c/chrome/browser/safe_browsing/BUILD.gn [modify] https://crrev.com/75cd8b6327e3b002adb48fbd7223d251e952eb5c/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_dependency_browsertest_win.cc [modify] https://crrev.com/75cd8b6327e3b002adb48fbd7223d251e952eb5c/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc [modify] https://crrev.com/75cd8b6327e3b002adb48fbd7223d251e952eb5c/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.h [modify] https://crrev.com/75cd8b6327e3b002adb48fbd7223d251e952eb5c/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.h [add] https://crrev.com/75cd8b6327e3b002adb48fbd7223d251e952eb5c/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_util_win.cc [add] https://crrev.com/75cd8b6327e3b002adb48fbd7223d251e952eb5c/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_util_win.h [modify] https://crrev.com/75cd8b6327e3b002adb48fbd7223d251e952eb5c/chrome/browser/ui/browser_dialogs.h [modify] https://crrev.com/75cd8b6327e3b002adb48fbd7223d251e952eb5c/chrome/browser/ui/views/settings_reset_prompt_dialog.cc [modify] https://crrev.com/75cd8b6327e3b002adb48fbd7223d251e952eb5c/chrome/browser/ui/views/settings_reset_prompt_dialog_browsertest.cc |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by jialiul@chromium.org
, Sep 13 2017Owner: alito@chromium.org
Status: Assigned (was: Untriaged)