feat(tool): Respect user-data-dir flag from web-browser-flag #169445

reynaldots · 2025-05-26T03:13:35Z

Respect user-data-dir flag from web-browser-flag

Currently, it's already possible to pass web-browser-flag when launching Chrome, but the user-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

TEST(CommandLineTest, MultipleSameSwitch) {
  const CommandLine::CharType* argv[] = {
      FILE_PATH_LITERAL("program"),
      FILE_PATH_LITERAL("--foo=one"),  // --foo first time
      FILE_PATH_LITERAL("-baz"),
      FILE_PATH_LITERAL("--foo=two")   // --foo second time
  };
  CommandLine cl(std::size(argv), argv);

  EXPECT_TRUE(cl.HasSwitch("foo"));
  EXPECT_TRUE(cl.HasSwitch("baz"));

  EXPECT_EQ("two", cl.GetSwitchValueASCII("foo"));
}

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 via web-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

{
  "version": "0.2.0",
  "configurations": [
    {
      "name": "flutter",
      "request": "launch",
      "type": "dart"
    },
    {
      "name": "flutter (profile mode)",
      "request": "launch",
      "type": "dart",
      "flutterMode": "profile"
    },
    {
      "name": "flutter (release mode)",
      "request": "launch",
      "type": "dart",
      "flutterMode": "release"
    },
    {
      "name": "Flutter for web (hot reloadable)",
      "type": "dart",
      "request": "launch",
      "program": "lib/main.dart",
      "args": [
        "-d",
        "chrome",
        "--web-port=5000",
        "--web-experimental-hot-reload",
        "--web-browser-flag=--user-data-dir=D:\\_Desenv\\flutter_tests\\chrome_profile"
      ]
    }
  ]
}

chrome://version

Before	After

Folder

Before	After

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

flutter-dashboard · 2025-05-26T03:13:38Z

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.

bkonyi

Thanks for the contribution!

This should ideally be tested. Can you add a test to make sure this flag is respected properly?

packages/flutter_tools/lib/src/web/chrome.dart

mdebbar

LGTM if it LGT @bkonyi

…d rearranged variables

bkonyi

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

packages/flutter_tools/lib/src/web/chrome.dart

bkonyi · 2025-06-23T14:35:02Z

@reynaldots, just checking in. Do you still plan on moving forward with this PR?

reynaldots · 2025-06-23T16:44:42Z

@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.
I didt understand how i can refactor when you sad: This file needs to be formatted.

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.

bkonyi · 2025-06-23T16:55:13Z

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 dart format packages/flutter_tools/lib/src/web/chrome.dart.

For the package:collection issue, it looks like there's no other uses of package:collection in flutter_tools, so it's not actually part of the project's pubspec.yaml. You'll need to add it to the pubspec.yaml (use the version listed here) and then run the following command: flutter update-packages --cherry-pick=collection:<VERSION_HERE>, where <VERSION_HERE> is replaced with the version of package:collection.

reynaldots · 2025-07-18T14:16:35Z

Hi @bkonyi! I finished the corrections.

bkonyi

LGTM!

auto-submit · 2025-07-25T15:40:02Z

autosubmit label was removed for flutter/flutter/169445, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

mdebbar · 2025-07-25T18:36:59Z

@zanderso I think this was fixed in #172776

auto-submit · 2025-07-25T18:37:09Z

Unable to create the revert pull request due to ProcessException: Standard out
Auto-merging packages/flutter_tools/test/web.shard/chrome_test.dart
CONFLICT (content): Merge conflict in packages/flutter_tools/test/web.shard/chrome_test.dart
Standard error
error: could not revert aea660d... feat(tool): Respect user-data-dir flag from web-browser-flag (#169445)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git revert --continue".
hint: You can instead skip this commit with "git revert --skip".
hint: To abort and get back to the state before "git revert",
hint: run "git revert --abort".
Command: git revert --no-edit -m 1 aea660d

zanderso · 2025-07-25T18:38:15Z

Oops. You're right. Thanks!

flutter/flutter#169445)

…#169445) ### Respect user-data-dir flag from web-browser-flag Currently, it's already possible to pass `web-browser-flag` when launching Chrome, but the `user-data-dir` flag doesn't work as expected, and there are some reasons for that. In the implementation made in [PR flutter#104935](flutter#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 ``` TEST(CommandLineTest, MultipleSameSwitch) { const CommandLine::CharType* argv[] = { FILE_PATH_LITERAL("program"), FILE_PATH_LITERAL("--foo=one"), // --foo first time FILE_PATH_LITERAL("-baz"), FILE_PATH_LITERAL("--foo=two") // --foo second time }; CommandLine cl(std::size(argv), argv); EXPECT_TRUE(cl.HasSwitch("foo")); EXPECT_TRUE(cl.HasSwitch("baz")); EXPECT_EQ("two", cl.GetSwitchValueASCII("foo")); } ``` 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 via `web-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: flutter#104935 (comment) Example: `launch.json` ``` { "version": "0.2.0", "configurations": [ { "name": "flutter", "request": "launch", "type": "dart" }, { "name": "flutter (profile mode)", "request": "launch", "type": "dart", "flutterMode": "profile" }, { "name": "flutter (release mode)", "request": "launch", "type": "dart", "flutterMode": "release" }, { "name": "Flutter for web (hot reloadable)", "type": "dart", "request": "launch", "program": "lib/main.dart", "args": [ "-d", "chrome", "--web-port=5000", "--web-experimental-hot-reload", "--web-browser-flag=--user-data-dir=D:\\_Desenv\\flutter_tests\\chrome_profile" ] } ] } ``` `chrome://version` | Before | After | |--------|--------| | ![before](https://github.com/user-attachments/assets/b3f33419-fbf1-446e-a471-745df2e8c4f6) | ![after](https://github.com/user-attachments/assets/ea74a203-c269-4ce1-8a9b-f97e9a4cdc78) | Folder | Before | After | |--------|--------| | ![before_files](https://github.com/user-attachments/assets/8522e60b-d0eb-4b66-a4b6-c2d6ed34c15b) | ![after_files](https://github.com/user-attachments/assets/ff94b498-b319-474c-89e5-c55f6e1c4383) | ## 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]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md