718322 - Add a "load in background" parameter to "open location" scripting support

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[Chris Lawson (gone)]

We need to be able to script things to happen in background windows/tabs (e.g. opening a bunch of bookmarks in background tabs or in a background window rather than the foreground window). Smokey thinks the surgery for this won't be too bad.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Because we can't "make new tab" or "make new browser window" (bug 395711 and bug 395712) and then set their properties, our only way of opening new items is to (ab)use "open location", which is eventually handled by MainController's showURL:usingReferrer:. 

It's very frustrating trying to write scripts given the limited flexibility in showURL: and the fact that it was really designed for handing non-scriptable URL-opening Apple Events and for various internal application usage.  Passing in an optional background argument would help with lots of scriptable URL-loading situations.

Aside from the
if (tabOrWindowIsAvailable || openExternal == kExternalLoadReusesWindow)
case, the foo-opening methods that showURL:usingReferrer: calls know about background foo; they're just hard-coded to loadInBackground:NO in showURL:usingReferrer:.

I don't think it would be too difficult to support passing in an optional background argument and using it for new tab/new window cases.  Background is irrelevant for the kExternalLoadReusesWindow case, but we probably want to short-circuit tabOrWindowIsAvailable when background is present and YES, which is probably the trickiest and sketchiest part.

I'll look at this once my plate is a little less full, but anyone else is free to steal it from me before then.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Thinking more about this, we might want to English-name the property "load in background" to make scripts seem less stilted:

open location "foo" load in background true

vs.

open location "foo" background true

It's still a little ambiguous as to what background means (bring the app to the front/keep in back vs. background tab/window), but 1) it's less ambiguous than "background" alone and 2) we can make the description mention bg tab/window.

Comment 3

[Stuart Morgan]

How about "in background"? Seems like it would read smoothly that way.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to Stuart Morgan from comment #3)
> How about "in background"? Seems like it would read smoothly that way.

Yeah, we could do that, too.  I think I was shying away from it before because I thought it was confusing regarding what it meant (bring the app to the front/keep in back vs. use a background tab/window), but looking at it again, that's really no more/less confusing in that regard than "load in background" is, and the parameter's description would clarify that.

--

I'll try and work this up later this weekend; I'd like to get this for 2.1.1 (so we're not repeatedly changing 'open location' in subsequent minor updates) and bug 717519, so we have a nice chunk of AppleScript fixes.  (Also because both of these bugs would make the script I wrote for cl work better and more sanely :P )

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Oh, or did you mean just "in background" with no true/false?  I don't think we can do that--or at least I have no idea how to do that.

Hmm, maybe we could fake it by using an enumeration "in" that has only one member, "background"?  I'm not sure if a single-member enumeration is possible, though…

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Or we could use a participle-like "loading in background" for our English, so

open location 'foo' loading in background yes

or

open location 'foo' with loading in background 

would work in scripts ("open location 'foo' loading in background true" appears to be rewritten as "with loading in background" by Script Editor).

This is also a common AppleScript language pattern, e.g., Standard Suite's "close/quit saving [yes/no/ask]" (although those are enumerations, since they provide 2+ options) and Mail's "forward/redirect reply opening window".

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

For some reason, the kExternalLoadOpensNewWindow case isn't properly handling a true/YES loadInBackground value and the window is always opened in front of an existing browser window; I'm going to have to try and debug that yet.

I think that unless we want to implement this with a "fake" enumeration (which I haven't tried to implement yet, and which would be bad for existing scripts if we later wanted to add different options, e.g. explicit control of new window or new tab, that would not be mutually exclusive with background), we need to use the "loading in background" English name, because "open location 'foo' with in background" is some really anguished English, even by AppleScript standards :P

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

So, everything's being passed along correctly into the |if| block in BWC's openNewWindow: http://mxr.mozilla.org/camino/source/camino/src/browser/BrowserWindowController.mm#3871

That code's just not working correctly in this case, and I'm not really sure what to do to debug why.  (It does work correctly if, say, I Ctrl-click on a link with Shift down and choose "Open in New Window", or if I do the same to a bookmark in the Bookmarks window, though that one doesn't go through this code path like links and showURL do).

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Logging stuff in that |if| block

AppleScript
browserWin: <BrowserWindow: 0x171dc040>
[browserWin windowNumber]: -1
[browserWin title]: Untitled
[self window]: <BrowserWindow: 0x15421ae0>
[[self window] windowNumber]: 7572
[[self window] title]: Google

Context menu
browserWin: <BrowserWindow: 0x12e97550>
[browserWin windowNumber]: -1
[browserWin title]: Untitled
[self window]: <BrowserWindow: 0x15421ae0>
[[self window] windowNumber]: 7572
[[self window] title]: Google

produces identical results in each case; [self window] is always the existing window, and the (new) BrowserWindow appears to have identical properties in each case.

What am I missing?

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: WIP

FWIW, here's the WIP patch (non-logging version) for this bug.

Comment 11

[Stuart Morgan]

The problem is [[controller window] makeKeyAndOrderFront:nil]; in MC; it's making the window frontmost.

I'm not convinced that adding another code path through method that bypasses almost all of it is the right approach though; the logic is getting pretty convoluted at that point. I think we should probably break out a helper for 'new window or tab according to user pref' that takes url, referrer, background, and then is used by showURL: but also used directly by the scripting code when background is true.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to Stuart Morgan from comment #11)
> The problem is [[controller window] makeKeyAndOrderFront:nil]; in MC; it's
> making the window frontmost.

Thanks!  Not sure how I missed that :-(

> I'm not convinced that adding another code path through method that bypasses
> almost all of it is the right approach though; the logic is getting pretty
> convoluted at that point.

Agreed.

> I think we should probably break out a helper for
> 'new window or tab according to user pref' that takes url, referrer,
> background, and then is used by showURL: but also used directly by the
> scripting code when background is true.

I've also been wondering about breaking out the find-and-show code, although it's less entangled in this scripting mess.

My thought was perhaps we could then have the plain-old showURL: (which is all of the old, non-scripting call sites) call findAndShow: and then, if necessary, newWindowAndTabAccordingToUserPref: directly, rather than calling through showURL:usingReferrer:loadInBackground:.  Then the scripting method becomes totally separate (while keeping the same general logic as the default showURL:) and hopefully more clear.  We'd have to rework the return from findAndShow: so that both showURL:s can know not to call newWindowAndTabAccordingToUserPref:, too.

Dunno.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: v1.1, with refactoring

Ok, here's a patch that does the refactoring I suggested and, I hope, the refactoring Stuart wanted.  showURL: and the Apple Event handling version (showURL:foo:bar:baz:etc:) now both call the same two methods on their own terms depending on their inputs.  Further, the "open-new-foo-based-on-pref" code specifically handles the background case in an early return, rather than with an if-if here and an if-if there, here an if, there an if, everywhere an if-if throughout Old MacCamino's "open-new-foo-based-on-pref" farm, er, code.

I've tested this (again, with everything except Services, which are a PITA), but a second look to make sure I didn't inadvertently change expected behavior for non-script/Apple Event cases would probably be good, too.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Sample text-format AppleScript

Here's an AppleScript to exercise some of this functionality and to illustrate some of the options you might want to use when testing this functionality.

Comment 15

[philippe (part-time)]

Comment on attachment 591697 [details] [diff] [review]
v1.1, with refactoring

This works fine so far. I tested opening url's from the Dock (both from the 'collections' and the 'recently opened documents' that 10.7 appends to the Dock menu); played around with search engines in the search field (add, delete, Find Search engines, etc). Cmd-Shift+U still works fine… 
I haven't had a chance to play around with AppleScripts yet, but I modified my 'open Safari URL in Camino' service which is a simple 'run AppleScript' action, to open in a background tab and keep Camino in the background - worked perfectly fine.

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 591697 [details] [diff] [review]
v1.1, with refactoring

>+			<parameter name="loading in background" code="bgnd" description="Should the URL be opened in a background tab (or background window)?" type="boolean" optional="yes">
>+				<cocoa key="background"/>
>+			</parameter>

The scripting guidelines suggest that we always specify what the default value of a boolean parameter is, so that description should change to something like:

"Should the URL be opened in a background tab (or background window)?  The default is to open a foreground tab or window."

or

"Should the URL be opened in a background tab (or background window)?  The default is not to open URLs in the background."

or anything you like better.

Comment 17

[Stuart Morgan]

Comment on attachment 591697 [details] [diff] [review]
v1.1, with refactoring

>+- (void)openNewTabOrWindowPerUserPrefForURL:(NSString*)aURL referrer:(NSString*)aReferrer loadInBackground:(BOOL)aLoadInBG;

This is a mouthful; how about openNewBrowserForURL:... ? It's a bit odd, but less tortured-sounding.

>+// Shows a given URL by finding and showing an existing tab/window with that URL.
>+- (BOOL)findAndShowURL:(NSString*)aURL

Document the return value as well.

>+- (void)openNewTabOrWindowPerUserPrefForURL:(NSString*)aURL referrer:(NSString*)aReferrer loadInBackground:(BOOL)aLoadInBG
...
>+    // If an "open location" Apple Event wants to load in a background tab or

Don't mention Apple Events here; it's none of this method's business why aLoadInBG is YES.

>+    if (aLoadInBG) {
>+      if (openExternal == kExternalLoadOpensNewWindow)
>+        controller = [controller openNewWindowWithURL:aURL referrer:aReferrer loadInBackground:YES allowPopups:NO];
>+      else
>+        // kExternalLoadOpensNewTab (or kExternalLoadReusesWindow or unexpected value)
>+        [controller openNewTabWithURL:aURL referrer:aReferrer loadInBackground:YES allowPopups:NO setJumpback:NO];

This is all duplicated with the code below. Just:
- change the first condition in the existing code to:
      if (!aLoadInBG && (<existing code>))
- Change tho loadInBackground params from NO to aLoadInBG
- put a |if (!aLoadInBG)| before the makeKeyAndOrderFront:
and then you can remove all the new code above.

>+// This is a convenience version for callers that are not Apple Event handlers.

Remove this comment.

>+  // Check to see if we already have the URL somewhere, and just show it if we do.
>+  BOOL urlFound = [self findAndShowURL:aURL];
>+  if (urlFound)
>+    return;

Inline the test:
  if ([self findAndShowURL:aURL])
    return;

>+  // If we got here, we didn't find it already open, so open the URL based on user
>+  // prefs.

This doesn't really say anything that's not obvious from the code, so remove it.

>+  // Only attempt to reuse an existing instance if there is no referrer and no
>+  // request to load the URL in the background.
>+  if (!aReferrer && !aLoadInBG) {
>+    BOOL urlFound = [self findAndShowURL:aURL];
>+    if (urlFound)
>+      return;
>+  }

Instead of duplicating all the implementation in the convenience method, the convenience version should call this one with |nil| and |NO|.

>+  // If we got here, we didn't find it already open, the caller specified a
>+  // referrer, or the caller requested the URL be opened in the background.
>+  // Open the URL in a tab or window based on user prefs and accounting for
>+  // parameters.

Again, remove this. It's just the |if| in English, and exactly duplicating code in English comments is a great way to end up with comments that disagree with code later on when someone changes the code and not the comment.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: v1.2, sr comments addressed

(In reply to Stuart Morgan from comment #17)
> >+    if (aLoadInBG) {
> >+      if (openExternal == kExternalLoadOpensNewWindow)
> >+        controller = [controller openNewWindowWithURL:aURL referrer:aReferrer loadInBackground:YES allowPopups:NO];
> >+      else
> >+        // kExternalLoadOpensNewTab (or kExternalLoadReusesWindow or unexpected value)
> >+        [controller openNewTabWithURL:aURL referrer:aReferrer loadInBackground:YES allowPopups:NO setJumpback:NO];
> 
> This is all duplicated with the code below. Just:
> - change the first condition in the existing code to:
>       if (!aLoadInBG && (<existing code>))
> - Change tho loadInBackground params from NO to aLoadInBG
> - put a |if (!aLoadInBG)| before the makeKeyAndOrderFront:
> and then you can remove all the new code above.

That's a lot nicer (and what I had done before); I'd just misunderstood what you meant earlier when you said

(In reply to Stuart Morgan from comment #11)
> I'm not convinced that adding another code path through method that bypasses
> almost all of it is the right approach though

;-)  I took it for the whole method, whereas you appear to have just meant the "find and show" code that's now factored out.  

Thanks for the pointers and review!  This version should have all your comments addressed, and it also includes the updated description from comment 16 for the "loading in background" parameter.

Comment 19

[Stuart Morgan]

Comment on attachment 596776 [details] [diff] [review]
v1.2, sr comments addressed

Review of attachment 596776 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, sr=smorgan

Comment 20

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/7c98789f98ca

Thanks for all the reviews!