Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2041573002:
Refactor flash component installer to use DefaultComponentInstaller. (Closed)
DescriptionRefactor flash component installer to use DefaultComponentInstaller.
This enables it to use differential updates.
BUG=601928
Committed: https://crrev.com/5a2538963cbd28c53ed0f4befc24bd9b4d99f77c
Cr-Commit-Position: refs/heads/master@{#401973}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Sync & non-Linux fix #
Messages
Total messages: 21 (4 generated)
waffles@chromium.org changed reviewers: + kerrnel@chromium.org, sorin@chromium.org, wfh@chromium.org
kerrnel, wfh, sorin, PTAL. I've verified that bundled Flash still loads correctly on Linux. Have not yet verified on other OS, or successfully observed a component update of Flash on Linux (something is wrong with my end to end test setup and the component update is not being correctly signed).
lgtm thank you. Stylewise, lgtm, I expect Greg and Will to review the logic of the code. https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/pepper_flash_component_installer.cc:238: Version unused; why is this variable named |unused| ?
you could consider re-using/stealing part of the testing plan for the last Flash component update change I made https://docs.google.com/document/d/1iTQiaqjuHsKV4cPqSOet-eJKWb2SsJLp2ieDj_Mul... In particular, I would recommend setting your official Flash version back by editing flapper_version.h and verifying a component update functions correctly.
https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/pepper_flash_component_installer.cc:205: #if defined(OS_LINUX) I notice it's no longer unpacking the plugin here. Does the DefaultComponentInstaller take care of that for us?
https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/pepper_flash_component_installer.cc:205: #if defined(OS_LINUX) On 2016/06/06 19:08:37, Greg Kerr wrote: > I notice it's no longer unpacking the plugin here. Does the > DefaultComponentInstaller take care of that for us? Yes, Install took the temp directory that the exploded CRX lived in and moved it to DIR_USER_DATA/PepperFlash/version/[contents]; The DefaultComponentInstaller's Install already does that, and then OnCustomInstall is called with the final path. https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/pepper_flash_component_installer.cc:238: Version unused; On 2016/06/03 23:41:44, Sorin Jianu wrote: > why is this variable named |unused| ? It's an outparam of CheckPepperFlashManifest, but we don't have any use for it at this call site.
https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... chrome/browser/component_updater/pepper_flash_component_installer.cc:205: #if defined(OS_LINUX) On 2016/06/06 19:37:03, waffles wrote: > On 2016/06/06 19:08:37, Greg Kerr wrote: > > I notice it's no longer unpacking the plugin here. Does the > > DefaultComponentInstaller take care of that for us? > > Yes, Install took the temp directory that the exploded CRX lived in and moved it > to DIR_USER_DATA/PepperFlash/version/[contents]; The DefaultComponentInstaller's > Install already does that, and then OnCustomInstall is called with the final > path. That might be a problem on Chrome OS where the temporary path of the component needs to be handed off to the ImageLoader dbus service. Is there a way to override the behavior for Chrome OS?
On 2016/06/06 20:43:06, Greg Kerr wrote: > https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... > File chrome/browser/component_updater/pepper_flash_component_installer.cc > (right): > > https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... > chrome/browser/component_updater/pepper_flash_component_installer.cc:205: #if > defined(OS_LINUX) > On 2016/06/06 19:37:03, waffles wrote: > > On 2016/06/06 19:08:37, Greg Kerr wrote: > > > I notice it's no longer unpacking the plugin here. Does the > > > DefaultComponentInstaller take care of that for us? > > > > Yes, Install took the temp directory that the exploded CRX lived in and moved > it > > to DIR_USER_DATA/PepperFlash/version/[contents]; The > DefaultComponentInstaller's > > Install already does that, and then OnCustomInstall is called with the final > > path. > > That might be a problem on Chrome OS where the temporary path of the component > needs to be handed off to the ImageLoader dbus service. Is there a way to > override the behavior for Chrome OS? Also I wanted to test this CL with Linux component updates myself and I saw it successfully component update flash to 21.0.0.216. I rebooted the browser to make sure it correctly picked up the new flash player still.
On 2016/06/06 20:55:50, Greg Kerr wrote: > On 2016/06/06 20:43:06, Greg Kerr wrote: > > > https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... > > File chrome/browser/component_updater/pepper_flash_component_installer.cc > > (right): > > > > > https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... > > chrome/browser/component_updater/pepper_flash_component_installer.cc:205: #if > > defined(OS_LINUX) > > On 2016/06/06 19:37:03, waffles wrote: > > > On 2016/06/06 19:08:37, Greg Kerr wrote: > > > > I notice it's no longer unpacking the plugin here. Does the > > > > DefaultComponentInstaller take care of that for us? > > > > > > Yes, Install took the temp directory that the exploded CRX lived in and > moved > > it > > > to DIR_USER_DATA/PepperFlash/version/[contents]; The > > DefaultComponentInstaller's > > > Install already does that, and then OnCustomInstall is called with the final > > > path. > > > > That might be a problem on Chrome OS where the temporary path of the component > > needs to be handed off to the ImageLoader dbus service. Is there a way to > > override the behavior for Chrome OS? > > Also I wanted to test this CL with Linux component updates myself and I saw it > successfully component update flash to 21.0.0.216. I rebooted the browser to > make sure it correctly picked up the new flash player still. Yep, I also see this working on Linux. I think it will work with my Mac build as well, however it won't with the official Mac build (because PepperFlash lives in "Internet Plug-Ins" instead of "Libraries"). How do you two feel about moving it?
On 2016/06/07 22:43:53, waffles wrote: > On 2016/06/06 20:55:50, Greg Kerr wrote: > > On 2016/06/06 20:43:06, Greg Kerr wrote: > > > > > > https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... > > > File chrome/browser/component_updater/pepper_flash_component_installer.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... > > > chrome/browser/component_updater/pepper_flash_component_installer.cc:205: > #if > > > defined(OS_LINUX) > > > On 2016/06/06 19:37:03, waffles wrote: > > > > On 2016/06/06 19:08:37, Greg Kerr wrote: > > > > > I notice it's no longer unpacking the plugin here. Does the > > > > > DefaultComponentInstaller take care of that for us? > > > > > > > > Yes, Install took the temp directory that the exploded CRX lived in and > > moved > > > it > > > > to DIR_USER_DATA/PepperFlash/version/[contents]; The > > > DefaultComponentInstaller's > > > > Install already does that, and then OnCustomInstall is called with the > final > > > > path. > > > > > > That might be a problem on Chrome OS where the temporary path of the > component > > > needs to be handed off to the ImageLoader dbus service. Is there a way to > > > override the behavior for Chrome OS? > > > > Also I wanted to test this CL with Linux component updates myself and I saw it > > successfully component update flash to 21.0.0.216. I rebooted the browser to > > make sure it correctly picked up the new flash player still. > > Yep, I also see this working on Linux. I think it will work with my Mac build as > well, however it won't with the official Mac build (because PepperFlash lives in > "Internet Plug-Ins" instead of "Libraries"). How do you two feel about moving > it? remind me again why we use a different directory for official and non-official? Something to do with testing? :S
On 2016/06/07 23:10:05, Will Harris wrote: > On 2016/06/07 22:43:53, waffles wrote: > > On 2016/06/06 20:55:50, Greg Kerr wrote: > > > On 2016/06/06 20:43:06, Greg Kerr wrote: > > > > > > > > > > https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... > > > > File chrome/browser/component_updater/pepper_flash_component_installer.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... > > > > chrome/browser/component_updater/pepper_flash_component_installer.cc:205: > > #if > > > > defined(OS_LINUX) > > > > On 2016/06/06 19:37:03, waffles wrote: > > > > > On 2016/06/06 19:08:37, Greg Kerr wrote: > > > > > > I notice it's no longer unpacking the plugin here. Does the > > > > > > DefaultComponentInstaller take care of that for us? > > > > > > > > > > Yes, Install took the temp directory that the exploded CRX lived in and > > > moved > > > > it > > > > > to DIR_USER_DATA/PepperFlash/version/[contents]; The > > > > DefaultComponentInstaller's > > > > > Install already does that, and then OnCustomInstall is called with the > > final > > > > > path. > > > > > > > > That might be a problem on Chrome OS where the temporary path of the > > component > > > > needs to be handed off to the ImageLoader dbus service. Is there a way to > > > > override the behavior for Chrome OS? > > > > > > Also I wanted to test this CL with Linux component updates myself and I saw > it > > > successfully component update flash to 21.0.0.216. I rebooted the browser to > > > make sure it correctly picked up the new flash player still. > > > > Yep, I also see this working on Linux. I think it will work with my Mac build > as > > well, however it won't with the official Mac build (because PepperFlash lives > in > > "Internet Plug-Ins" instead of "Libraries"). How do you two feel about moving > > it? > > remind me again why we use a different directory for official and non-official? > Something to do with testing? :S Sorry, I was actually incorrect with respect to the difference between my local and the official build. (I mistook the existence of the PepperFlash dir in ./out/GNOfficial/PepperFlash as indicating that it would be comparable to Linux - now that I look more closely I see Flash is also in ./out/GNOfficial/Chrome.app/Contents/.../Internet Plug-Ins/PepperFlash). So now there are two problems: (1) On Mac, PepperFlash lives in Internet Plug-Ins, other component(s) (Widevine) lives in Libraries. (2) On Mac, PepperFlash actually contains PepperFlash.plugin/...a bunch of stuff, and the path to Pepper Flash binaries themselves are buried deep down. We already have an e-mail thread on this topic, so let's just take the discussion back there.
On 2016/06/08 18:42:56, waffles wrote: > On 2016/06/07 23:10:05, Will Harris wrote: > > On 2016/06/07 22:43:53, waffles wrote: > > > On 2016/06/06 20:55:50, Greg Kerr wrote: > > > > On 2016/06/06 20:43:06, Greg Kerr wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... > > > > > File > chrome/browser/component_updater/pepper_flash_component_installer.cc > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2041573002/diff/1/chrome/browser/component_up... > > > > > > chrome/browser/component_updater/pepper_flash_component_installer.cc:205: > > > #if > > > > > defined(OS_LINUX) > > > > > On 2016/06/06 19:37:03, waffles wrote: > > > > > > On 2016/06/06 19:08:37, Greg Kerr wrote: > > > > > > > I notice it's no longer unpacking the plugin here. Does the > > > > > > > DefaultComponentInstaller take care of that for us? > > > > > > > > > > > > Yes, Install took the temp directory that the exploded CRX lived in > and > > > > moved > > > > > it > > > > > > to DIR_USER_DATA/PepperFlash/version/[contents]; The > > > > > DefaultComponentInstaller's > > > > > > Install already does that, and then OnCustomInstall is called with the > > > final > > > > > > path. > > > > > > > > > > That might be a problem on Chrome OS where the temporary path of the > > > component > > > > > needs to be handed off to the ImageLoader dbus service. Is there a way > to > > > > > override the behavior for Chrome OS? > > > > > > > > Also I wanted to test this CL with Linux component updates myself and I > saw > > it > > > > successfully component update flash to 21.0.0.216. I rebooted the browser > to > > > > make sure it correctly picked up the new flash player still. > > > > > > Yep, I also see this working on Linux. I think it will work with my Mac > build > > as > > > well, however it won't with the official Mac build (because PepperFlash > lives > > in > > > "Internet Plug-Ins" instead of "Libraries"). How do you two feel about > moving > > > it? > > > > remind me again why we use a different directory for official and > non-official? > > Something to do with testing? :S > > Sorry, I was actually incorrect with respect to the difference between my local > and the official build. (I mistook the existence of the PepperFlash dir in > ./out/GNOfficial/PepperFlash as indicating that it would be comparable to Linux > - now that I look more closely I see Flash is also in > ./out/GNOfficial/Chrome.app/Contents/.../Internet Plug-Ins/PepperFlash). > > So now there are two problems: > (1) On Mac, PepperFlash lives in Internet Plug-Ins, other component(s) > (Widevine) lives in Libraries. > (2) On Mac, PepperFlash actually contains PepperFlash.plugin/...a bunch of > stuff, and the path to Pepper Flash binaries themselves are buried deep down. > > We already have an e-mail thread on this topic, so let's just take the > discussion back there. (Actually, the 2nd of those isn't a problem because the component update has that structure, too! I never knew... but we are missing manifest.json on Mac, AFAICT.)
OK, PTAL - The code has not changed but the Mac issues are fixed: https://codereview.chromium.org/2080313004/ added manifest.json for Mac. https://codereview.chromium.org/2085583005/ added Internet Plug-Ins as a valid place for DCI to pick up Flash on Mac. I've successfully tested this code on Linux and Mac. > problem on Chrome OS I believe we've covered this concern (and Chrome OS behavior in general) in the e-mail "Strawman Flash Updates on ChromeOS". (I don't think there are any other outstanding concerns.)
On 2016/06/21 22:23:03, waffles wrote: > OK, PTAL - The code has not changed but the Mac issues are fixed: > https://codereview.chromium.org/2080313004/ added manifest.json for Mac. > https://codereview.chromium.org/2085583005/ added Internet Plug-Ins as a valid > place for DCI to pick up Flash on Mac. > > I've successfully tested this code on Linux and Mac. > > > problem on Chrome OS > I believe we've covered this concern (and Chrome OS behavior in general) in the > e-mail "Strawman Flash Updates on ChromeOS". > > (I don't think there are any other outstanding concerns.) LGTM
On 2016/06/21 22:23:03, waffles wrote: > OK, PTAL - The code has not changed but the Mac issues are fixed: > https://codereview.chromium.org/2080313004/ added manifest.json for Mac. > https://codereview.chromium.org/2085583005/ added Internet Plug-Ins as a valid > place for DCI to pick up Flash on Mac. > > I've successfully tested this code on Linux and Mac. > > > problem on Chrome OS > I believe we've covered this concern (and Chrome OS behavior in general) in the > e-mail "Strawman Flash Updates on ChromeOS". > > (I don't think there are any other outstanding concerns.) wfh@ is out sick; going to commit this to catch a dev rev or two before the branch cut. Will, if you spot any areas of concern, please reach out.
The CQ bit was checked by waffles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org Link to the patchset: https://codereview.chromium.org/2041573002/#ps20001 (title: "Sync & non-Linux fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Refactor flash component installer to use DefaultComponentInstaller. This enables it to use differential updates. BUG=601928 ========== to ========== Refactor flash component installer to use DefaultComponentInstaller. This enables it to use differential updates. BUG=601928 Committed: https://crrev.com/5a2538963cbd28c53ed0f4befc24bd9b4d99f77c Cr-Commit-Position: refs/heads/master@{#401973} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5a2538963cbd28c53ed0f4befc24bd9b4d99f77c Cr-Commit-Position: refs/heads/master@{#401973} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
