Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
30593007:
Implements basic listing of USB drives for windows. (Closed)
|
Created:
7 years, 2 months ago by Drew Haven Modified:
6 years, 10 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, stephenlin1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplements basic listing of USB drives for windows.
BUG=310328
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247892
Patch Set 1 #Patch Set 2 : Removes a few unnecessary lines. #
Total comments: 2
Patch Set 3 : Simplifies code to use helper function. #Patch Set 4 : Simplifies code to use helper function. #Patch Set 5 : Modifies to use the Win32 API. #Patch Set 6 : Reformats to remove c-style variable declaration. #
Total comments: 28
Patch Set 7 : Cleans up code based on review feedback. #
Total comments: 15
Patch Set 8 : Cleans up based on more review feedback. #Patch Set 9 : Updates scoped_ptr initializations to be more consistent in typing. #Messages
Total messages: 25 (0 generated)
Carlos, Could you either take a look at this CL or direct it to someone who has some time to review it? It's still a bit rough, so I'd love some more input to make sure I'm using the Windows idioms correctly. Next up is writing the actual image-writing code. -- Drew
Do we need WMI to be able to enumerate? WMI is kind of heavy handed. Adding Rick. https://codereview.chromium.org/30593007/diff/30001/chrome/browser/extensions... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc (right): https://codereview.chromium.org/30593007/diff/30001/chrome/browser/extensions... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:91: } any reason why not use WMI::CreateLocalConnection and the other helpers there?
On 2013/10/23 20:17:52, cpu wrote: > Do we need WMI to be able to enumerate? WMI is kind of heavy handed. > > Adding Rick. > > https://codereview.chromium.org/30593007/diff/30001/chrome/browser/extensions... > File > chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc > (right): > > https://codereview.chromium.org/30593007/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:91: > } > any reason why not use WMI::CreateLocalConnection and the other helpers there? I really wouldn't know any other way to do it. I was just assuming it was the right way based on the previous tool. I'll hold off editing for not until I know what direction to go in. I'll also look into those helpers. I think I skipped the helpers in my attempt to just get something working and understood with the raw calls before I started abstracting things. I'll take a look at those. Thanks again for reviewing.
you want to list the volumes or the drives? http://msdn.microsoft.com/en-us/library/aa364972%28VS.85%29.aspx http://msdn.microsoft.com/en-us/library/aa364975(v=vs.85).aspx then plus this http://msdn.microsoft.com/en-us/library/windows/desktop/aa364939(v=vs.85).aspx ?
On 2013/10/24 00:55:07, cpu wrote: > you want to list the volumes or the drives? > > http://msdn.microsoft.com/en-us/library/aa364972%2528VS.85%2529.aspx > > http://msdn.microsoft.com/en-us/library/aa364975%28v=vs.85%29.aspx > > then plus this > http://msdn.microsoft.com/en-us/library/windows/desktop/aa364939%28v=vs.85%29... > > ? I tried that, but I immediately discounted it because we need to be able to list non-mounted drives as well. I wanted to list available removable storage devices. Though maybe if they are unmounted they won't show up at all anyway. I might try that again. The disk I was using to test was already a disk with a recovery-image partition table, so it was only showing the one tiny partition that got mounted and not revealing the true nature of the disk immediately. However, if I can take the drive letters and map those up to the logical drive indexes I'm in the same place anyway. I'll see what the results look like.
I updated to use the helper function as requested. I've had a lot of trouble getting the GetLogicalDriveStrings to work. It's a pain because it gives back only logical drives and the data has to be massaged to get to physical disks. What I want to do is enumerate available disks. https://codereview.chromium.org/30593007/diff/30001/chrome/browser/extensions... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc (right): https://codereview.chromium.org/30593007/diff/30001/chrome/browser/extensions... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:91: } On 2013/10/23 20:17:52, cpu wrote: > any reason why not use WMI::CreateLocalConnection and the other helpers there? Done. I figured I shouldn't use that file since it was in the "installer" package which I assumed was only for the installer.
On 2013/10/24 21:25:55, Drew Haven wrote: > I updated to use the helper function as requested. > > I've had a lot of trouble getting the GetLogicalDriveStrings to work. It's a > pain because it gives back only logical drives and the data has to be massaged > to get to physical disks. What I want to do is enumerate available disks. > > https://codereview.chromium.org/30593007/diff/30001/chrome/browser/extensions... > File > chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc > (right): > > https://codereview.chromium.org/30593007/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:91: > } > On 2013/10/23 20:17:52, cpu wrote: > > any reason why not use WMI::CreateLocalConnection and the other helpers there? > > Done. > > I figured I shouldn't use that file since it was in the "installer" package > which I assumed was only for the installer. So, I have spent way too long trying to get through these Win32 APIs. The one thing I haven't been able to figure out is how to get the size. I've tried * GetFileAttributes -> Incorrect Function, * _wstat64 -> invalid file * CreateFile/IoControl methods -> access denied I may just need to wait on the privileged thread if I don't want to use WMI. Is it possible to move forward with this WMI implementation?
I figured out how to get this to work with the Win32 API. The key was setting the dwDesiredAccess parameter of CreateFile to 0. Sorry this took so long to get back to. Thanks for taking a look.
so you want a review now? My top level comment is to format the code in our style, we don't do C-style int foo; .. some code here... foo = DoWork();
On 2014/01/17 22:04:36, cpu wrote: > so you want a review now? > > My top level comment is to format the code in our style, we don't do C-style > > int foo; > .. some code here... > > foo = DoWork(); Yes, I was looking to get this reviewed. However, I wasn't aware of that and it seemed like the common Windows style so I did it anyway. I'll clean that up and then let you know. Sorry about that. Thanks for taking the time to take a look. I'll ping you again when it's ready.
On 2014/01/17 22:15:44, Drew Haven wrote: > On 2014/01/17 22:04:36, cpu wrote: > > so you want a review now? > > > > My top level comment is to format the code in our style, we don't do C-style > > > > int foo; > > .. some code here... > > > > foo = DoWork(); > > Yes, I was looking to get this reviewed. However, I wasn't aware of that and it > seemed like the common Windows style so I did it anyway. I'll clean that up and > then let you know. > > Sorry about that. Thanks for taking the time to take a look. I'll ping you > again when it's ready. Alright, I removed the c-style formatting. Mind taking a look when it's convenient?
https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc (right): https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:16: namespace { not a fan of the anonymous namespace inside a named namespace. I don't see what it gives you besides more nesting. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:18: bool AddDeviceInfo(HDEVINFO int_device_info, int_device_info seems to a bad name, either spell the whole word or remove it, device_info seems ok to me. 'int' has other meanings. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:26: NULL, // Device Info data period at the end of comments. Here and elsewhere. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:31: DWORD error_code; does not seem you need error_code as a stanalone variable. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:35: DVLOG(0) << "No more interfaces"; please remove dvlogs like this above. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:52: // This call returns ERROR_INSUFFICIENT_BUFFER with reqSize no such variable as reqSize. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:65: (PSP_DEVICE_INTERFACE_DETAIL_DATA) malloc(interface_detail_data_size); avoid c-style casting. Are you avoiding scoped_ptr and new for a particular reason? https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:81: PLOG(ERROR) << "SetupDiGetDeviceInterfaceDetail failed"; like for example we leak the malloc'ed data here and in some other places. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:125: (ULONG) geometry.BytesPerSector; more c-style casting. avoid. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:131: char output_buf[512]; I feel that better off with this being 'new char[1024]' or something like that. Again an scoped_ptr makes all this painless. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:149: (PSTORAGE_DEVICE_DESCRIPTOR) output_buf; c-style cast. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:160: // Windows strings are UTF16. remove obvious comment. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:181: remove this space. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:195: DWORD index = 0; index seems unused.
Okay, I did a lot of comment. I was so caught up in the windows samples I had been referencing that I just didn't think about switching it to use scoped_ptrs and C++ casts. Sorry about that. It should be much better now. One thing I'm still not sure about is using some of the function calls directly in the if-statements. For example, the call on 42 has its return value checked on 49. The function call is just so physically large that it makes it awkward to put in there. Thoughts? https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc (right): https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:16: namespace { On 2014/01/18 03:14:39, cpu wrote: > not a fan of the anonymous namespace inside a named namespace. I don't see what > it gives you besides more nesting. I was told it was preferred to avoid leaking symbols to a larger namespace. I can correct it though. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:18: bool AddDeviceInfo(HDEVINFO int_device_info, On 2014/01/18 03:14:39, cpu wrote: > int_device_info seems to a bad name, either spell the whole word or remove it, > device_info seems ok to me. 'int' has other meanings. Good point. Done. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:26: NULL, // Device Info data On 2014/01/18 03:14:39, cpu wrote: > period at the end of comments. Here and elsewhere. Done. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:31: DWORD error_code; On 2014/01/18 03:14:39, cpu wrote: > does not seem you need error_code as a stanalone variable. Done. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:35: DVLOG(0) << "No more interfaces"; On 2014/01/18 03:14:39, cpu wrote: > please remove dvlogs like this above. Done. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:52: // This call returns ERROR_INSUFFICIENT_BUFFER with reqSize On 2014/01/18 03:14:39, cpu wrote: > no such variable as reqSize. This comment was silly anyway. I just removed it. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:65: (PSP_DEVICE_INTERFACE_DETAIL_DATA) malloc(interface_detail_data_size); On 2014/01/18 03:14:39, cpu wrote: > avoid c-style casting. Are you avoiding scoped_ptr and new for a particular > reason? Only because I got into bad habits digging through Windows examples. Sorry about that. Time to modernize. This one is a bit weird though because theoretically the size of the structure could change and so the API is built for this double-call mechanism. However, it's not something that I expect has ever changed, but it still means we can't get away with using new, can we? https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:81: PLOG(ERROR) << "SetupDiGetDeviceInterfaceDetail failed"; On 2014/01/18 03:14:39, cpu wrote: > like for example we leak the malloc'ed data here and in some other places. Right. scoped_ptr to the rescue. I'm still not entirely used to using smart pointers. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:125: (ULONG) geometry.BytesPerSector; On 2014/01/18 03:14:39, cpu wrote: > more c-style casting. avoid. Done. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:131: char output_buf[512]; On 2014/01/18 03:14:39, cpu wrote: > I feel that better off with this being 'new char[1024]' or something like that. > Again an scoped_ptr makes all this painless. Done. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:149: (PSTORAGE_DEVICE_DESCRIPTOR) output_buf; On 2014/01/18 03:14:39, cpu wrote: > c-style cast. Done. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:160: // Windows strings are UTF16. On 2014/01/18 03:14:39, cpu wrote: > remove obvious comment. Done. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:181: On 2014/01/18 03:14:39, cpu wrote: > remove this space. Done. https://codereview.chromium.org/30593007/diff/220001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:195: DWORD index = 0; On 2014/01/18 03:14:39, cpu wrote: > index seems unused. SetupDiEnumDeviceInterfaces takes an index variable to look up the interface by its index in device info enumerator. Perhaps I should pull that function call up a level so that it is more clear.
https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc (right): https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:35: I think the usage is incorrect, scoped_ptr will call delete not free. See scoped_ptr<C, base::FreeDeleter> in the header. you can also call new char[size] and cast that. https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:55: HANDLE device_handle = CreateFile( looks like we leak the device_handle ? https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:74: device_handle, // Device handle indent comment of 74 like 75, so it looks pretty. also needs periods here and elsewhere. https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:94: STORAGE_PROPERTY_QUERY query; STORAGE_PROPERTY_QUERY query = {0} ? or is it only two fields? https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:98: scoped_ptr<char[]> output_buf(new char[1024]); scoped_ptr<char> I believe. https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:149: NULL, // Enumerator periods on the comments, or remove the comments, your choice.
Sorry about forgetting to follow through on the periods. I think I got everything taken care of. I'm a little confused about the error I got initializing that struct, but I was able to use the constructor-style initialization without a problem. https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc (right): https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:35: On 2014/01/23 22:46:32, cpu wrote: > I think the usage is incorrect, scoped_ptr will call delete not free. > See scoped_ptr<C, base::FreeDeleter> in the header. > > you can also call new char[size] and cast that. > > Ah, it's the size thing that was confusing me with the use of new. I'll cast a new char[] then. It seems the most common. https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:55: HANDLE device_handle = CreateFile( On 2014/01/23 22:46:32, cpu wrote: > looks like we leak the device_handle ? Oh, you're right. I thought thinking of it as a value-type, but it should be closed. It looks like I can use base::win::ScopedHandle. https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:74: device_handle, // Device handle On 2014/01/23 22:46:32, cpu wrote: > indent comment of 74 like 75, so it looks pretty. also needs periods here and > elsewhere. Done. https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:94: STORAGE_PROPERTY_QUERY query; On 2014/01/23 22:46:32, cpu wrote: > STORAGE_PROPERTY_QUERY query = {0} ? > or is it only two fields? There's an extra one-byte field of extra parameters that I think is actually unused. But it's a good idea to initialize it. I tried initializing it with {0} but oddly that gave me an error tha tit could convert from int to the STORAGE_PROPERTY_ID enum, so does it work to do constructor-style initialization? https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:98: scoped_ptr<char[]> output_buf(new char[1024]); On 2014/01/23 22:46:32, cpu wrote: > scoped_ptr<char> I believe. It apparently used to be scoped_array, but now there are just two forms of scoped_ptr. <char> would use new/delete, <char[]> will use new[]/delete[]. https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:149: NULL, // Enumerator On 2014/01/23 22:46:32, cpu wrote: > periods on the comments, or remove the comments, your choice. Whoops, I started doing this and forgot to complete it. Done.
Looking good now. https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc (right): https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:98: scoped_ptr<char[]> output_buf(new char[1024]); Ok, then the line 37 one is mismatched? On 2014/01/23 23:58:51, Drew Haven wrote: > On 2014/01/23 22:46:32, cpu wrote: > > scoped_ptr<char> I believe. > > It apparently used to be scoped_array, but now there are just two forms of > scoped_ptr. <char> would use new/delete, <char[]> will use new[]/delete[].
I put in a rather long winded explanation about the pointer types. I really am not sure what the best way to deal with it is. It feels awkward as it is primarily a C API. https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc (right): https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:98: scoped_ptr<char[]> output_buf(new char[1024]); The issue here is tat the output of DeviceIoControl seems to be implemented as filling the buffer with null-terminated strings preceded by a header that has their offsets. So the returned data has a variable size and requires we treat it both as a struct and as a char-array for different offsets. Up above we just have a struct that could have different sizes on different windows implementations (though it looks like it has never been changed yet) and it uses the structure size as a versioning tool and we request the size before making the call so that we'll have whatever size the API wants. So I feel like the question here is whether we have our pointer to the buffer be a char-array and cast the header to the correct struct, or hold the pointer to the buffer as a struct and then cast to a char-array when we read at the offsets beyond the struct. So I guess the question is which way to store it. The function takes a void* pointer so it seemed to just treat it as an array of bytes made the most sense. On 2014/01/24 20:37:50, cpu wrote: > Ok, then the line 37 one is mismatched? > > On 2014/01/23 23:58:51, Drew Haven wrote: > > On 2014/01/23 22:46:32, cpu wrote: > > > scoped_ptr<char> I believe. > > > > It apparently used to be scoped_array, but now there are just two forms of > > scoped_ptr. <char> would use new/delete, <char[]> will use new[]/delete[]. >
https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc (right): https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:98: scoped_ptr<char[]> output_buf(new char[1024]); Yeah I think the smart pointer needs to be of the type of the new so scoped_ptr<T> p = new T or so scoped_ptr<T[]> p = new T[x] then you can do K k = reinterpet_cast<K*>(p.Get()) and use k;
On 2014/01/28 00:58:49, cpu wrote: > https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... > File > chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc > (right): > > https://codereview.chromium.org/30593007/diff/270001/chrome/browser/extension... > chrome/browser/extensions/api/image_writer_private/removable_storage_provider_win.cc:98: > scoped_ptr<char[]> output_buf(new char[1024]); > Yeah I think the smart pointer needs to be of the type of the new > > so scoped_ptr<T> p = new T or so scoped_ptr<T[]> p = new T[x] > then you can do K k = reinterpet_cast<K*>(p.Get()) and use k; Alright, that makes good sense. Sounds good to make sure that the scoped_ptrs are clear about how the memory is actually represented in the request, but then we can reinterpret it later. Done.
lgtm
On 2014/01/29 22:14:17, cpu wrote: > lgtm Thanks for the review. I learned something. :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haven@chromium.org/30593007/370001
CQ bit was unchecked on CL. Ignoring.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haven@chromium.org/30593007/370001
Message was sent while issue was closed.
Change committed as 247892 |
