Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2424093003:
Include the AppData file for Chrome and Chromium (Closed)
DescriptionInclude the AppData file for Chrome and Chromium
Add the two AppData files, one for Chromium and one for Chrome. The Chrome one
will be filled with the information from the common/google-chrome/google-chrome.info
file and should reflect the current channel (stable, beta, devel). On the other
side the Chromium one should be a general one and it will be reused by
distributions that are packaging the Chromium themselves.
For AppData specification please see:
https://people.freedesktop.org/~hughsient/appdata/
BUG=654190
Review-Url: https://codereview.chromium.org/2424093003
Cr-Original-Original-Commit-Position: refs/heads/master@{#455457}
Committed: https://chromium.googlesource.com/chromium/src/+/d6807b9a7da70dae9191b93d4ec6157187515078
Review-Url: https://codereview.chromium.org/2424093003
Cr-Original-Commit-Position: refs/heads/master@{#455731}
Committed: https://chromium.googlesource.com/chromium/src/+/caa17f475b1ccf16ce0d4f89902663f4462396a8
Review-Url: https://codereview.chromium.org/2424093003
Cr-Commit-Position: refs/heads/master@{#458406}
Committed: https://chromium.googlesource.com/chromium/src/+/dd765c0d5ff5089329188018ac826e0728cad15e
Patch Set 1 #
Total comments: 1
Patch Set 2 : Change Chromium appdata description #Patch Set 3 : Add screenshot, feedback link and fix some of appstream-util validation warnings #
Total comments: 1
Patch Set 4 : Use the right directory with the AppData files while installing #Patch Set 5 : Fix the build and introduce the AppData template even for Chromium #
Total comments: 1
Patch Set 6 : Remove the Chromium AppData template to avoid duplicity #
Messages
Total messages: 56 (26 generated)
Description was changed from ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=564190 ========== to ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=564190 ==========
Description was changed from ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=564190 ========== to ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=654190 ==========
phajdan.jr@chromium.org changed reviewers: + phajdan.jr@chromium.org
I'm happy to get back to you with some results of internal review. I'm still working on getting official screenshots hosted on Google infrastructure. https://codereview.chromium.org/2424093003/diff/1/chrome/installer/linux/comm... File chrome/installer/linux/common/chromium-browser/chromium-browser.appdata.xml (right): https://codereview.chromium.org/2424093003/diff/1/chrome/installer/linux/comm... chrome/installer/linux/common/chromium-browser/chromium-browser.appdata.xml:11: Chromium is an open-source browser that aims to build a safer, faster, and Please change that to following approved snippet: Chromium is an open-source browser project that aims to build a safer, faster, and more stable way to experience the web. We invite you to join our effort to build a powerful platform for developing a new generation of web applications. Chromium supports Vorbis, Theora, WebM and HTML5 audio and video standards, but does not include the non-free AAC, H.264, MP3 or Adobe Flash code that is found in Chrome.
Update the description based on Pawel's feedback. Also there is a new pre-submit
warning:
Found Google support URL addressed by answer number. Please replace with a p=
identifier instead. See crbug.com/679462
chrome/installer/linux/common/google-chrome/google-chrome.appdata.xml.template:27
<url type="bugtracker">https://support.google.com/chrome/answer/95315</url>
Can anyone please create a p identifier for that link?
Please use https://support.google.com/chrome/?p=feedback (it'll get linked to the proper destination soon). I'll also point you to the new links for images once I get them hosted. There's a good progress on that.
On 2017/03/07 18:18:58, Paweł Hajdan Jr. wrote: > Please use https://support.google.com/chrome/?p=feedback (it'll get linked to > the proper destination soon). Thank you! > I'll also point you to the new links for images once I get them hosted. There's > a good progress on that. I will update the patch once you will send me the new links for images.
Sounds good. Please use https://www.gstatic.com/chrome/appstream/chrome-1.png for the screenshot. Chrome and Chromium look the same, I think it makes sense to only make a single screenshot.
Add screenshot, feedback link and fix some of appstream-util validation warnings. There are still some warnings left: 1 $ appstream-util validate-strict chrome/installer/linux/common/chromium-browser/chromium-browser.appdata.xml chrome/installer/linux/common/chromium-browser/chromium-browser.appdata.xml: FAILED: • aspect-ratio-invalid : <screenshot> aspect ratio not 16:9 [https://www.gstatic.com/chrome/appstream/chrome-1.png] • translations-required : <name> has no translations • translations-required : <summary> has no translations • translations-required : <description> has no translations Validation of files failed
LGTM
The CQ bit was checked by tomas.popela@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tomas.popela@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488986959609420,
"parent_rev": "ae6bbc0f6bed9bb278889e3548b3a7fa1d9e84ef", "commit_rev":
"d6807b9a7da70dae9191b93d4ec6157187515078"}
Message was sent while issue was closed.
Description was changed from ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=654190 ========== to ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=654190 Review-Url: https://codereview.chromium.org/2424093003 Cr-Commit-Position: refs/heads/master@{#455457} Committed: https://chromium.googlesource.com/chromium/src/+/d6807b9a7da70dae9191b93d4ec6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d6807b9a7da70dae9191b93d4ec6...
Message was sent while issue was closed.
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
Message was sent while issue was closed.
This is breaking Google Chrome Linux bots. https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2734783009/ by xhwang@chromium.org. The reason for reverting is: This is breaking Google Chrome Linux bots: https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux....
Message was sent while issue was closed.
On 2017/03/08 17:35:12, xhwang_slow wrote: > A revert of this CL (patchset #3 id:40001) has been created in > https://codereview.chromium.org/2734783009/ by mailto:xhwang@chromium.org. > > The reason for reverting is: This is breaking Google Chrome Linux bots: > > https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux.... I don't know what the root cause of the failure is, but from the log it could be related to Widevine signing script. If that's the case, we are reverting some of the changes related to Widevine signing: https://chromiumcodereview.appspot.com/2743433002/. You probably want to try again after that CL lands and see whether that fixes your issue. Sorry for the noise.
Message was sent while issue was closed.
On 2017/03/08 21:52:09, xhwang_slow wrote: > On 2017/03/08 17:35:12, xhwang_slow wrote: > > A revert of this CL (patchset #3 id:40001) has been created in > > https://codereview.chromium.org/2734783009/ by mailto:xhwang@chromium.org. > > > > The reason for reverting is: This is breaking Google Chrome Linux bots: > > > > > https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux.... > > I don't know what the root cause of the failure is, but from the log it could be > related to Widevine signing script. If that's the case, we are reverting some of > the changes related to Widevine signing: > https://chromiumcodereview.appspot.com/2743433002/. You probably want to try > again after that CL lands and see whether that fixes your issue. Sorry for the > noise. I don't think it has anything to do with widevine. There are errors like: src/out/Release/installer/common/google-chrome/google-chrome.appdata.xml: No such file or directory so I think the template file just isn't getting processed properly.
Message was sent while issue was closed.
On 2017/03/08 21:58:53, Michael Moss wrote: > On 2017/03/08 21:52:09, xhwang_slow wrote: > > On 2017/03/08 17:35:12, xhwang_slow wrote: > > > A revert of this CL (patchset #3 id:40001) has been created in > > > https://codereview.chromium.org/2734783009/ by mailto:xhwang@chromium.org. > > > > > > The reason for reverting is: This is breaking Google Chrome Linux bots: > > > > > > > > > https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux.... > > > > I don't know what the root cause of the failure is, but from the log it could > be > > related to Widevine signing script. If that's the case, we are reverting some > of > > the changes related to Widevine signing: > > https://chromiumcodereview.appspot.com/2743433002/. You probably want to try > > again after that CL lands and see whether that fixes your issue. Sorry for the > > noise. > > I don't think it has anything to do with widevine. There are errors like: > src/out/Release/installer/common/google-chrome/google-chrome.appdata.xml: No > such file or directory > so I think the template file just isn't getting processed properly. sg, I really don't know what this CL is doing :)
Message was sent while issue was closed.
In chrome/installer/linux/BUILD.gn the files are copied to
${BUILDDIR}/installer/common/ and not to
${BUILDDIR}/installer/common/google-chrome/ or
${BUILDDIR}/installer/common/chromium/.
Message was sent while issue was closed.
Description was changed from ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=654190 Review-Url: https://codereview.chromium.org/2424093003 Cr-Commit-Position: refs/heads/master@{#455457} Committed: https://chromium.googlesource.com/chromium/src/+/d6807b9a7da70dae9191b93d4ec6... ========== to ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=654190 Review-Url: https://codereview.chromium.org/2424093003 Cr-Commit-Position: refs/heads/master@{#455457} Committed: https://chromium.googlesource.com/chromium/src/+/d6807b9a7da70dae9191b93d4ec6... ==========
The CQ bit was checked by tomas.popela@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tomas.popela@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/2424093003/#ps60001 (title: "Use the right directory with the AppData files while installing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489059453940000,
"parent_rev": "b9712c4647ff4ca3f0e882bd3be173d1395d485b", "commit_rev":
"caa17f475b1ccf16ce0d4f89902663f4462396a8"}
Message was sent while issue was closed.
Description was changed from ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=654190 Review-Url: https://codereview.chromium.org/2424093003 Cr-Commit-Position: refs/heads/master@{#455457} Committed: https://chromium.googlesource.com/chromium/src/+/d6807b9a7da70dae9191b93d4ec6... ========== to ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=654190 Review-Url: https://codereview.chromium.org/2424093003 Cr-Original-Commit-Position: refs/heads/master@{#455457} Committed: https://chromium.googlesource.com/chromium/src/+/d6807b9a7da70dae9191b93d4ec6... Review-Url: https://codereview.chromium.org/2424093003 Cr-Commit-Position: refs/heads/master@{#455731} Committed: https://chromium.googlesource.com/chromium/src/+/caa17f475b1ccf16ce0d4f899026... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/caa17f475b1ccf16ce0d4f899026...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2738043003/ by fserb@chromium.org. The reason for reverting is: This is breaking linux build bot (again), please do not submit before fixing: https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux....
Message was sent while issue was closed.
Previously the builds failed because we were not creating the /usr/share/appdata directory in the build root. Now I managed to build the RPMs by modifying the chrome/installer/BUILD.gn and building the *_rpm targets. I also added the Chromium AppData template so it can be tested by the build scripts (even though it now requires modifying the chrome/installer/BUILD.gn). The change now contains the Chromium Appdata template and the generic Chromium Appdata file as well (that will be used by distributions).
Message was sent while issue was closed.
https://codereview.chromium.org/2424093003/diff/80001/chrome/installer/linux/... File chrome/installer/linux/common/chromium-browser/chromium-browser.appdata.xml (right): https://codereview.chromium.org/2424093003/diff/80001/chrome/installer/linux/... chrome/installer/linux/common/chromium-browser/chromium-browser.appdata.xml:1: <!-- Copyright 2017 The Chromium Authors --> Is the non-template file used?
Message was sent while issue was closed.
Description was changed from ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=654190 Review-Url: https://codereview.chromium.org/2424093003 Cr-Original-Commit-Position: refs/heads/master@{#455457} Committed: https://chromium.googlesource.com/chromium/src/+/d6807b9a7da70dae9191b93d4ec6... Review-Url: https://codereview.chromium.org/2424093003 Cr-Commit-Position: refs/heads/master@{#455731} Committed: https://chromium.googlesource.com/chromium/src/+/caa17f475b1ccf16ce0d4f899026... ========== to ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=654190 Review-Url: https://codereview.chromium.org/2424093003 Cr-Original-Commit-Position: refs/heads/master@{#455457} Committed: https://chromium.googlesource.com/chromium/src/+/d6807b9a7da70dae9191b93d4ec6... Review-Url: https://codereview.chromium.org/2424093003 Cr-Commit-Position: refs/heads/master@{#455731} Committed: https://chromium.googlesource.com/chromium/src/+/caa17f475b1ccf16ce0d4f899026... ==========
On 2017/03/20 10:47:24, Paweł Hajdan Jr. wrote: > Is the non-template file used? No, that one is meant to be used by distributions when they will package Chromium. So they don't need to process the template themselves.
xhwang@chromium.org changed reviewers: - xhwang@chromium.org
On 2017/03/20 10:53:21, tpopela wrote: > On 2017/03/20 10:47:24, Paweł Hajdan Jr. wrote: > > Is the non-template file used? > > No, that one is meant to be used by distributions when they will package > Chromium. So they don't need to process the template themselves. Can we somehow get rid of the duplication?
On 2017/03/21 13:04:06, Paweł Hajdan Jr. wrote: > Can we somehow get rid of the duplication? Yes, by removing one of them. Most like the template as this one is used by the various ninja targets to build the packages and this can't be achieved without modifying the GN files.
On 2017/03/21 13:08:23, tpopela wrote: > On 2017/03/21 13:04:06, Paweł Hajdan Jr. wrote: > > Can we somehow get rid of the duplication? > > Yes, by removing one of them. Most like the template as this one is used by the > various ninja targets to build the packages and this can't be achieved without > modifying the GN files. sgtm ; could you update the patch?
Removed the Chromium AppData template to avoid duplicity between it and the regular AppData file.
LGTM, thanks!
The CQ bit was checked by tomas.popela@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490104117310630,
"parent_rev": "05567589ab894a0524ac8263250905484fc3845e", "commit_rev":
"dd765c0d5ff5089329188018ac826e0728cad15e"}
Message was sent while issue was closed.
Description was changed from ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=654190 Review-Url: https://codereview.chromium.org/2424093003 Cr-Original-Commit-Position: refs/heads/master@{#455457} Committed: https://chromium.googlesource.com/chromium/src/+/d6807b9a7da70dae9191b93d4ec6... Review-Url: https://codereview.chromium.org/2424093003 Cr-Commit-Position: refs/heads/master@{#455731} Committed: https://chromium.googlesource.com/chromium/src/+/caa17f475b1ccf16ce0d4f899026... ========== to ========== Include the AppData file for Chrome and Chromium Add the two AppData files, one for Chromium and one for Chrome. The Chrome one will be filled with the information from the common/google-chrome/google-chrome.info file and should reflect the current channel (stable, beta, devel). On the other side the Chromium one should be a general one and it will be reused by distributions that are packaging the Chromium themselves. For AppData specification please see: https://people.freedesktop.org/~hughsient/appdata/ BUG=654190 Review-Url: https://codereview.chromium.org/2424093003 Cr-Original-Original-Commit-Position: refs/heads/master@{#455457} Committed: https://chromium.googlesource.com/chromium/src/+/d6807b9a7da70dae9191b93d4ec6... Review-Url: https://codereview.chromium.org/2424093003 Cr-Original-Commit-Position: refs/heads/master@{#455731} Committed: https://chromium.googlesource.com/chromium/src/+/caa17f475b1ccf16ce0d4f899026... Review-Url: https://codereview.chromium.org/2424093003 Cr-Commit-Position: refs/heads/master@{#458406} Committed: https://chromium.googlesource.com/chromium/src/+/dd765c0d5ff5089329188018ac82... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/dd765c0d5ff5089329188018ac82...
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2424093003/diff/40001/chrome/installer/linux/... File chrome/installer/linux/common/installer.include (right): https://codereview.chromium.org/2424093003/diff/40001/chrome/installer/linux/... chrome/installer/linux/common/installer.include:231: if [ ${PACKAGE:0:6} = google ]; then Curious, why not just check ${PACKAGE} ? The only possible values are google-chrome and chromium-browser.
Message was sent while issue was closed.
On 2017/07/25 22:13:59, Lei Zhang wrote: > https://codereview.chromium.org/2424093003/diff/40001/chrome/installer/linux/... > File chrome/installer/linux/common/installer.include (right): > > https://codereview.chromium.org/2424093003/diff/40001/chrome/installer/linux/... > chrome/installer/linux/common/installer.include:231: if [ ${PACKAGE:0:6} = > google ]; then > Curious, why not just check ${PACKAGE} ? The only possible values are > google-chrome and chromium-browser. Uploaded https://chromium-review.googlesource.com/c/585454/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
