Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11712003:
[Fixit-Dec-2012] Also add dual_mode to Start Menu shortcuts in MigrateChromiumShortcuts. (Closed)
|
Created:
7 years, 12 months ago by gab Modified:
7 years, 11 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Fixit-Dec-2012] Also add dual_mode to Start Menu shortcuts in MigrateChromiumShortcuts.
Restructured this code quite a bit to use the new shortcut magic.
Added tests which turned out to expose edge cases that I think weren't covered by the previous implementation (i.e. would potentially update shortcuts when unecessary -- flashing the desktop in the process -- or not do it when necessary).
NOTRY=TRUE
(Actually dis use CQ at first, but iOS is messes up at LKGR looks like...)
BUG=142980
TEST=Pin Desktop shortcut to Start Screen or pin chrome.exe directly to the start screen via context menu.
Notice that there are now multiple shortcuts of Chrome (non-tiled; 1 for each "Pin to Start" action; upon launching Chrome those should all be merged down to one Chrome tile shortcut (not on the file system, but visually on the Start Screen itself).
Note: if you do try this; there is an intentional 15s delay before the migration kicks in to avoid delaying Chrome startup; so be patient, it will work ;)!
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175230
Reverted in: https://src.chromium.org/viewvc/chrome?view=rev&revision=175244
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175302
Patch Set 1 #Patch Set 2 : small tweaks #
Total comments: 34
Patch Set 3 : grt++ #
Total comments: 9
Patch Set 4 : add space to test shortcut paths #
Total comments: 2
Patch Set 5 : change aligment #Patch Set 6 : DelayLoad propsys.dll #
Messages
Total messages: 25 (0 generated)
Sir, sending your way since you volunteered for fixit reviews, it's been a couple I send your way already though so feel free to deflect this one to Robert :). Thanks! Gab
https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration.h:28: int MigrateShortcutsInPath( Name this MigrateShortcutsInPathInternal, add to the comment that it's only public for the sake of tests, and move it down just after MigrateChromiumShortcuts (in the #if defined(OS_WIN) block). (move the impl accordingly, too.) https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:92: // |is_per_user_install|. nit: close-paren at the end of the sentence. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:93: void GetExpectedAppId(const CommandLine& command_line, string16 is a value type, so return the expected id rather than using an out param. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:206: BOOL existing_dual_mode; move down to line 272/3 https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:230: NOTREACHED(); it seems wrong for chrome to crash if there's an invalid .lnk file in |path|. i suggest DLOG(WARNING) << "Failed loading shortcut at " << shortcut.value(); or something like that. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:234: // Any properties that needs to be updated on the shortcut will be stored in needs -> need https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:238: // Get existing app id from shortcut if any. "Get the existing app id from the shortcut." https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:242: // return S_OK and |pv_app_id| will be set to VT_EMPTY which will in turn be please explicitly test for VT_EMPTY rather than use PropVariantToString. the latter is overkill for your needs here. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:245: PROPVARIANT pv_app_id; please continue to use PropVariantInit and PropVariantClear (see the previous code). https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:248: // When in doubt, prefer not updating the shortcut. "not updating" -> "to not update" https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:268: // Note, similar to the behavior described above, if same comment about checking for VT_EMPTY instead of using PropVariantTo* https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:276: // When in doubt, prefer not updating the shortcut. "not updating" -> "to not update" https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:287: // Update the shortcut iff some of its properties need to be updated. iff -> if (this is becoming a pet peeve of mine. :-) ) https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... File chrome/browser/shell_integration_win_unittest.cc (right): https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win_unittest.cc:30: file_util::CreateTemporaryFile(&other_target); could these files be placed in temp_dir_ so they're not leaked (there's a function for that in file_util)? https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win_unittest.cc:94: FilePath shortcuts_[kNumTestShortcuts]; suggest making this a vector so you don't need kNumTestShortcuts and its pitfalls. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win_unittest.cc:97: base::win::ShortcutProperties shortcuts_properties_[kNumTestShortcuts]; vectorize this, too?
Robert, can you take this review over from grt? Thanks :)! Gab https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration.h:28: int MigrateShortcutsInPath( On 2013/01/03 02:30:44, grt wrote: > Name this MigrateShortcutsInPathInternal, add to the comment that it's only > public for the sake of tests, and move it down just after > MigrateChromiumShortcuts (in the #if defined(OS_WIN) block). (move the impl > accordingly, too.) Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:92: // |is_per_user_install|. On 2013/01/03 02:30:44, grt wrote: > nit: close-paren at the end of the sentence. Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:93: void GetExpectedAppId(const CommandLine& command_line, On 2013/01/03 02:30:44, grt wrote: > string16 is a value type, so return the expected id rather than using an out > param. Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:206: BOOL existing_dual_mode; On 2013/01/03 02:30:44, grt wrote: > move down to line 272/3 Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:230: NOTREACHED(); On 2013/01/03 02:30:44, grt wrote: > it seems wrong for chrome to crash if there's an invalid .lnk file in |path|. i > suggest DLOG(WARNING) << "Failed loading shortcut at " << shortcut.value(); or > something like that. Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:234: // Any properties that needs to be updated on the shortcut will be stored in On 2013/01/03 02:30:44, grt wrote: > needs -> need Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:238: // Get existing app id from shortcut if any. On 2013/01/03 02:30:44, grt wrote: > "Get the existing app id from the shortcut." Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:242: // return S_OK and |pv_app_id| will be set to VT_EMPTY which will in turn be On 2013/01/03 02:30:44, grt wrote: > please explicitly test for VT_EMPTY rather than use PropVariantToString. the > latter is overkill for your needs here. Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:245: PROPVARIANT pv_app_id; On 2013/01/03 02:30:44, grt wrote: > please continue to use PropVariantInit and PropVariantClear (see the previous > code). Done, but I don't see why those are necessary; I have not been using them in similar code in test_shortcut_win.cc, nor do I see any recommendations to use them on MSDN. If not calling PropvariantClear results in a potential memory leak, then test_shortcut_win.cc should also be fixed. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:248: // When in doubt, prefer not updating the shortcut. On 2013/01/03 02:30:44, grt wrote: > "not updating" -> "to not update" Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:268: // Note, similar to the behavior described above, if On 2013/01/03 02:30:44, grt wrote: > same comment about checking for VT_EMPTY instead of using PropVariantTo* Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:276: // When in doubt, prefer not updating the shortcut. On 2013/01/03 02:30:44, grt wrote: > "not updating" -> "to not update" Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:287: // Update the shortcut iff some of its properties need to be updated. On 2013/01/03 02:30:44, grt wrote: > iff -> if > (this is becoming a pet peeve of mine. :-) ) Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... File chrome/browser/shell_integration_win_unittest.cc (right): https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win_unittest.cc:30: file_util::CreateTemporaryFile(&other_target); On 2013/01/03 02:30:44, grt wrote: > could these files be placed in temp_dir_ so they're not leaked (there's a > function for that in file_util)? Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win_unittest.cc:94: FilePath shortcuts_[kNumTestShortcuts]; On 2013/01/03 02:30:44, grt wrote: > suggest making this a vector so you don't need kNumTestShortcuts and its > pitfalls. Done. https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win_unittest.cc:97: base::win::ShortcutProperties shortcuts_properties_[kNumTestShortcuts]; On 2013/01/03 02:30:44, grt wrote: > vectorize this, too? Done.
https://codereview.chromium.org/11712003/diff/19001/chrome/browser/shell_inte... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11712003/diff/19001/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:397: L"\"%ls\" %ls", target_path.value().c_str(), arguments.c_str()))); Just to double check, is there any way that target_path.value() would already be quoted? https://codereview.chromium.org/11712003/diff/19001/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:470: PropVariantClear(&pv_dual_mode); Should we not clear the PropVariant in the if branch immediately above? Don't have to do this in this CL, but this looks to me to be crying out for a ScopedPropVariant. There's the beginnings of one in jumplist_win.cc that could perhaps be used or moved. https://codereview.chromium.org/11712003/diff/19001/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:481: base::win::CreateOrUpdateShortcutLink( minor nit: DoFooToShortcutLink reads a little bit like DoFooToDirectoryFolder, are the words "Shortcut" and "Link" redundant here? If you feel like changing it, don't have to do it in this CL if it would interfere with merging.
Thanks, see replies below. https://codereview.chromium.org/11712003/diff/19001/chrome/browser/shell_inte... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11712003/diff/19001/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:397: L"\"%ls\" %ls", target_path.value().c_str(), arguments.c_str()))); On 2013/01/03 22:41:59, robertshield wrote: > Just to double check, is there any way that target_path.value() would already be > quoted? Good question, this code was already there, I find it ugly, but didn't bother touching that part... MSDN says this about IShellLink.GetPath(): (...) receives the path and file name of the target of the Shell link object. I doubt this would be quoted... FWIW, I added a space to the path of the tested shortcuts and they still pass (I even added a dump locally to be sure the path returned with a space had no quotes). https://codereview.chromium.org/11712003/diff/19001/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:470: PropVariantClear(&pv_dual_mode); On 2013/01/03 22:41:59, robertshield wrote: > Should we not clear the PropVariant in the if branch immediately above? > > Don't have to do this in this CL, but this looks to me to be crying out for a > ScopedPropVariant. There's the beginnings of one in jumplist_win.cc that could > perhaps be used or moved. Right, MSDN is not clear about when clearing is required or not... I agree that a ScopedPropVariant would be nice... added it to the installer fixit list. https://docs.google.com/a/chromium.org/document/d/1Js17FLROy5L83Q8Qp9G6l6hRq1... https://codereview.chromium.org/11712003/diff/19001/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:481: base::win::CreateOrUpdateShortcutLink( On 2013/01/03 22:41:59, robertshield wrote: > minor nit: DoFooToShortcutLink reads a little bit like DoFooToDirectoryFolder, > are the words "Shortcut" and "Link" redundant here? If you feel like changing > it, don't have to do it in this CL if it would interfere with merging. I agree that this name is redundant, added this to the fixit as well, but it touches base so I really don't want to do this in this CL.
lgtm, please double check a shortcut with a quoted target to make sure things don't break. https://codereview.chromium.org/11712003/diff/19001/chrome/browser/shell_inte... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11712003/diff/19001/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:397: L"\"%ls\" %ls", target_path.value().c_str(), arguments.c_str()))); On 2013/01/03 23:12:43, gab wrote: > On 2013/01/03 22:41:59, robertshield wrote: > > Just to double check, is there any way that target_path.value() would already > be > > quoted? > > Good question, this code was already there, I find it ugly, but didn't bother > touching that part... > > MSDN says this about IShellLink.GetPath(): (...) receives the path and file name > of the target of the Shell link object. > > I doubt this would be quoted... > > FWIW, I added a space to the path of the tested shortcuts and they still pass (I > even added a dump locally to be sure the path returned with a space had no > quotes). For peace of mind, could you edit the shortcut in explorer to manually add quotes to the target and see what happens? https://codereview.chromium.org/11712003/diff/19001/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:470: PropVariantClear(&pv_dual_mode); On 2013/01/03 23:12:43, gab wrote: > On 2013/01/03 22:41:59, robertshield wrote: > > Should we not clear the PropVariant in the if branch immediately above? > > > > Don't have to do this in this CL, but this looks to me to be crying out for a > > ScopedPropVariant. There's the beginnings of one in jumplist_win.cc that could > > perhaps be used or moved. > > Right, MSDN is not clear about when clearing is required or not... I agree that > a ScopedPropVariant would be nice... added it to the installer fixit list. > > https://docs.google.com/a/chromium.org/document/d/1Js17FLROy5L83Q8Qp9G6l6hRq1... sgtm
Thanks, will verify quoted path tomorrow to make sure, but at the very least this can't be a regression as this hasn't changed from the previous code. @brettw for OWNER approval. Thanks, Gab
@robertshield: FYI, see below. Still @brettw for OWNER approval. Thanks! Gab https://codereview.chromium.org/11712003/diff/19001/chrome/browser/shell_inte... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11712003/diff/19001/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:397: L"\"%ls\" %ls", target_path.value().c_str(), arguments.c_str()))); On 2013/01/04 01:38:14, robertshield wrote: > On 2013/01/03 23:12:43, gab wrote: > > On 2013/01/03 22:41:59, robertshield wrote: > > > Just to double check, is there any way that target_path.value() would > already > > be > > > quoted? > > > > Good question, this code was already there, I find it ugly, but didn't bother > > touching that part... > > > > MSDN says this about IShellLink.GetPath(): (...) receives the path and file > name > > of the target of the Shell link object. > > > > I doubt this would be quoted... > > > > FWIW, I added a space to the path of the tested shortcuts and they still pass > (I > > even added a dump locally to be sure the path returned with a space had no > > quotes). > > For peace of mind, could you edit the shortcut in explorer to manually add > quotes to the target and see what happens? I just tried and this is not even doable in Explorer, i.e. the quotes added to the target path are removed as soon as I click "Ok" or "Apply". I think we're safe :).
lgtm https://codereview.chromium.org/11712003/diff/23004/chrome/browser/shell_inte... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/11712003/diff/23004/chrome/browser/shell_inte... chrome/browser/shell_integration.h:166: static int MigrateShortcutsInPathInternal( I'd align all these after the ( on different lines.
Thanks! https://codereview.chromium.org/11712003/diff/23004/chrome/browser/shell_inte... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/11712003/diff/23004/chrome/browser/shell_inte... chrome/browser/shell_integration.h:166: static int MigrateShortcutsInPathInternal( On 2013/01/04 21:04:08, brettw wrote: > I'd align all these after the ( on different lines. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/11712003/29001
Message was sent while issue was closed.
Change committed as 175230
Adding a delay load to propsys.dll, like we already do at http://code.google.com/searchframe#OAMlx_jo-ck/src/base/base.gyp&exact_packag... Reverted in https://codereview.chromium.org/11777006/ because it broke tests (propsys.dll doesn't exist on XP; but this method is only actually called on Win7+). PTAL, will commit to see if it fixes it, but feel free to disagree and I can fix it after the commit. Thanks, Gab
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/11712003/28002
Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/11712003/28002
Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/11712003/28002
Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/11712003/28002
Message was sent while issue was closed.
Change committed as 175302
Message was sent while issue was closed.
https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:242: // return S_OK and |pv_app_id| will be set to VT_EMPTY which will in turn be On 2013/01/03 21:14:57, gab wrote: > On 2013/01/03 02:30:44, grt wrote: > > please explicitly test for VT_EMPTY rather than use PropVariantToString. the > > latter is overkill for your needs here. > > Done. Why did you mark this "Done" but not actually do it? I think that PropVariantTo* is overkill, and adding more delayloads for unnecessary convenience code (it only adds overhead) isn't good practice. The code you have here is quite a bit more complex and involves extra allocations, string copies, and error handling compare to what was here before. In what way is this an improvement?
Message was sent while issue was closed.
https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:242: // return S_OK and |pv_app_id| will be set to VT_EMPTY which will in turn be On 2013/01/07 15:23:22, grt wrote: > On 2013/01/03 21:14:57, gab wrote: > > On 2013/01/03 02:30:44, grt wrote: > > > please explicitly test for VT_EMPTY rather than use PropVariantToString. the > > > latter is overkill for your needs here. > > > > Done. > > Why did you mark this "Done" but not actually do it? Hmm?! I did do this... i.e. check for VT_EMPTY. If what you meant is to not use PropVariantTo* altogether that's not what I gathered from your comment. However after having to add DelayLoad I agree that this seems overkill, I was going to talk with you guys today to see if you feel that directly looking in the PropVariant (as was done before) is safe since the PropVariantTo* methods unfortunately imply this undesired overhead (seems to me it would be safe, but I don't know if PropVariantTo* does some clever things I'm not aware of...). >I think that PropVariantTo* > is overkill, and adding more delayloads for unnecessary convenience code (it > only adds overhead) isn't good practice. The code you have here is quite a bit > more complex and involves extra allocations, string copies, and error handling > compare to what was here before. In what way is this an improvement?
Message was sent while issue was closed.
On 2013/01/07 15:37:09, gab wrote: > https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... > File chrome/browser/shell_integration_win.cc (right): > > https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... > chrome/browser/shell_integration_win.cc:242: // return S_OK and |pv_app_id| will > be set to VT_EMPTY which will in turn be > On 2013/01/07 15:23:22, grt wrote: > > On 2013/01/03 21:14:57, gab wrote: > > > On 2013/01/03 02:30:44, grt wrote: > > > > please explicitly test for VT_EMPTY rather than use PropVariantToString. > the > > > > latter is overkill for your needs here. > > > > > > Done. > > > > Why did you mark this "Done" but not actually do it? > > Hmm?! I did do this... i.e. check for VT_EMPTY. > > If what you meant is to not use PropVariantTo* altogether that's not what I > gathered from your comment. Ah, that's precisely what I meant. Sorry I wasn't more clear. > However after having to add DelayLoad I agree that > this seems overkill, I was going to talk with you guys today to see if you feel > that directly looking in the PropVariant (as was done before) is safe since the > PropVariantTo* methods unfortunately imply this undesired overhead (seems to me > it would be safe, but I don't know if PropVariantTo* does some clever things I'm > not aware of...). Yeah, it's safe. PropVariantTo* is useful if you have no idea what the inbound PV is. In this case, the type of the inbound PV is known.
Message was sent while issue was closed.
On 2013/01/07 15:43:06, grt wrote: > On 2013/01/07 15:37:09, gab wrote: > > > https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... > > File chrome/browser/shell_integration_win.cc (right): > > > > > https://codereview.chromium.org/11712003/diff/8001/chrome/browser/shell_integ... > > chrome/browser/shell_integration_win.cc:242: // return S_OK and |pv_app_id| > will > > be set to VT_EMPTY which will in turn be > > On 2013/01/07 15:23:22, grt wrote: > > > On 2013/01/03 21:14:57, gab wrote: > > > > On 2013/01/03 02:30:44, grt wrote: > > > > > please explicitly test for VT_EMPTY rather than use PropVariantToString. > > the > > > > > latter is overkill for your needs here. > > > > > > > > Done. > > > > > > Why did you mark this "Done" but not actually do it? > > > > Hmm?! I did do this... i.e. check for VT_EMPTY. > > > > If what you meant is to not use PropVariantTo* altogether that's not what I > > gathered from your comment. > > Ah, that's precisely what I meant. Sorry I wasn't more clear. > > > However after having to add DelayLoad I agree that > > this seems overkill, I was going to talk with you guys today to see if you > feel > > that directly looking in the PropVariant (as was done before) is safe since > the > > PropVariantTo* methods unfortunately imply this undesired overhead (seems to > me > > it would be safe, but I don't know if PropVariantTo* does some clever things > I'm > > not aware of...). > > Yeah, it's safe. PropVariantTo* is useful if you have no idea what the inbound > PV is. In this case, the type of the inbound PV is known. Sounds good, doing this in https://codereview.chromium.org/11786005/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
