flutter

flutter copied to clipboard

Makes sure to call FreeLibrary even if GetProcAddress fails.

Pre-launch Checklist

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
• [x] I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• [x] I signed the CLA.
• [ ] I listed at least one issue that this PR fixes in the description above.
• [x] I updated/added relevant documentation (doc comments with ///).
• [ ] I added new tests to check the change I am making, or this PR is test-exempt.
• [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

cc @loic-sharma can you review this? As a non C++ dev, this looks like something we'd want :)

@cbracken Should we get a test exemption for this? We'd need to somehow check if there's a memory leak when creating a window on a machine that doesn't have EnableNonClientDpiScaling defined.

Alternately you could wrap the Win32 APIs with a DI-able object, and then test that you get the expected calls when you simulate various return values (using a mock version of that API wrapper). That's how we test the clipboard, for instance, and also how some of our Windows plugin unit tests work.

Thanks for looking at this!

Let me know about how you want to handle the test cases. I can give adding one a it a try, if you can give me some more concrete pointers in the right direction. I did this PR via the Github web-editor, and only because I happened to run across the example file in another project. In other words, I am far from fluent in this code base :).

Here's a very simple example of that pattern in one of the Windows plugins:

• Wrapper header and real implementation
• Test implementation and example test configuration

fwiw I don't think we need to do the above refactor in this PR. I'm fine landing as-is to get the library cleanup in. It would be straightforward to do the cleanup later, since it might involve some bikeshedding on our part :)

re tests: Thanks for the pointers, looks very reasonable! Let me know when you decide if a test is needed or not. I might need some additional help in where to place the tests if so.

Agreed this change is probably not strictly necessary, leaving the dll loaded is likely not a problem in practice. I still did this PR because this is part of sample code (as far as I understood), so there is some value in "doing the right thing", I think. Also because this was a very easy fix :smile:.

I also think a RAII wrapper is better. I did not do that initially because that would be a much bigger change, which I would feel uncomfortable doing as a first contribution. Will happily leave this to you people to clean up further!

One more thing. There seems to be multiple win32_window.cpp files in this repo. Would it make sense to update all/any of these in addition to the one I modified for this PR?

• dev/manual_tests/windows/runner/win32_window.cpp
• dev/integration_tests/flutter_gallery/windows/runner/win32_window.cpp
• examples/api/windows/runner/win32_window.cpp
• examples/platform_channel/windows/runner/win32_window.cp

I also think a RAII wrapper is better. I did not do that initially because that would be a much bigger change, which I would feel uncomfortable doing as a first contribution. Will happily leave this to you people to clean up further!

No worries at all. There's also a lot of value in keeping the code as simple and readable as possible. The RAII solution is something we'd definitely do in the engine, but I can definitely see the case for keeping it as a single FreeLibrary in the template. :)

One more thing. There seems to be multiple win32_window.cpp files in this repo. Would it make sense to update all/any of these in addition to the one I modified for this PR?

Good catch! Those are clones of this one. As part of our integration testing, we have a few instantiated apps checked into the repo. The code in those apps should but updated with updates to the template, like this patch does.

I made the same change to all win32_window.cpp in the repo. I also rebased on master to pickup some fix for what I think was an unrelated plugin test failure.