Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2570563002:
Use LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl (Closed)
|
Created:
4 years ago by Patrick Monette Modified:
4 years ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl
As per Microsoft's recommendation for LoadLibraryEx on MSDN.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx
"Use this flag when you want to load a DLL only to extract messages or
resources from it."
Committed: https://crrev.com/0feb351d0498b3db22790162b43e096da8f67df8
Cr-Commit-Position: refs/heads/master@{#438867}
Patch Set 1 #
Messages
Total messages: 23 (11 generated)
Description was changed from ========== Use LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl ========== to ========== Use LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl As per Microsoft's recommendation. ==========
Description was changed from ========== Use LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl As per Microsoft's recommendation. ========== to ========== Use LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl As per Microsoft's recommendation for LoadLibraryEx on MSDN. ==========
pmonette@chromium.org changed reviewers: + jochen@chromium.org
Hi, PTAL Greg suggested to do that in a previous CL but I forgot to do it.
jochen@chromium.org changed reviewers: + scottmg@chromium.org
deferring to Scott Can you please link to the MSDN article from the CL description? also, what's the impact of doing this?
scottmg@chromium.org changed reviewers: + grt@chromium.org
I guess Greg probably knows better why he asked for it. :) From looking at the Remarks of https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx it sounds like it doesn't do anything if shell32 is already loaded, which I imagine there's a good chance it is?
Description was changed from ========== Use LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl As per Microsoft's recommendation for LoadLibraryEx on MSDN. ========== to ========== Use LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl As per Microsoft's recommendation for LoadLibraryEx on MSDN. https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx ==========
While it has no effect on this particular scenario because shell32.dll is already loaded as a normal module, I think it's good to use the right semantics for the task at hand, just to make the code clearer. The documentation states to "Use this flag when you want to load a DLL only to extract messages or resources from it.", which is what the code does there. I guess it could have a potential impact if this gets refactored differently or someone copy-pastes the code in a different context. I can drop this change if you think it's not worth it. I don't have strong feeling about this!
Description was changed from ========== Use LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl As per Microsoft's recommendation for LoadLibraryEx on MSDN. https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx ========== to ========== Use LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl As per Microsoft's recommendation for LoadLibraryEx on MSDN. https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx "Use this flag when you want to load a DLL only to extract messages or resources from it." ==========
Sure. lgtm
On 2016/12/14 20:15:56, Patrick Monette wrote: > While it has no effect on this particular scenario because shell32.dll is > already loaded as a normal module, I think it's good to use the right semantics > for the task at hand, just to make the code clearer. You took the words right out of my mouth. :-)
lgtm
Thanks!
The CQ bit was checked by pmonette@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1481820289324970, "parent_rev":
"743eae8469a9af62193a5cd3271d8f0142fa9830", "commit_rev":
"5ebc094761ae1ae93aa65b493c0c247fe548d109"}
Message was sent while issue was closed.
Description was changed from ========== Use LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl As per Microsoft's recommendation for LoadLibraryEx on MSDN. https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx "Use this flag when you want to load a DLL only to extract messages or resources from it." ========== to ========== Use LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl As per Microsoft's recommendation for LoadLibraryEx on MSDN. https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx "Use this flag when you want to load a DLL only to extract messages or resources from it." Review-Url: https://codereview.chromium.org/2570563002 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Use LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl As per Microsoft's recommendation for LoadLibraryEx on MSDN. https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx "Use this flag when you want to load a DLL only to extract messages or resources from it." Review-Url: https://codereview.chromium.org/2570563002 ========== to ========== Use LOAD_LIBRARY_AS_IMAGE_RESOURCE to open shell32.dll in ShellHandlerImpl As per Microsoft's recommendation for LoadLibraryEx on MSDN. https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx "Use this flag when you want to load a DLL only to extract messages or resources from it." Committed: https://crrev.com/0feb351d0498b3db22790162b43e096da8f67df8 Cr-Commit-Position: refs/heads/master@{#438867} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/0feb351d0498b3db22790162b43e096da8f67df8 Cr-Commit-Position: refs/heads/master@{#438867} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
