Avoid using non-UTF-8 encoded argv more often#36590
Conversation
|
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. |
fml/command_line.h
Outdated
| // Builds a |CommandLine| from the usual argc/argv. | ||
| // | ||
| // First checks if CommandLineFromPlatform returns a value, to avoid using an | ||
| // argv that is not provided as UTF-8. |
There was a problem hiding this comment.
tester_main.cc uses this method, and now will pick up the UTF-8 encoded args on windows.
There was a problem hiding this comment.
I think this will break embedders who provide argc and argv explicitly.
There was a problem hiding this comment.
Here
engine/shell/platform/embedder/embedder.cc
Line 1331 in e6e94e4
There was a problem hiding this comment.
It would mean we ignore the specified argc/argv on Windows and instead use the platform based method. I'm not really clear on how that's used today, but I suspect that embedders on Windows doing this are currently broken unless they're already using this API.
I suppose this could be a problem if the embedder is specifying values for argc/argv that weren't already passed in to the process though...
There was a problem hiding this comment.
I'll refactor this to a separate method though to avoid disrupting the embedder API.
|
The test for this is in the linked bug. |
|
The declaration for Otherwise LGTM Verified that |
|
Kicked off and tried to patch in both the gerrit utf-8 and this PR. I think I did it correctly, build is here: https://chromium-swarm.appspot.com/task?id=5dbce228851c7310 |
|
@jason-simmons - I rearranged the methods @chinmaygarde - I added a new method and updated all the callsites except embedder.cc and one unit test explicitly testing the old method. |
|
Ahh that task is trying to pull from the framework instead of the engine, so it won't work. |
|
@sealesj - once this rolls through to the framework you should be able to try again. |
Should fix flutter/flutter#112819
I have not tested this on a windows machine though. @sealesj would you be able to kick off a LED run with this patched in for one of your failing windows tests?