Adding missing call to native.closeLiveBrowser()

[fungl164]

This should trigger the appropriate calls to handle the LiveBrowser state on the native side for OSX.

This requires brackets-shell pull request adobe/brackets-shell#359

[redmunds]

All of my basic test cases are working perfectly! Thanks for sticking with this!

We still need to figure out how to get the @try/@catch to compile in brackets-shell. And I need to make a final pass through the brackets-shell code.

[redmunds]

@fungl164 Note: make comments in the Files Changed tab of pull request (and not directly in the commit) so they don't get lost.

I like the idea of displaying the "err", but it should get displayed in addition to (not in place of) the "reason". Maybe something like:

fail(function (err) { 
    if (err) {
        reason += " (" err + ")"
    }
    _setStatus(STATUS_INACTIVE, reason || "explicit_close"); 
}

[redmunds]

NativeApp.closeLiveBrowser() should be getting done in the previous call to _doInspectorDisconnect(), so I'm curious as to what is the case why this needs to be done (again?) here?

If we do need it, then I think this should be isolated for Mac-only, and add a comment to explain why:

    // Need to do this on Mac because...
    var closeDeferred = (brackets.platform === "mac") ? NativeApp.closeLiveBrowser() : $.Deferred().resolve();
    closeDeferred.done(function () {
        _setStatus(STATUS_INACTIVE, reason || "explicit_close");
        deferred.resolve();
    }).fail(function () {
        _setStatus(STATUS_INACTIVE, reason || "explicit_close");
        deferred.resolve();
    });

[fungl164]

I fail to see that in _doInspectorDisconnect(). Can you point me in the right direction?

[fungl164]

I think the LiveInspector is just issuing a local window.close() via javascript on the browser. This never translates to the native call to closeLiveBrowser() in the app-shell code, unless it's buried somewhere I haven't looked. In any case, your solution would suffice. :)

[redmunds]

You're right -- NativeApp.closeLiveBrowser() only seems to get called in response to "Relaunch" dialog.

I wonder if the timeout in Inspector.disconnect() could be a problem. When the call to _socket.close() times out, _onDisconnect() doesn't get called, so "disconnect" event doesn't get triggered.

[fungl164]

That's what I was trying to track down: especially since an Inspector.disconnect()/detached() also triggers a LiveDevelopment.close(), things could be getting hairy. The _openDeferred variable is still somewhat suspicious to me, but you would probably know better...

[redmunds]

is there anything particularly bad about having NativeApp.closeLiveBrowser() be called from the cleanup code?

I originally asked about it to try to understand your code. It seems OK to me. As I said earlier, it should be isolated to the Mac and say in a comment "Mac doesn't close Chrome app when last window is closed".

[fungl164]

Got it. Also, it took me a while to figure we may occasionally have disconnects in logic. So, in trying to understand my code, keep in mind that I have different take on how Brackets deals with LD.

For OpenLiveBrowser, my logic is simply and roughly as follows:

1. Check if chrome is running in debug mode (port=9222)
2. if NO, open URL on a fresh instance of chrome running in debug + start listening to workspace notifications (1x only)
3. if YES, grab the running instance and open URL on a new tab**

This approach affords a bunch of desirable things (at least for me). Among them:

1. Brackets is guaranteed to always find or create a running debuggable instance of chrome for LD purposes
2. The user never gets prompted to relaunch (a much faster and less obstrusive approach)
3. The user's chrome profile never used, touched nor required, so it can never be polluted or corrupted by Brackets

For CloseLiveBrowser:

1. Grab any live instance of chrome running in debug mode (port=9222)
2. If none found, report no error
3. If chrome.windows > 1, report no error
4. If chrome.windows == 1 and chrome.window.tabs > 0, report no error
5. If chrome.windows == 0 Else, quit chrome

**Since OSX let's you have a valid running instance of chrome with no windows open, sometimes a new window object needs to be created before opening a new tab.

I only did this because I wanted to completely separate my profile and current live sessions with chrome from whatever I was doing with Brackets LD at the time. I can understand not everyone needs, wants or cares about this workflow, but it works for me in OSX.

I hope this helps. Thanks for providing guidance and comments. :)

[fungl164]

@redmunds In case you may wonder, this is how it works based on the latest code fixes I have not yet committed. Should I create a new pull for that branch?

[redmunds]

That sounds great. Yes, start a new PR whenever you start a new branch.

[fungl164]

Done. (see adobe/brackets-shell#359)

[fungl164]

With regards to @try/@catch, I think it would be useful to find out why XCode 4 is resisting. Knowing what I know now, I'm also considering bypassing the ScriptingBridge for now and going back to making lower-level system calls via sysctl. This would fix the compilation issues for sure.

[redmunds]

Brackets style guide says to limit code to 80 columns. We're lax on that up to maybe 90 (and comment in header of this function goes to 99), but this comment should be broken to a second line.

[redmunds]

Done with review.

[redmunds]

Looks good. This is ready to merge when adobe/brackets-shell#359 is ready.

[redmunds]

@fungl164 You'll need to agree to the Brackets CLA before I can merge your code.

[fungl164]

Cool!

[redmunds]

Merging.