|
|
Created:
6 years, 6 months ago by Sorin Jianu Modified:
6 years, 6 months ago CC:
chromium-reviews, cpu_(ooo_6.6-7.5) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake CUS interface more resilient by returning a copy of the data instead of raw pointer.
The idea here is to elminate the ambiguity about who owns what and protect
about misuse by callers.
BUG=381710
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276561
Patch Set 1 #Patch Set 2 : #
Total comments: 1
Patch Set 3 : Changed to use copy and value semantics #Patch Set 4 : Fixed comment #
Total comments: 2
Patch Set 5 : #
Messages
Total messages: 37 (0 generated)
lgtm; please try it and stuff of course
Carlos?
https://codereview.chromium.org/318143003/diff/20001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.h (right): https://codereview.chromium.org/318143003/diff/20001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.h:212: virtual base::WeakPtr<CrxUpdateItem> GetComponentDetails( This sparked an interesting discussion among few folks. The conclusion is that WeakPtr is not the appropriate mechanism for this and if you really want to avoid naked ptrs a const reference would be the best way to go about here. I personally can live with as is from the perspective of being a private function or make it a cons ref.
lgtm
The CQ bit was checked by sorin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sorin@chromium.org/318143003/60001
lgtm
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by sorin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sorin@chromium.org/318143003/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
James, please, I need an owner review for chrome\browser\ui\webui\components_ui.cc Thank you!
https://codereview.chromium.org/318143003/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/components_ui.cc (right): https://codereview.chromium.org/318143003/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/components_ui.cc:172: const bool res = cus->GetComponentDetails(component_ids[j], &item); What does 'res' mean? Can we get a better variable name? We could simplify it further by: if (cus->GetComponentDetails(...)) { ... }
Thank you James. Fixed. https://codereview.chromium.org/318143003/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/components_ui.cc (right): https://codereview.chromium.org/318143003/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/components_ui.cc:172: const bool res = cus->GetComponentDetails(component_ids[j], &item); On 2014/06/11 17:50:39, James Hawkins wrote: > What does 'res' mean? Can we get a better variable name? We could simplify it > further by: > > if (cus->GetComponentDetails(...)) { > ... > } Done.
lgtm
The CQ bit was checked by sorin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sorin@chromium.org/318143003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15705) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18838)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15750) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18882)
The CQ bit was checked by sorin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sorin@chromium.org/318143003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15761) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18893)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15765) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18897)
The CQ bit was checked by sorin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sorin@chromium.org/318143003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15771) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18903)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15817)
The CQ bit was checked by sorin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sorin@chromium.org/318143003/80001
Message was sent while issue was closed.
Change committed as 276561 |