-
Notifications
You must be signed in to change notification settings - Fork 28.9k
feat(tool): Respect user-data-dir flag from web-browser-flag #169445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat(tool): Respect user-data-dir flag from web-browser-flag #169445
Conversation
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. 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.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
This should ideally be tested. Can you add a test to make sure this flag is respected properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if it LGT @bkonyi
…d rearranged variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
There's some analysis and formatting failures that need to be addressed before this can land:
info • The imported package 'collection' isn't a dependency of the importing package • packages/flutter_tools/lib/src/web/chrome.dart:7:8 • depend_on_referenced_packages
info • Use 'const' with the constructor to improve performance • packages/flutter_tools/test/web.shard/chrome_test.dart:942:7 • prefer_const_constructors
info • Use 'const' literals as arguments to constructors of '@immutable' classes • packages/flutter_tools/test/web.shard/chrome_test.dart:943:18 • prefer_const_literals_to_create_immutables
@reynaldots, just checking in. Do you still plan on moving forward with this PR? |
@bkonyi Yes! I've been having a crazy month releasing an app, but this weekend I'm going to refactor it. And when analysis sad: info • The imported package 'collection' isn't a dependency of the importing package • packages/flutter_tools/lib/src/web/chrome.dart:7:8 • depend_on_referenced_packages What should i do? Because firstWhereOrNull uses that package. |
Not a problem! Just wanted to make sure you had intentions of coming back to this :) To fix the formatting issue, you should just have to run For the |
Hi @bkonyi! I finished the corrections. |
Respect user-data-dir flag from web-browser-flag
Currently, it's already possible to pass
web-browser-flag
when launching Chrome, but theuser-data-dir
flag doesn't work as expected, and there are some reasons for that.In the implementation made in PR #104935, the
web-browser-flag
is appended at the end of the Chrome launch arguments.For most scenarios, this works fine, as demonstrated in the Chrome unit test below:
https://source.chromium.org/chromium/chromium/src/+/main:base/command_line_unittest.cc
In this scenario, the parser will consider the last occurrence of a flag.
However, this behavior does not apply to certain flags, because Chrome processes some of them based on the first occurrence, not the last. This is the case for
--user-data-dir
, which is parsed very early during Chrome startup.The proposed code checks whether
--user-data-dir
was provided viaweb-browser-flag
, and if so, it uses that value instead of the default%temp%\flutter_tools_chrome_device.xpto
temporary directory.This also resolve this comment:
#104935 (comment)
Example:
launch.json
chrome://version
Folder
Pre-launch Checklist
//github.com/
).