Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[web] Fixing launching Safari. This should solve the LUCI issue #16590

Merged
merged 2 commits into from
Feb 14, 2020
Merged

[web] Fixing launching Safari. This should solve the LUCI issue #16590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfc3e35
Fixing launching Safari. This should solve the LUCI issue
Feb 13, 2020
595f851
more comments and linking the issue
Feb 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions lib/web_ui/dev/common.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ abstract class PlatformBinding {
String getChromeExecutablePath(io.Directory versionDir);
String getFirefoxExecutablePath(io.Directory versionDir);
String getFirefoxLatestVersionUrl();
String getSafariSystemExecutablePath();
String getMacApplicationLauncher();
String getCommandToRunEdge();
}

Expand Down Expand Up @@ -85,7 +85,7 @@ class _WindowsBinding implements PlatformBinding {
'https://download.mozilla.org/?product=firefox-latest&os=win&lang=en-US';

@override
String getSafariSystemExecutablePath() =>
String getMacApplicationLauncher() =>
throw UnsupportedError('Safari is not supported on Windows');

@override
Expand Down Expand Up @@ -120,7 +120,7 @@ class _LinuxBinding implements PlatformBinding {
'https://download.mozilla.org/?product=firefox-latest&os=linux64&lang=en-US';

@override
String getSafariSystemExecutablePath() =>
String getMacApplicationLauncher() =>
throw UnsupportedError('Safari is not supported on Linux');

@override
Expand Down Expand Up @@ -161,8 +161,7 @@ class _MacBinding implements PlatformBinding {
'https://download.mozilla.org/?product=firefox-latest&os=osx&lang=en-US';

@override
String getSafariSystemExecutablePath() =>
'/Applications/Safari.app/Contents/MacOS/Safari';
String getMacApplicationLauncher() => 'open';

@override
String getCommandToRunEdge() =>
Expand Down
39 changes: 16 additions & 23 deletions lib/web_ui/dev/safari.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,8 @@
// found in the LICENSE file.

import 'dart:async';
import 'dart:convert';
import 'dart:io';

import 'environment.dart';

import 'package:path/path.dart' as path;
import 'package:pedantic/pedantic.dart';

import 'package:test_core/src/util/io.dart'; // ignore: implementation_imports

import 'browser.dart';
import 'safari_installation.dart';
import 'common.dart';
Expand Down Expand Up @@ -43,21 +35,22 @@ class Safari extends Browser {
infoLog: DevNull(),
);

// Safari will only open files (not general URLs) via the command-line
// API, so we create a dummy file to redirect it to the page we actually
// want it to load.
final Directory redirectDir = Directory(
path.join(environment.webUiDartToolDir.path),
);
final redirect = path.join(redirectDir.path, 'redirect.html');
File(redirect).writeAsStringSync(
'<script>location = ' + jsonEncode(url.toString()) + '</script>');

var process =
await Process.start(installation.executable, [redirect] /* args */);

unawaited(process.exitCode
.then((_) => File(redirect).deleteSync(recursive: true)));
// In the latest versions of MacOs opening Safari browser with a file brings
// a popup which halts the test.
// The following list of arguments needs to be provided to the `open` command
// to open Safari for a given URL. In summary they provide a new instance
// to open, that instance to wait for opening the url until Safari launches,
// provide Safari bundles identifier.
// The details copied from `man open` on MacOS.
// TODO(nurhan): https://github.com/flutter/flutter/issues/50809
var process = await Process.start(installation.executable, [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Browser class expects that the process object returned from this function is the browser process, or at least some proxy of it. However, this would return the open process that quits immediately. I have a feeling this will become a thorn in future evolution of the Browser class. For example, this will prevent us from being able to implement logic that watches the health of the browser process, such as premature termination (examples).

Having said that, I don't have an immediate solution to this, other than what we talked about offline, i.e. use WebDriver to control launching of browsers. If I'm reading the w3c spec correctly, Selenium is not necessary to use it (see WebDriver Protocol). So please, treat this comment as non-blocking. I'll file a separate issue for us to investigate a WebDriver solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

'-F', // Open a fresh application with no persistant state.
'-W', // Open to wait until the applications it opens.
'-n', // Open a new instance of the application.
'-b', // Specifies the bundle identifier for the application to use.
'com.apple.Safari', // Bundle identifier for Safari.
'${url.toString()}'
]);

return process;
});
Expand Down
2 changes: 1 addition & 1 deletion lib/web_ui/dev/safari_installation.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ Future<BrowserInstallation> getOrInstallSafari(
infoLog.writeln('Using the system version that is already installed.');
return BrowserInstallation(
version: 'system',
executable: PlatformBinding.instance.getSafariSystemExecutablePath(),
executable: PlatformBinding.instance.getMacApplicationLauncher(),
);
} else {
infoLog.writeln('Unsupported version $requestedVersion.');
Expand Down