Skip to content
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

Quickly Launch Different Chrome for Privacy Sandbox Demo #211

Merged
merged 25 commits into from
Jan 15, 2024

Conversation

MiteshShah
Copy link
Collaborator

@MiteshShah MiteshShah commented Oct 24, 2023

Description

This script helps to launch different Chrome ( Macintosh and Linux platforms) quickly for Privacy Sandbox Demo.

Testing Instructions

curl -sL https://rt.cx/psat | bash   

Above command call install.sh which download chrome_launcher.sh and source it into .zshrc or .bashrc also automatically source into the current terminal session as well.

@google-cla
Copy link

google-cla bot commented Oct 24, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

chrome_launcher.sh Outdated Show resolved Hide resolved
@MiteshShah MiteshShah requested a review from dhsathiya October 24, 2023 12:10
@mohdsayed
Copy link
Collaborator

Instead of downloading the script, shouldn't they be able to run using npm script ? Something like this?

In package.json

"scripts": {
	"launch-chrome:default": "bash ./bin/chrome_launcher/chrome-default.sh",
	"launch-chrome:default-ps": "bash ./bin/chrome_launcher/chrome-default-ps.sh",
	"launch-chrome:3pcd": "bash ./bin/chrome_launcher/chrome-3pcd.sh",
	"launch-chrome:3pcd-ps": "bash ./bin/chrome_launcher/chrome-3pcd-ps.sh",
	"launch-chrome:chip": "bash ./bin/chrome_launcher/chrome-chip.sh"		
}

and then run

npm run launch-chrome:default
npm run launch-chrome:default-ps
npm run launch-chrome:3pcd
...

@gagan0123
Copy link
Collaborator

@sayedtaqui

If we go with npm based commands, it will require the users to install git, clone the code, install node, install npm, then install npm dependencies, then they'll be able to run the commands. While going with directly having the script, they only need to run the script once, whether they have git, node or npm installed, it won't matter.

@mohdsayed
Copy link
Collaborator

@sayedtaqui

If we go with npm based commands, it will require the users to install git, clone the code, install node, install npm, then install npm dependencies, then they'll be able to run the commands. While going with directly having the script, they only need to run the script once, whether they have git, node or npm installed, it won't matter.

Well, we do expect the user to have those for running CLI commands (example npm run cli -- -u https://bbc.com). However, if we are only expecting them to download the file, it doesn't make sense to keep it in the repo because it has no relation to the rest of the codebase. Instead, it can simply be a link to a gist file in the readme. Although, I still think it would be helpful to include the npm command as well.

@amedina
Copy link
Collaborator

amedina commented Oct 24, 2023 via email

@gagan0123
Copy link
Collaborator

@MiteshShah

/usr/local/bin is owned by the user root so the script won't be able to write data directly. For writing there we'll need to add sudo when downloading the file.

@gagan0123
Copy link
Collaborator

Also, can we have a wrapper around to check whether the file exists or not before sourcing it in the .zshrc and .bashrc, something like this:

# Chrome profiles
if [ -f ~/Tools/Scripts/chrome.sh ]; then
    source ~/Tools/Scripts/chrome.sh
fi

@MiteshShah
Copy link
Collaborator Author

@gagan0123 I'd modify script so its wont need sudo anymore

To test please clear old source line from .bashrc .zshrc and run following command again

curl -sL https://rt.cx/gpsat | bash   

Its will create ~/bin directory and add our chrome_launcher.sh inside it so wont needed sudo
Also now if you check your .bashrc or .zshrc its also check if file exist before source it like you mentioned on above comments.

@gagan0123
Copy link
Collaborator

@MiteshShah

The URL https://rt.cx/gpsat is still pointing to the old version of the install script, can we please update this?

@mohdsayed
Copy link
Collaborator

@MiteshShah Can you please move these files in a new bin folder.

@gagan0123
Copy link
Collaborator

@MiteshShah

Thanks, now its loading the correct install script, the redirection was cached in my browser so I was still seeing the old install script.

@sayedtaqui

Can you provide more information where to create a new bin directory and why?

Do you mean in the repo, we should create a bin directory and place these scripts there?

chrome_launcher.sh Outdated Show resolved Hide resolved
@mohdsayed
Copy link
Collaborator

@gagan0123

Can you provide more information where to create a new bin directory and why?

Do you mean in the repo, we should create a bin directory and place these scripts there?

Yes, please create a bin folder in the root of the repository and move these .sh files into that folder. These .sh files should not be directly dumped in the root folder.

@MiteshShah
Copy link
Collaborator Author

@sayedtaqui Moved script to bin folder
@gagan0123 v0.3.1 is updated as well.

@MiteshShah MiteshShah requested a review from gagan0123 October 25, 2023 10:30
extension_setup
launch_chrome \
--install-autogenerated-theme='150,220,150' \
--load-extension="/tmp/ps-analysis-tool-v0.3.0/extension/"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Modify it to get the version from the ps_analysis_tool_version variable that we defined

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

launch_chrome \
--install-autogenerated-theme='255,51,51' \
--test-third-party-cookie-phaseout \
--load-extension="/tmp/ps-analysis-tool-v0.3.0/extension" \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Modify it to get the version from the ps_analysis_tool_version variable that we defined

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@amedina amedina added P0 Highest priority PSAT Features and tasks related to DevTools extension labels Oct 31, 2023
@gagan0123
Copy link
Collaborator

@MiteshShah

I added two commits for:

  1. Go back to the initial directory you started the command in
  2. Start Chrome maximized and open DevTools automatically

Can you please review the code, test it out and improve if needed?

@MiteshShah
Copy link
Collaborator Author

@gagan0123 All looks good to me.


# Download Extension
extension_setup() {
ps_analysis_tool_version=v0.3.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

v0.3.2 just released today

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

Updating extension version
@gagan0123
Copy link
Collaborator

@MiteshShah

When using MacOS, the directory /private/tmp is utilized to download the extension. However, this directory has a feature that deletes files that have not been used in a while, which results in the deletion of extension files. Even though the directory structure remains in place, all files within it are deleted. Currently, we only verify the existence of the directory, which returns true even when all the extension files have been deleted. Therefore, we need to relocate the unzipped extension to a different temporary location for MacOS. I think /var/tmp will work for our usecase. Do you have any suggestions?

@MiteshShah
Copy link
Collaborator Author

@gagan0123 we can use /var/tmp or we can use ~/.psat inside user home directory.
Let me know what you prefer and I'll update the script.

1 similar comment
@MiteshShah
Copy link
Collaborator Author

@gagan0123 we can use /var/tmp or we can use ~/.psat inside user home directory.
Let me know what you prefer and I'll update the script.

@gagan0123
Copy link
Collaborator

@mohdsayed

After the script is installed, we now show a message that you have to either re-source the ~/.bashrc or ~/.zshrc based on the shell the user has, and provide the list of commands as well as shown in the following screenshot:
image

Do you have any suggestions to improve the output or to make it even clearer to the user, in terms of instructions, what the next steps are after installing the script?

@mohdsayed
Copy link
Collaborator

@mohdsayed

After the script is installed, we now show a message that you have to either re-source the ~/.bashrc or ~/.zshrc based on the shell the user has, and provide the list of commands as well as shown in the following screenshot: image

Do you have any suggestions to improve the output or to make it even clearer to the user, in terms of instructions, what the next steps are after installing the script?

I think we can also add some description to the commands, explaining what they do.

@gagan0123
Copy link
Collaborator

@mohdsayed

How about the following?
image

@prasadnevase
Copy link
Collaborator

@gagan0123 this looks good to me. Lets merge as it will also address #409

@gagan0123 gagan0123 merged commit 6ee6f91 into develop Jan 15, 2024
1 check passed
@gagan0123
Copy link
Collaborator

I missed the last commit, so added it in this PR: #416

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 Highest priority PSAT Features and tasks related to DevTools extension
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants