Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17556002:
Implement GetInstalledFile() in WidevineCdmComponentInstaller. (Closed)
|
Created:
7 years, 6 months ago by xhwang Modified:
7 years, 6 months ago CC:
chromium-reviews, waffles Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement GetInstalledFile() in WidevineCdmComponentInstaller.
TBR=cpu@chromium.org
BUG=180260
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208050
Patch Set 1 #
Total comments: 7
Patch Set 2 : ddorwin's comments #
Total comments: 2
Patch Set 3 : resolve sorin's comment #Patch Set 4 : rebase #Messages
Total messages: 15 (0 generated)
PTAL
https://codereview.chromium.org/17556002/diff/1/chrome/browser/component_upda... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/17556002/diff/1/chrome/browser/component_upda... chrome/browser/component_updater/widevine_cdm_component_installer.cc:254: // Only the CDM is component updated. tiny nit: component-updated https://codereview.chromium.org/17556002/diff/1/chrome/browser/component_upda... chrome/browser/component_updater/widevine_cdm_component_installer.cc:258: // No CDM has been installed yet. This reads as a fact when it should describe the check (or be above "return false;" https://codereview.chromium.org/17556002/diff/1/chrome/browser/component_upda... chrome/browser/component_updater/widevine_cdm_component_installer.cc:266: // Ideally we should check if PathExists(installed_cdm_path) here. But this What are the other implementations doing? Maybe the GetInstalledFile interface description should explain that "true" means I have a filename but don't know if it exists or is valid. I assume the differential updater will check that.
https://codereview.chromium.org/17556002/diff/1/chrome/browser/component_upda... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/17556002/diff/1/chrome/browser/component_upda... chrome/browser/component_updater/widevine_cdm_component_installer.cc:254: // Only the CDM is component updated. On 2013/06/21 17:54:49, ddorwin wrote: > tiny nit: component-updated Done. https://codereview.chromium.org/17556002/diff/1/chrome/browser/component_upda... chrome/browser/component_updater/widevine_cdm_component_installer.cc:258: // No CDM has been installed yet. On 2013/06/21 17:54:49, ddorwin wrote: > This reads as a fact when it should describe the check (or be above "return > false;" Done. https://codereview.chromium.org/17556002/diff/1/chrome/browser/component_upda... chrome/browser/component_updater/widevine_cdm_component_installer.cc:266: // Ideally we should check if PathExists(installed_cdm_path) here. But this On 2013/06/21 17:54:49, ddorwin wrote: > What are the other implementations doing? > Maybe the GetInstalledFile interface description should explain that "true" > means I have a filename but don't know if it exists or is valid. I assume the > differential updater will check that. I think I am the first one implementing this :) Sorin@: What do you think on this? I suppose the updater will check the path returned by this method, correct?
LGTM % sorin's reply.
https://codereview.chromium.org/17556002/diff/1/chrome/browser/component_upda... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/17556002/diff/1/chrome/browser/component_upda... chrome/browser/component_updater/widevine_cdm_component_installer.cc:266: // Ideally we should check if PathExists(installed_cdm_path) here. But this On 2013/06/21 17:54:49, ddorwin wrote: > What are the other implementations doing? > Maybe the GetInstalledFile interface description should explain that "true" > means I have a filename but don't know if it exists or is valid. I assume the > differential updater will check that. We will have a failure with a more specific error code later if GetInstalledFile returns true with a path that doesn't point to a file, or a path that has a file with a different CRC than we expect, so we don't consider it the installer's responsibility to verify that the file is as it should be. This function is just a name mapping function. There does not need to be any verification done at this step. I agree with ddorwin that the interface description should be expanded to detail this. By the way, we expect to call this on the "FILE" thread.
Please don't commit this until I LGTM. We've been reverted and landing this will break the build. https://codereview.chromium.org/17556002/diff/3002/chrome/browser/component_u... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/17556002/diff/3002/chrome/browser/component_u... chrome/browser/component_updater/widevine_cdm_component_installer.cc:265: // Ideally we should check if PathExists(installed_cdm_path) here. But this This comment can be removed. The function just maps file names to their location, no need to do any I/O to check the file presence or their integrity. You could also avoid the named local variable here and just assigned to the out parameter of the function. If you prefer the named variable, you could declare it const. Please read waffle's comment about what is expected of this function.
https://codereview.chromium.org/17556002/diff/3002/chrome/browser/component_u... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/17556002/diff/3002/chrome/browser/component_u... chrome/browser/component_updater/widevine_cdm_component_installer.cc:265: // Ideally we should check if PathExists(installed_cdm_path) here. But this On 2013/06/21 19:49:24, sorinj wrote: > This comment can be removed. The function just maps file names to their > location, no need to do any I/O to check the file presence or their integrity. > > You could also avoid the named local variable here and just assigned to the out > parameter of the function. If you prefer the named variable, you could declare > it const. > > Please read waffle's comment about what is expected of this function. Done.
sorin: you L'G'T'M'ed when you say I should wait for you to L'G'T'M :) You can revert this by typing "not l-g-t-m" (w/o dashes) in a new message. Anyway, I saw the revert and will wait for it to land again.
cpu@: could you please do an OWNER's review?
Hey...I've done any possible single n00b mistake that I could have done. Good job we have owners still.
lgtm We landed the diff update for components. https://codereview.chromium.org/15908002/ You can commit as soon as cpu approves it.
TBRing cpu since this is a pretty simple CL.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/17556002/14001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/17556002/14001
Message was sent while issue was closed.
Change committed as 208050 |
