Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(290)

Issue 1261333004: Add support for Flash Player Component updates on Linux (Closed)

Created:
5 years, 4 months ago by Greg K
Modified:
5 years, 4 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add support for Flash Player Component updates on Linux This change adds support for Flash Player Component updates on Linux. It enables the component updater on Linux, and creates a hints file that the zygote will use to locate and preload the latest component updated flash. This should not affect component update OS X and Windows, however, because it forks the update path on Linux from those platforms, there is a risk of a bug that could affect component update on OS X and Windows. BUG=460595 Committed: https://crrev.com/2bb08b2a5f6bcc382ebaccf1b50bd655592dfe51 Cr-Commit-Position: refs/heads/master@{#343435}

Patch Set 1 #

Total comments: 12

Patch Set 2 : Rebase to fix merge conflicts #

Total comments: 14

Patch Set 3 : Use SecureHash, fix up unit tests, general cleanup #

Total comments: 54

Patch Set 4 : Updates per jln's code review. #

Total comments: 10

Patch Set 5 : Further refinements. #

Total comments: 44

Patch Set 6 : Updates per bauerb's code review. #

Total comments: 2

Patch Set 7 : Fix up bauerb's final few comments. #

Total comments: 20
Unified diffs Side-by-side diffs Delta from patch set Stats (+647 lines, -17 lines) Patch
M chrome/browser/component_updater/pepper_flash_component_installer.cc View 1 2 3 4 5 6 11 chunks +33 lines, -11 lines 4 comments Download
M chrome/chrome_common.gypi View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_content_client.h View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/common/chrome_content_client.cc View 1 2 3 4 5 6 4 chunks +103 lines, -6 lines 10 comments Download
M chrome/common/chrome_content_client_unittest.cc View 1 2 3 2 chunks +40 lines, -0 lines 0 comments Download
M chrome/common/chrome_paths.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/common/chrome_paths.cc View 1 2 chunks +13 lines, -0 lines 2 comments Download
A chrome/common/component_flash_hint_file_linux.h View 1 2 3 4 5 6 1 chunk +52 lines, -0 lines 2 comments Download
A chrome/common/component_flash_hint_file_linux.cc View 1 2 3 4 5 6 1 chunk +225 lines, -0 lines 2 comments Download
A chrome/common/component_flash_hint_file_linux_unittest.cc View 1 2 3 4 5 6 1 chunk +167 lines, -0 lines 0 comments Download

Messages

Total messages: 37 (16 generated)
rickyz (no longer on Chrome)
https://codereview.chromium.org/1261333004/diff/1/chrome/browser/component_updater/pepper_flash_component_installer.cc File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1261333004/diff/1/chrome/browser/component_updater/pepper_flash_component_installer.cc#newcode16 chrome/browser/component_updater/pepper_flash_component_installer.cc:16: #include "base/files/important_file_writer.h" Are these still used in this file? ...
5 years, 4 months ago (2015-07-31 00:13:56 UTC) #2
Greg K
https://codereview.chromium.org/1261333004/diff/1/chrome/browser/component_updater/pepper_flash_component_installer.cc File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1261333004/diff/1/chrome/browser/component_updater/pepper_flash_component_installer.cc#newcode16 chrome/browser/component_updater/pepper_flash_component_installer.cc:16: #include "base/files/important_file_writer.h" On 2015/07/31 00:13:56, rickyz wrote: > Are ...
5 years, 4 months ago (2015-08-04 00:21:17 UTC) #6
jln (very slow on Chromium)
I took a 10s look and I'll take a better look tomorrow. https://codereview.chromium.org/1261333004/diff/60001/chrome/common/chrome_content_client.cc File chrome/common/chrome_content_client.cc ...
5 years, 4 months ago (2015-08-04 01:08:33 UTC) #7
Greg K
https://codereview.chromium.org/1261333004/diff/60001/chrome/common/chrome_content_client.cc File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/60001/chrome/common/chrome_content_client.cc#newcode323 chrome/common/chrome_content_client.cc:323: #endif // defined(FLAPPER_AVAILABLE) On 2015/08/04 01:08:33, jln wrote: > ...
5 years, 4 months ago (2015-08-04 18:30:00 UTC) #8
jln (very slow on Chromium)
Looks good in general. A few remarks. I wonder if we could get rid of ...
5 years, 4 months ago (2015-08-06 18:48:15 UTC) #15
Greg K
https://codereview.chromium.org/1261333004/diff/200001/chrome/browser/component_updater/pepper_flash_component_installer.cc File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1261333004/diff/200001/chrome/browser/component_updater/pepper_flash_component_installer.cc#newcode106 chrome/browser/component_updater/pepper_flash_component_installer.cc:106: #if !defined(OS_LINUX) || defined(GOOGLE_CHROME_BUILD) On 2015/08/06 18:48:14, jln wrote: ...
5 years, 4 months ago (2015-08-07 21:15:29 UTC) #17
jln (very slow on Chromium)
https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_content_client.cc File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_content_client.cc#newcode7 chrome/common/chrome_content_client.cc:7: #include <fcntl.h> might want to #ifdef this to Linux ...
5 years, 4 months ago (2015-08-07 22:27:20 UTC) #18
Greg K
https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_content_client.cc File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_content_client.cc#newcode7 chrome/common/chrome_content_client.cc:7: #include <fcntl.h> On 2015/08/07 22:27:19, jln wrote: > might ...
5 years, 4 months ago (2015-08-08 00:23:57 UTC) #19
jln (very slow on Chromium)
lgtm, on to owner reviews! Nit: it's better if your commit message respects 80 columns.
5 years, 4 months ago (2015-08-08 00:53:44 UTC) #20
Greg K
jochen@chromium.org: Please review changes.
5 years, 4 months ago (2015-08-10 16:28:37 UTC) #22
jochen (gone - plz use gerrit)
deferring to bauerb@ rubberstamp lgtm once he's happy
5 years, 4 months ago (2015-08-12 16:23:42 UTC) #24
Bernhard Bauer
https://codereview.chromium.org/1261333004/diff/260001/chrome/common/chrome_content_client.cc File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/260001/chrome/common/chrome_content_client.cc#newcode510 chrome/common/chrome_content_client.cc:510: // static Move the comment inside of the #ifdef? ...
5 years, 4 months ago (2015-08-13 12:40:27 UTC) #25
Greg K
https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/chrome_content_client.cc File chrome/common/chrome_content_client.cc (right): https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/chrome_content_client.cc#newcode510 chrome/common/chrome_content_client.cc:510: // static On 2015/08/13 12:40:26, Bernhard Bauer wrote: > ...
5 years, 4 months ago (2015-08-13 22:00:45 UTC) #26
Bernhard Bauer
LGTM with an optional comment and a nit: https://codereview.chromium.org/1261333004/diff/260001/chrome/common/component_flash_hint_file_linux.cc File chrome/common/component_flash_hint_file_linux.cc (right): https://codereview.chromium.org/1261333004/diff/260001/chrome/common/component_flash_hint_file_linux.cc#newcode5 chrome/common/component_flash_hint_file_linux.cc:5: #include ...
5 years, 4 months ago (2015-08-14 07:55:12 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1261333004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261333004/300001
5 years, 4 months ago (2015-08-14 18:10:53 UTC) #30
commit-bot: I haz the power
Committed patchset #7 (id:300001)
5 years, 4 months ago (2015-08-14 18:16:05 UTC) #31
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/2bb08b2a5f6bcc382ebaccf1b50bd655592dfe51 Cr-Commit-Position: refs/heads/master@{#343435}
5 years, 4 months ago (2015-08-14 18:16:33 UTC) #32
Lei Zhang
Just some drive by nits to fix later. https://codereview.chromium.org/1261333004/diff/300001/chrome/browser/component_updater/pepper_flash_component_installer.cc File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1261333004/diff/300001/chrome/browser/component_updater/pepper_flash_component_installer.cc#newcode246 chrome/browser/component_updater/pepper_flash_component_installer.cc:246: if ...
5 years, 4 months ago (2015-08-14 22:37:08 UTC) #34
Lei Zhang
https://codereview.chromium.org/1261333004/diff/300001/chrome/browser/component_updater/pepper_flash_component_installer.cc File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1261333004/diff/300001/chrome/browser/component_updater/pepper_flash_component_installer.cc#newcode232 chrome/browser/component_updater/pepper_flash_component_installer.cc:232: GetPepperFlashBaseDirectory().AppendASCII(version.GetString()); Since |path| is being deleted recursively, we should ...
5 years, 4 months ago (2015-08-17 19:06:13 UTC) #35
Greg K
https://codereview.chromium.org/1261333004/diff/280001/chrome/common/component_flash_hint_file.h File chrome/common/component_flash_hint_file.h (right): https://codereview.chromium.org/1261333004/diff/280001/chrome/common/component_flash_hint_file.h#newcode4 chrome/common/component_flash_hint_file.h:4: // On 2015/08/14 07:55:12, Bernhard Bauer wrote: > Remove ...
5 years, 4 months ago (2015-08-17 21:11:57 UTC) #36
Lei Zhang
5 years, 4 months ago (2015-08-17 21:14:50 UTC) #37
Message was sent while issue was closed.
For the record, the follow-up CL is https://codereview.chromium.org/1298013002/

Powered by Google App Engine
This is Rietveld 408576698