Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10917119:
Dynamically link to deprecated APIs when running on OS 10.6 devices. (Closed)
|
Created:
8 years, 3 months ago by Wez Modified:
8 years, 3 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDynamically link to deprecated APIs when running on OS 10.6 devices.
BUG=139965
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155344
Patch Set 1 #Patch Set 2 : Also dynamic-link to CGLSetFullScreen API. #
Total comments: 10
Patch Set 3 : Address review comments. #Messages
Total messages: 11 (0 generated)
jamiewalch: Please review. thakis: Please check the dynamic-linking setup; specifically, is there a better way to generate the framework library names?
http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... File remoting/host/video_frame_capturer_mac.mm (right): http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... remoting/host/video_frame_capturer_mac.mm:540: // crbug.com/92354. Can this comment be updated? http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... remoting/host/video_frame_capturer_mac.mm:546: // Use deprecated APIs to determine the display buffer layout. // because the replacement APIs exist on 10.6, but crash when used. // (insert link to bug for that issue here) http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... remoting/host/video_frame_capturer_mac.mm:685: FilePath opengl_path(kOpenGlLibraryName); http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/surface/io_surface_supp... does something similar; they also hardcode the path. Maybe you should use a rooted path like they do.
lgtm http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... File remoting/host/video_frame_capturer_mac.mm (right): http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... remoting/host/video_frame_capturer_mac.mm:684: app_services_library_.Reset(base::LoadNativeLibrary(app_services_path, NULL)); Pass a string for error logging? http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... remoting/host/video_frame_capturer_mac.mm:728: (*cgl_set_full_screen_)(cgl_context_); I think it's worth preserving the TODO in some form, since the failure mode of the new API here is different.
http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... File remoting/host/video_frame_capturer_mac.mm (right): http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... remoting/host/video_frame_capturer_mac.mm:540: // crbug.com/92354. On 2012/09/06 21:12:19, Nico wrote: > Can this comment be updated? The changes in this CL don't affect whether we need to copy the previous buffer content, so I'm not sure what update you have in mind? http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... remoting/host/video_frame_capturer_mac.mm:546: // Use deprecated APIs to determine the display buffer layout. On 2012/09/06 21:12:19, Nico wrote: > // because the replacement APIs exist on 10.6, but crash when used. > // (insert link to bug for that issue here) This is the original capture code; we don't have a bug filed for the newer capture APIs being crashy. http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... remoting/host/video_frame_capturer_mac.mm:685: FilePath opengl_path(kOpenGlLibraryName); On 2012/09/06 21:12:19, Nico wrote: > http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/surface/io_surface_supp... > does something similar; they also hardcode the path. Maybe you should use a > rooted path like they do. Won't that break things if the locations of those libraries change in a future OS release if we hard-code paths?
On 2012/09/06 21:59:07, Wez wrote: > http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... > File remoting/host/video_frame_capturer_mac.mm (right): > > http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... > remoting/host/video_frame_capturer_mac.mm:540: // crbug.com/92354. > On 2012/09/06 21:12:19, Nico wrote: > > Can this comment be updated? > > The changes in this CL don't affect whether we need to copy the previous buffer > content, so I'm not sure what update you have in mind? > > http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... > remoting/host/video_frame_capturer_mac.mm:546: // Use deprecated APIs to > determine the display buffer layout. > On 2012/09/06 21:12:19, Nico wrote: > > // because the replacement APIs exist on 10.6, but crash when used. > > // (insert link to bug for that issue here) > > This is the original capture code; we don't have a bug filed for the newer > capture APIs being crashy. > > http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... > remoting/host/video_frame_capturer_mac.mm:685: FilePath > opengl_path(kOpenGlLibraryName); > On 2012/09/06 21:12:19, Nico wrote: > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/surface/io_surface_supp... > > does something similar; they also hardcode the path. Maybe you should use a > > rooted path like they do. > > Won't that break things if the locations of those libraries change in a future > OS release if we hard-code paths? This is only needed on 10.6. They won't change on 10.6.
On 2012/09/06 22:03:19, Nico wrote: > On 2012/09/06 21:59:07, Wez wrote: > > > http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... > > File remoting/host/video_frame_capturer_mac.mm (right): > > > > > http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... > > remoting/host/video_frame_capturer_mac.mm:540: // crbug.com/92354. > > On 2012/09/06 21:12:19, Nico wrote: > > > Can this comment be updated? > > > > The changes in this CL don't affect whether we need to copy the previous > buffer > > content, so I'm not sure what update you have in mind? > > > > > http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... > > remoting/host/video_frame_capturer_mac.mm:546: // Use deprecated APIs to > > determine the display buffer layout. > > On 2012/09/06 21:12:19, Nico wrote: > > > // because the replacement APIs exist on 10.6, but crash when used. > > > // (insert link to bug for that issue here) > > > > This is the original capture code; we don't have a bug filed for the newer > > capture APIs being crashy. > > > > > http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... > > remoting/host/video_frame_capturer_mac.mm:685: FilePath > > opengl_path(kOpenGlLibraryName); > > On 2012/09/06 21:12:19, Nico wrote: > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/surface/io_surface_supp... > > > does something similar; they also hardcode the path. Maybe you should use a > > > rooted path like they do. > > > > Won't that break things if the locations of those libraries change in a future > > OS release if we hard-code paths? > > This is only needed on 10.6. They won't change on 10.6. As far as I'm aware, though, there's no gain in providing the full 10.6 path, since the implicit load of the library will follow the library path? Plus it adds an ugly string line-wrap to the code... :P
On 2012/09/06 22:11:49, Wez wrote: > On 2012/09/06 22:03:19, Nico wrote: > > On 2012/09/06 21:59:07, Wez wrote: > > > > > > http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... > > > File remoting/host/video_frame_capturer_mac.mm (right): > > > > > > > > > http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... > > > remoting/host/video_frame_capturer_mac.mm:540: // crbug.com/92354. > > > On 2012/09/06 21:12:19, Nico wrote: > > > > Can this comment be updated? > > > > > > The changes in this CL don't affect whether we need to copy the previous > > buffer > > > content, so I'm not sure what update you have in mind? > > > > > > > > > http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... > > > remoting/host/video_frame_capturer_mac.mm:546: // Use deprecated APIs to > > > determine the display buffer layout. > > > On 2012/09/06 21:12:19, Nico wrote: > > > > // because the replacement APIs exist on 10.6, but crash when used. > > > > // (insert link to bug for that issue here) > > > > > > This is the original capture code; we don't have a bug filed for the newer > > > capture APIs being crashy. > > > > > > > > > http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... > > > remoting/host/video_frame_capturer_mac.mm:685: FilePath > > > opengl_path(kOpenGlLibraryName); > > > On 2012/09/06 21:12:19, Nico wrote: > > > > > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/surface/io_surface_supp... > > > > does something similar; they also hardcode the path. Maybe you should use > a > > > > rooted path like they do. > > > > > > Won't that break things if the locations of those libraries change in a > future > > > OS release if we hard-code paths? > > > > This is only needed on 10.6. They won't change on 10.6. > > As far as I'm aware, though, there's no gain in providing the full 10.6 path, > since the implicit load of the library will follow the library path? Plus it > adds an ugly string line-wrap to the code... :P My only concern would be potential library path attacks, especially since this code will run privileged at the login screen.
PTAL http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... File remoting/host/video_frame_capturer_mac.mm (right): http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... remoting/host/video_frame_capturer_mac.mm:684: app_services_library_.Reset(base::LoadNativeLibrary(app_services_path, NULL)); On 2012/09/06 21:41:50, Jamie wrote: > Pass a string for error logging? Seems overkill in this case, but OK... http://codereview.chromium.org/10917119/diff/3/remoting/host/video_frame_capt... remoting/host/video_frame_capturer_mac.mm:728: (*cgl_set_full_screen_)(cgl_context_); On 2012/09/06 21:41:50, Jamie wrote: > I think it's worth preserving the TODO in some form, since the failure mode of > the new API here is different. We'll never use the new API, though; it's crashy on 10.6 and deprecated in 10.7, and we don't capture with GL on 10.7 and above.
lgtm!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wez@chromium.org/10917119/8001
Change committed as 155344 |
