Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

Fix for launching Chrome with temporary profile in OSX. #339

Closed

Conversation

fungl164
Copy link
Contributor

Prevents Brackets from interfering with end-user sessions in Chrome (i.e. history, open tabs, etc.).

See related: adobe/brackets#2998.

This requires brackets pull request: adobe/brackets#5392

@ghost ghost assigned redmunds Sep 26, 2013
@redmunds
Copy link
Contributor

@fungl164 Thanks for submitting this pull request! You'll need to agree to the Brackets CLA before I can merge this.

@redmunds
Copy link
Contributor

@fungl164 This doesn't seem to work how I was expecting it to. On Windows, you can open a Chrome browser window and have some Tabs open in that window. Then in Brackets, when you start Live Development, it opens a second Chrome browser window.

On MacOS 10.8, Live Dev tries to open a new Tab in existing window, fails, and then prompts me to Relaunch Chrome.

With this code, can you open Live Dev in the scenario above without getting the Relaunch Chrome prompt? If so, is Live Dev opened in a new window, or just a new Tab?

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2013

@fungl164 I've been playing around with this and I can get it to work when I type in the args on the command line, but not in Brackets, so I guess it must have something to do with how the command is getting built in brackets-shell.

If you get a chance to work on this some more, we want the user data dir to go the app support folder, and use a different name to be consistent with other platforms, which needs to be quoted since there are spaces in the path, so something like:

--user-data-dir=\"" + ClientApp::AppGetSupportDirectory() + "/live-dev-profile\""

And one other flag for those that have a default browser other than Chrome:

--no-default-browser-check

@fungl164
Copy link
Contributor Author

fungl164 commented Oct 1, 2013

@redmunds so I made a few changes to make chrome startup and terminate based on proper process ids (fungl164@8e0fa25). I've also added your comments as a separate commit (fungl164@44b8b79).

I still cannot get Brackets to issue a CloseLiveBrowser() call when clicking on the Live Dev icon. I believe its somewhere in the js code, but I don't know the proper place to put it. Once that's done, it should terminate correctly.

Disabling Live Dev from the toolbar just closes the tab but leaves the chrome instance running right now because of the missing call to CloseLiveBrowser(). To enable Live Dev, you'll need to close the second instance of Chrome and click the icon in Brackets.

@fungl164
Copy link
Contributor Author

fungl164 commented Oct 1, 2013

amended last commit to account for common profile path in process id look-up.
old -> fungl164@44b8b79
new -> fungl164@107ce80

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2013

@fungl164 Can you push your changes to the same branch so I can pull them and take a look?

@fungl164
Copy link
Contributor Author

fungl164 commented Oct 1, 2013

@redmunds ok. try it now.

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2013

Awesome! This is getting very close.

It's not critical that Chrome app stays open with no windows open because that's how it behaves now, but yes it would be nice. If you want to mess with it, the next function in the brackets-shell code is CloseLiveBrowser() if you want to try to close it from brackets-shell, or use NativeApp.closeLiveBrowser() in brackets.

The main problem I am seeing is that a Tab with the "interstitial" page (LiveDevelopment/launch.html) is staying open on the first launch. If no Chrome window is open, then the tab is in the live-dev-profile window. If there is already a default Chrome window open, then the Tab is in the default window. Any ideas on that?

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2013

Also, is --temp-profile necessary? I commented it out and it doesn't seem to have any effect.

@fungl164
Copy link
Contributor Author

fungl164 commented Oct 1, 2013

@redmunds --temp-profile AFAIK is only necessary and useful if no --user-data-dir is specified. Otherwise, chrome will attempt to use the current/last used profile with the corresponding state.

@fungl164
Copy link
Contributor Author

fungl164 commented Oct 1, 2013

@redmunds added fix for brackets source to call native impl of CloseLiveBrowser().
(see fungl164/brackets@7d9bff2)

} else if (apps.count > 0 && !LiveBrowserMgrMac::GetInstance()->GetTerminateObserver()) {
}

if (!LiveBrowserMgrMac::GetInstance()->GetTerminateObserver()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few places where LiveBrowserMgrMac::GetInstance() can be repplaced with liveBrowserMgr.

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2013

@fungl164 This seems to work great! Brackets Mac users will be very happy about this. I'm going to bang on it a little more and make one more pass through the code.

/* Note location of current '\0'. */
np = cp;

if (!show_args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how the value of show_args can ever change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I just tweaked the code to get what I needed for the moment. I made no optimizations attempts. Do you want it taken out?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you can think of a good reason that we might need it someday then add a comment explaining it, otherwise just pull it out.

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2013

Found a bug:

  1. Start a default Chrome window
  2. Start Live Dev into a live-dev-profile Chrome window
  3. Stop Live Dev -- notice second instance of Chrome is still in taskbar
  4. Start Live Dev again

Results
New Tab is opened in default Chrome window, not live-dev-profile

@fungl164
Copy link
Contributor Author

fungl164 commented Oct 1, 2013

@redmunds Hmmm. I can't replicate it over here. I works for me. Can you download the latest code and test with it?

@fungl164
Copy link
Contributor Author

fungl164 commented Oct 1, 2013

also make sure you have the modifications I made to brackets src/LiveDevelopment/LiveDevelopment.js (fungl164/brackets@7d9bff2) installed.

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2013

No, I did not see the change you made to the brackets repo. Please submit a pull request so I can test & then merge both pull requests together. Thanks.

@fungl164
Copy link
Contributor Author

fungl164 commented Oct 1, 2013

Done.

@fungl164
Copy link
Contributor Author

I'm out for the eve. I'll try it again tom.
@gruehle @JeffryBooher Is there anything special about the dark shell I should be aware of? thnxs.

@gruehle
Copy link
Member

gruehle commented Oct 16, 2013

@fungl164 did you run grunt after pulling the latest source? This is needed to update the xcode project files.

@fungl164
Copy link
Contributor Author

Note to self:

  • Run grunt, not grunt build-mac when sync'n with master.

Thank you @gruehle

@gruehle
Copy link
Member

gruehle commented Oct 16, 2013

@fungl164 you can also run grunt create-project if you only need to create the Xcode project file. Run grunt --help to list the available grunt tasks.

(I always run grunt whenever pulling, since that guarantees to run all of the necessary sub-tasks to get the project up to date).

@JeffryBooher
Copy link
Contributor

(I always do a grunt setup followed by grunt when switching branches and after pulling especially if I notice that new files were added.) The grunt setup task ensures that new 3rd party dependencies are downloaded (i.e. CEF). I'm not certain if just running grunt alone will download them.

@gruehle
Copy link
Member

gruehle commented Oct 16, 2013

From grunt --help:

              default  Alias for "setup", "build" tasks.                      

So, yes, grunt alone runs the setup task.

@fungl164
Copy link
Contributor Author

Thank you fellas.

@fungl164
Copy link
Contributor Author

Can anyone tell me the intended lifecycle of _openDeferred in LiveDevelopment.js? It seems like it gets arbitrarily replaced when turning on LD. Since some functions make use of it, I'm thinking this could be a source of problems. I'm trying to track down some extra calls to native.closeLiveBrowser() when toggling LD on and off.

@redmunds
Copy link
Contributor

_openDeferred only resolves if all agents load successfully. If a failure is detected, it gets rejected. It is also possible that an agent never responds, and it is still in a pending state when closing LD.

@fungl164
Copy link
Contributor Author

@redmunds can you go over the latest brackets-shell changes. I've put comments around the code to make it easy to follow. I'm still getting calls to close the LB window from the brackets side. not sure where it's coming from...

@fungl164
Copy link
Contributor Author

@redmunds any ideas on what would be the best way to ensure the Inspector connection is clean & in-sync with the LiveDevelopment module status when connecting/disconnecting the LD session via the open/close icon?

@redmunds
Copy link
Contributor

To test that LiveDevelopement is connected, use: if (LiveDevelopment.status === LiveDevelopment.STATUS_ACTIVE).
To test that Inspector is connected, use: if (Inspector.connected())

Or were you asking how to deal with when it's not in this state? If so, what particular states are you seeing?

@redmunds
Copy link
Contributor

I'm hitting the compile error with @try again with your latest code.

@fungl164
Copy link
Contributor Author

Arrrrgh! I'm trying to protect against the ScriptingBridge since it is known to throw up every once in a while. @try should work on XCode 4. Is there anyone out there you can reach out to re: exceptions? I had to re-enable C++ exceptions on my project. Maybe that can help.

@redmunds
Copy link
Contributor

@fungl164 I can't figure this out. I have some time to mess with this. Can you come to the IRC #brackets channel so we can chat? Or maybe somewhere else?

@fungl164
Copy link
Contributor Author

@redmunds So I think I found the secret sauce and have it working with the latest brackets master branch w/ minor modifications to the LD module (which I'm thinking of force pushing to my repo fork). Or should I just create a new pull request?

@redmunds
Copy link
Contributor

Cool! I think starting new branch(es) with a new pull request(s) is a good idea.

@fungl164
Copy link
Contributor Author

New Brackets pull request created to go along with this one (see adobe/brackets#5569)

@fungl164
Copy link
Contributor Author

@redmunds I have a version that does real-time detection of the LiveBrowser process running in debug mode. This is useful for the corner case when chrome is already running in debug mode before Brackets starts (so no need to re-start).

Before I commit it though, I just want to know what's your take on using locks with @try/@finally blocks. Should I keep them or get rid of them?

@redmunds
Copy link
Contributor

what's your take on using locks with @try/@finally blocks.

I haven't had the time to research why those won't compile for me in Xcode 4. Once that's figured out, I don't have anything against them.

@fungl164
Copy link
Contributor Author

Ok. Discussion moved to pull request #359.

@peterflynn
Copy link
Member

@fungl164 @redmunds Should we close this? It sounds like it's been superseded by #359.

@fungl164
Copy link
Contributor Author

@peterflynn Yes, #359 supercedes this. Thanks.

@peterflynn
Copy link
Member

Thanks -- closing then.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants