Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1269383005:
Add channel-specific product logo to chrome://help (Closed)
|
Created:
5 years, 4 months ago by Evan Stade Modified:
5 years, 4 months ago CC:
chromium-reviews, arv+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd channel-specific product logo to chrome://help
BUG=511000
Committed: https://crrev.com/9836302ea4ffb887ce6ffa597c1ee5484d2f1091
Cr-Commit-Position: refs/heads/master@{#342183}
Patch Set 1 #Patch Set 2 : self review #
Total comments: 2
Patch Set 3 : sort #
Total comments: 8
Messages
Total messages: 22 (3 generated)
estade@chromium.org changed reviewers: + dbeam@chromium.org, oshima@chromium.org
+dbeam for browser/resources/ +oshima for app/theme/ This CL depends on two patches to add the images to the internal repo.
lgtm https://codereview.chromium.org/1269383005/diff/20001/chrome/app/theme/theme_... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/1269383005/diff/20001/chrome/app/theme/theme_... chrome/app/theme/theme_resources.grd:609: <structure type="chrome_scaled_image" name="IDR_PRODUCT_LOGO_32_BETA" file="google_chrome/product_logo_32_beta.png" /> nit: can you sort (move it after ID_PRODUCT_LOGO_32") ?
https://codereview.chromium.org/1269383005/diff/20001/chrome/app/theme/theme_... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/1269383005/diff/20001/chrome/app/theme/theme_... chrome/app/theme/theme_resources.grd:609: <structure type="chrome_scaled_image" name="IDR_PRODUCT_LOGO_32_BETA" file="google_chrome/product_logo_32_beta.png" /> On 2015/08/05 21:40:12, oshima wrote: > nit: can you sort (move it after ID_PRODUCT_LOGO_32") ? Done.
> +dbeam for browser/resources/ as well as webui/
https://codereview.chromium.org/1269383005/diff/40001/chrome/app/theme/theme_... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/1269383005/diff/40001/chrome/app/theme/theme_... chrome/app/theme/theme_resources.grd:601: <structure type="chrome_scaled_image" name="IDR_PRODUCT_LOGO" file="google_chrome/product_logo.png" /> why can't we just change IDR_PRODUCT_LOGO based on the channel at compile time?
https://codereview.chromium.org/1269383005/diff/40001/chrome/app/theme/theme_... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/1269383005/diff/40001/chrome/app/theme/theme_... chrome/app/theme/theme_resources.grd:601: <structure type="chrome_scaled_image" name="IDR_PRODUCT_LOGO" file="google_chrome/product_logo.png" /> On 2015/08/05 22:41:25, Dan Beam wrote: > why can't we just change IDR_PRODUCT_LOGO based on the channel at compile time? channel is not a compile time property
https://codereview.chromium.org/1269383005/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/help_ui.cc (right): https://codereview.chromium.org/1269383005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_ui.cc:33: #if defined(GOOGLE_CHROME_BUILD) why is this necessary? doesn't GetChannel() do this for you? https://codereview.chromium.org/1269383005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_ui.cc:58: source->AddResourcePath("current-channel-logo", product_logo); nit: int GetProductLogo() { switch (chrome::GetChannel()) { case version_info::Channel::CANARY: return IDR_PRODUCT_LOGO_32_CANARY; case version_info::Channel::DEV: return IDR_PRODUCT_LOGO_32_DEV; case version_info::Channel::BETA: return IDR_PRODUCT_LOGO_32_BETA; } return IDR_PRODUCT_LOGO_32; } source->AddResourcePath("current-channel-logo", GetProductLogo()); seems a bit simpler
https://codereview.chromium.org/1269383005/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/help_ui.cc (right): https://codereview.chromium.org/1269383005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_ui.cc:33: #if defined(GOOGLE_CHROME_BUILD) On 2015/08/06 02:49:07, Dan Beam wrote: > why is this necessary? doesn't GetChannel() do this for you? GetChannel should never return CANARY for chromium builds. But since IDR_PRODUCT_LOGO_32_[CHANNEL] is Chrome-only, it can't be referenced in chromium builds. https://codereview.chromium.org/1269383005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_ui.cc:58: source->AddResourcePath("current-channel-logo", product_logo); On 2015/08/06 02:49:07, Dan Beam wrote: > nit: > > int GetProductLogo() { > switch (chrome::GetChannel()) { > case version_info::Channel::CANARY: > return IDR_PRODUCT_LOGO_32_CANARY; > case version_info::Channel::DEV: > return IDR_PRODUCT_LOGO_32_DEV; > case version_info::Channel::BETA: > return IDR_PRODUCT_LOGO_32_BETA; > } > return IDR_PRODUCT_LOGO_32; > } > > source->AddResourcePath("current-channel-logo", GetProductLogo()); > > seems a bit simpler that doesn't make the compile break when a new channel type is added
https://codereview.chromium.org/1269383005/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/help_ui.cc (right): https://codereview.chromium.org/1269383005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_ui.cc:58: source->AddResourcePath("current-channel-logo", product_logo); On 2015/08/06 03:01:24, Evan Stade wrote: > On 2015/08/06 02:49:07, Dan Beam wrote: > > nit: > > > > int GetProductLogo() { > > switch (chrome::GetChannel()) { > > case version_info::Channel::CANARY: > > return IDR_PRODUCT_LOGO_32_CANARY; > > case version_info::Channel::DEV: > > return IDR_PRODUCT_LOGO_32_DEV; > > case version_info::Channel::BETA: > > return IDR_PRODUCT_LOGO_32_BETA; > > } > > return IDR_PRODUCT_LOGO_32; > > } > > > > source->AddResourcePath("current-channel-logo", GetProductLogo()); > > > > seems a bit simpler > > that doesn't make the compile break when a new channel type is added then handle version_info::Channel::UNKNOWN
lgtm after in-person explanation https://codereview.chromium.org/1269383005/diff/40001/chrome/app/theme/theme_... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/1269383005/diff/40001/chrome/app/theme/theme_... chrome/app/theme/theme_resources.grd:600: <if expr="_google_chrome"> ah, this is why
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1269383005/#ps40001 (title: "sort")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1269383005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1269383005/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9836302ea4ffb887ce6ffa597c1ee5484d2f1091 Cr-Commit-Position: refs/heads/master@{#342183}
Message was sent while issue was closed.
On 2015/08/06 19:56:08, commit-bot: I haz the power wrote: > Patchset 3 (id:??) landed as > https://crrev.com/9836302ea4ffb887ce6ffa597c1ee5484d2f1091 > Cr-Commit-Position: refs/heads/master@{#342183} Could it be that the new files themselves are missing? I'm getting this error now when I sync: Exception: The resource file was not found. : Tried app/theme/default_{200,100}_percent/google_chrome/product_logo_32_beta.png Traceback (most recent call last): File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 491, in CallLoadTargetBuildFile includes, depth, check, False) File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 412, in LoadTargetBuildFile build_file_data, PHASE_EARLY, variables, build_file_path) File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1283, in ProcessVariablesAndConditionsInDict build_file) File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1298, in ProcessVariablesAndConditionsInList ProcessVariablesAndConditionsInDict(item, phase, variables, build_file) File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1283, in ProcessVariablesAndConditionsInDict build_file) File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1298, in ProcessVariablesAndConditionsInList ProcessVariablesAndConditionsInDict(item, phase, variables, build_file) File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1283, in ProcessVariablesAndConditionsInDict build_file) File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1302, in ProcessVariablesAndConditionsInList expanded = ExpandVariables(item, phase, variables, build_file) File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 886, in ExpandVariables replacement = str(py_module.DoMain(parsed_contents[1:])).rstrip() File "/Users/tommi/src/src/tools/grit/grit_info.py", line 150, in DoMain options.target_platform) File "/Users/tommi/src/src/tools/grit/grit_info.py", line 81, in Inputs path = node.GetInputPath() File "/Users/tommi/src/src/tools/grit/grit/node/structure.py", line 213, in GetInputPath return self.gatherer.GetInputPath() File "/Users/tommi/src/src/tools/grit/grit/gather/chrome_scaled_image.py", line 137, in GetInputPath path, scale, req_scale = self._FindInputFile() File "/Users/tommi/src/src/tools/grit/grit/gather/chrome_scaled_image.py", line 134, in _FindInputFile 'Tried ' + self.grd_node.ToRealPath(os.path.join(dir, self.rc_file))) FileNotFound: The resource file was not found. : Tried app/theme/default_{200,100}_percent/google_chrome/product_logo_32_beta.png Error processing node <?xml version="1.0" encoding="UTF-8"?> <structure file="google_chrome/product_logo_32_beta.png" name="IDR_PRODUCT_LOGO_32_BETA" type="chrome_scaled_image" /> Error: Command '/usr/bin/python src/build/gyp_chromium' returned non-zero exit status 1 in /Users/tommi/src
Message was sent while issue was closed.
On 2015/08/07 09:19:19, tommi wrote: > On 2015/08/06 19:56:08, commit-bot: I haz the power wrote: > > Patchset 3 (id:??) landed as > > https://crrev.com/9836302ea4ffb887ce6ffa597c1ee5484d2f1091 > > Cr-Commit-Position: refs/heads/master@{#342183} > > Could it be that the new files themselves are missing? > I'm getting this error now when I sync: > > Exception: The resource file was not found. > : Tried > app/theme/default_{200,100}_percent/google_chrome/product_logo_32_beta.png > Traceback (most recent call last): > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 491, in > CallLoadTargetBuildFile > includes, depth, check, False) > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 412, in > LoadTargetBuildFile > build_file_data, PHASE_EARLY, variables, build_file_path) > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1283, in > ProcessVariablesAndConditionsInDict > build_file) > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1298, in > ProcessVariablesAndConditionsInList > ProcessVariablesAndConditionsInDict(item, phase, variables, build_file) > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1283, in > ProcessVariablesAndConditionsInDict > build_file) > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1298, in > ProcessVariablesAndConditionsInList > ProcessVariablesAndConditionsInDict(item, phase, variables, build_file) > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1283, in > ProcessVariablesAndConditionsInDict > build_file) > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1302, in > ProcessVariablesAndConditionsInList > expanded = ExpandVariables(item, phase, variables, build_file) > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 886, in > ExpandVariables > replacement = str(py_module.DoMain(parsed_contents[1:])).rstrip() > File "/Users/tommi/src/src/tools/grit/grit_info.py", line 150, in DoMain > options.target_platform) > File "/Users/tommi/src/src/tools/grit/grit_info.py", line 81, in Inputs > path = node.GetInputPath() > File "/Users/tommi/src/src/tools/grit/grit/node/structure.py", line 213, in > GetInputPath > return self.gatherer.GetInputPath() > File "/Users/tommi/src/src/tools/grit/grit/gather/chrome_scaled_image.py", > line 137, in GetInputPath > path, scale, req_scale = self._FindInputFile() > File "/Users/tommi/src/src/tools/grit/grit/gather/chrome_scaled_image.py", > line 134, in _FindInputFile > 'Tried ' + self.grd_node.ToRealPath(os.path.join(dir, self.rc_file))) > FileNotFound: The resource file was not found. > : Tried > app/theme/default_{200,100}_percent/google_chrome/product_logo_32_beta.png > > Error processing node <?xml version="1.0" encoding="UTF-8"?> > <structure file="google_chrome/product_logo_32_beta.png" > name="IDR_PRODUCT_LOGO_32_BETA" type="chrome_scaled_image" /> > Error: Command '/usr/bin/python src/build/gyp_chromium' returned non-zero exit > status 1 in /Users/tommi/src I have GYP_DEFINES='branding=Chrome' btw.
Message was sent while issue was closed.
On 2015/08/07 09:22:32, tommi wrote: > On 2015/08/07 09:19:19, tommi wrote: > > On 2015/08/06 19:56:08, commit-bot: I haz the power wrote: > > > Patchset 3 (id:??) landed as > > > https://crrev.com/9836302ea4ffb887ce6ffa597c1ee5484d2f1091 > > > Cr-Commit-Position: refs/heads/master@{#342183} > > > > Could it be that the new files themselves are missing? > > I'm getting this error now when I sync: > > > > Exception: The resource file was not found. > > : Tried > > app/theme/default_{200,100}_percent/google_chrome/product_logo_32_beta.png > > Traceback (most recent call last): > > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 491, in > > CallLoadTargetBuildFile > > includes, depth, check, False) > > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 412, in > > LoadTargetBuildFile > > build_file_data, PHASE_EARLY, variables, build_file_path) > > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1283, in > > ProcessVariablesAndConditionsInDict > > build_file) > > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1298, in > > ProcessVariablesAndConditionsInList > > ProcessVariablesAndConditionsInDict(item, phase, variables, build_file) > > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1283, in > > ProcessVariablesAndConditionsInDict > > build_file) > > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1298, in > > ProcessVariablesAndConditionsInList > > ProcessVariablesAndConditionsInDict(item, phase, variables, build_file) > > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1283, in > > ProcessVariablesAndConditionsInDict > > build_file) > > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 1302, in > > ProcessVariablesAndConditionsInList > > expanded = ExpandVariables(item, phase, variables, build_file) > > File "/Users/tommi/src/src/tools/gyp/pylib/gyp/input.py", line 886, in > > ExpandVariables > > replacement = str(py_module.DoMain(parsed_contents[1:])).rstrip() > > File "/Users/tommi/src/src/tools/grit/grit_info.py", line 150, in DoMain > > options.target_platform) > > File "/Users/tommi/src/src/tools/grit/grit_info.py", line 81, in Inputs > > path = node.GetInputPath() > > File "/Users/tommi/src/src/tools/grit/grit/node/structure.py", line 213, in > > GetInputPath > > return self.gatherer.GetInputPath() > > File "/Users/tommi/src/src/tools/grit/grit/gather/chrome_scaled_image.py", > > line 137, in GetInputPath > > path, scale, req_scale = self._FindInputFile() > > File "/Users/tommi/src/src/tools/grit/grit/gather/chrome_scaled_image.py", > > line 134, in _FindInputFile > > 'Tried ' + self.grd_node.ToRealPath(os.path.join(dir, self.rc_file))) > > FileNotFound: The resource file was not found. > > : Tried > > app/theme/default_{200,100}_percent/google_chrome/product_logo_32_beta.png > > > > Error processing node <?xml version="1.0" encoding="UTF-8"?> > > <structure file="google_chrome/product_logo_32_beta.png" > > name="IDR_PRODUCT_LOGO_32_BETA" type="chrome_scaled_image" /> > > Error: Command '/usr/bin/python src/build/gyp_chromium' returned non-zero exit > > status 1 in /Users/tommi/src > > I have GYP_DEFINES='branding=Chrome' btw. OK, so I tried reverting this patch locally and now things work again. So I'm going to revert this change in order to fix chrome builds.
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1280033002/ by tommi@chromium.org. The reason for reverting is: Reverting due to missing files. See comments in cl: https://codereview.chromium.org/1269383005/.
Message was sent while issue was closed.
On 2015/08/07 09:27:44, tommi wrote: > A revert of this CL (patchset #3 id:40001) has been created in > https://codereview.chromium.org/1280033002/ by mailto:tommi@chromium.org. > > The reason for reverting is: Reverting due to missing files. See comments in > cl: > https://codereview.chromium.org/1269383005/. Did this actually break any bots? As of this deps roll[1] (which landed before this change), you should have had the files in your checkout. [1] https://chromereviews.googleplex.com/235977013/diff/1/DEPS
Message was sent while issue was closed.
On 2015/08/07 17:12:48, Evan Stade wrote: > On 2015/08/07 09:27:44, tommi wrote: > > A revert of this CL (patchset #3 id:40001) has been created in > > https://codereview.chromium.org/1280033002/ by mailto:tommi@chromium.org. > > > > The reason for reverting is: Reverting due to missing files. See comments in > > cl: > > https://codereview.chromium.org/1269383005/. > > Did this actually break any bots? > > As of this deps roll[1] (which landed before this change), you should have had > the files in your checkout. > > [1] https://chromereviews.googleplex.com/235977013/diff/1/DEPS yea, it looks like the official builders were happy with this change. Perhaps your local config is messed up? (No longer checking out src-internal?) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
