Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2607953002:
Add flash plugin path to chrome://version (Closed)
|
Created:
3 years, 11 months ago by Greg K Modified:
3 years, 10 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd flash plugin path to chrome://version
This adds the flash plugin path to the chrome://version page, which
allows users to see which flash plugin is loaded. This also allows
chrome://plugins to be removed.
BUG=667826
Committed: https://crrev.com/74ec606eb2ec3da72cb6cf142cbe2c327726198b
Cr-Commit-Position: refs/heads/master@{#441452}
Patch Set 1 #Patch Set 2 : Fix build on windows. #
Total comments: 6
Patch Set 3 : Fix RTL issues #
Total comments: 1
Patch Set 4 : Fix the final nit. #Messages
Total messages: 34 (27 generated)
The CQ bit was checked by kerrnel@chromium.org 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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by kerrnel@chromium.org 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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by kerrnel@chromium.org 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.
kerrnel@chromium.org changed reviewers: + bauerb@chromium.org
PTAL. Thanks, Greg
https://codereview.chromium.org/2607953002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/version_handler.cc (right): https://codereview.chromium.org/2607953002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/version_handler.cc:113: l10n_util::GetStringUTF16(IDS_PLUGINS_DISABLED_PLUGIN); So, it looks like chrome://version is localized? If that is indeed the case, manually concatenating strings like below might lead to wrong results, e.g. when using RTL languages. https://codereview.chromium.org/2607953002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/version_handler.cc:120: flash_version_and_path.append(base::UTF8ToUTF16(" ")); I would use base::ASCIIToUTF16() for the literal. But actually, should this be base::StringPrintF()? https://codereview.chromium.org/2607953002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/version_handler.cc:125: base::UTF8ToUTF16(info_array[i].path.value())); Use FilePath::LossyDisplayName() instead of platform-specific conversions.
The CQ bit was checked by kerrnel@chromium.org 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.
https://codereview.chromium.org/2607953002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/version_handler.cc (right): https://codereview.chromium.org/2607953002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/version_handler.cc:113: l10n_util::GetStringUTF16(IDS_PLUGINS_DISABLED_PLUGIN); On 2017/01/03 15:23:17, Bernhard (just came back) wrote: > So, it looks like chrome://version is localized? If that is indeed the case, > manually concatenating strings like below might lead to wrong results, e.g. when > using RTL languages. Acknowledged. https://codereview.chromium.org/2607953002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/version_handler.cc:120: flash_version_and_path.append(base::UTF8ToUTF16(" ")); On 2017/01/03 15:23:17, Bernhard (just came back) wrote: > I would use base::ASCIIToUTF16() for the literal. But actually, should this be > base::StringPrintF()? I switched this to base::StringPrintf and tested with an RTL language (Hebrew), and it produces the correct result. https://codereview.chromium.org/2607953002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/version_handler.cc:125: base::UTF8ToUTF16(info_array[i].path.value())); On 2017/01/03 15:23:17, Bernhard (just came back) wrote: > Use FilePath::LossyDisplayName() instead of platform-specific conversions. Done.
lgtm https://codereview.chromium.org/2607953002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/version_handler.cc (right): https://codereview.chromium.org/2607953002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/version_handler.cc:113: base::string16 flash_version_and_path = If you make this a std::string, you can save one UTF8-to-UTF16 conversion below. GetStringF() has an UTF8 variant, and the StringValue constructor takes either.
The CQ bit was checked by kerrnel@chromium.org 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 kerrnel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2607953002/#ps80001 (title: "Fix the final nit.")
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": 80001, "attempt_start_ts": 1483560993779380,
"parent_rev": "3c869b94c794242b70a90be68511d5494eaae2e9", "commit_rev":
"b4860ef947f28e5ad1101b9b75ec18ef94468186"}
Message was sent while issue was closed.
Description was changed from ========== Add flash plugin path to chrome://version This adds the flash plugin path to the chrome://version page, which allows users to see which flash plugin is loaded. This also allows chrome://plugins to be removed. BUG=667826 ========== to ========== Add flash plugin path to chrome://version This adds the flash plugin path to the chrome://version page, which allows users to see which flash plugin is loaded. This also allows chrome://plugins to be removed. BUG=667826 Review-Url: https://codereview.chromium.org/2607953002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add flash plugin path to chrome://version This adds the flash plugin path to the chrome://version page, which allows users to see which flash plugin is loaded. This also allows chrome://plugins to be removed. BUG=667826 Review-Url: https://codereview.chromium.org/2607953002 ========== to ========== Add flash plugin path to chrome://version This adds the flash plugin path to the chrome://version page, which allows users to see which flash plugin is loaded. This also allows chrome://plugins to be removed. BUG=667826 Committed: https://crrev.com/74ec606eb2ec3da72cb6cf142cbe2c327726198b Cr-Commit-Position: refs/heads/master@{#441452} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/74ec606eb2ec3da72cb6cf142cbe2c327726198b Cr-Commit-Position: refs/heads/master@{#441452} |
