Closed Bug 557057 Opened 13 years ago Closed 11 years ago

Can't "reload current tab" with AppleScript

Categories

(Camino Graveyard :: OS Integration, enhancement)

x86
macOS
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: web, Assigned: alqahira)

Details

(Whiteboard: [camino-2.1.1][needs rdar:// on comment 14])

Attachments

(3 files, 6 obsolete files)

18.58 KB, text/plain
Details
9.22 KB, patch
stuart.morgan+bugzilla
: superreview+
Details | Diff | Splinter Review
8.42 KB, text/plain; charset=utf16
Details
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.19pre) Gecko/2010032100 Camino/2.1a1pre (like Firefox/3.0.19pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.19pre) Gecko/2010032100 Camino/2.1a1pre (like Firefox/3.0.19pre)

I see this isn't implemented in Safari either, so maybe it isn't trivial, but I'd like to be able to 'tell application "Camino" to reload the current tab' or something similar, to refresh the current page.

Reproducible: Always
Summary: Can't "make reload current tab" with AppleScript → Can't "reload current tab" with AppleScript
This is probably do-able, and seems reasonable to me.

In the meantime, there's a work-around that's just one more line of code (given the way we currently behave when told by an external app to open URLs that are already open):

tell application "Camino"
	set theURL to URL of current tab of browser window 1
	open location theURL
end tell

Inelegant, but functional.
Thanks, that works exactly as needed! If you could give me wiki access, I'd happily add this to:
 http://wiki.caminobrowser.org/Development:Camino_AppleScript_Guide
?
Peter actually had a stub reload in the original .sdef (http://hg.mozilla.org/camino/rev/87b3d60e08fe#l4.248), but it wasn't the child of either browser window or tab, so it wouldn't even work (and was commented out, at that).  I expect it was probably on the SoC project agenda at some point and later waylaid?
Yeah, I don't see any reason not to do this either.
Attached patch Non-working patch (obsolete) — Splinter Review
I've worked on this bug on and off, never with any success.  This is the latest attempt, and iirc it still didn't make the command show up properly in the dictionary (let alone work), but I do think it compiled :P  Not sure what I'm missing, but someone with more AS know-how is needed ;)
Status: UNCONFIRMED → NEW
Ever confirmed: true
From doing some reading, at least part of my problem before was that I wasn't implementing the Cocoa code for the command correctly; in particular, the method should have been:

- (void)scriptableReload:(NSScriptCommand*)command

or similar.
Here's a skeleton of a fix, finally with working AppleScript dispatch.  With this patch, the following commands are successfully dispatched to the appropriate methods in ScriptingSupport.mm:

tell application "/path/to/patched/Camino.app"
	reload tab 1 of browser window 1
	reload browser window 1
	tell tab 1 of browser window 1 to reload
	tell browser window 1 to reload
end tell

I don't like the duplication of methods, but I don't think there's a way for browser window to implicitly inherit reload like it implicitly inherits all of the properties defined in the .sdef only for tab (not defining responds-to for BrowserWindow leads the AS to fail to run, and not adding the method in ScriptingSupport causes nothing to get called when the command is run).

So, at this point, what needs to be done is:

1) Figure out what we're asking to reload, and see if it can reload
2) Throw errors as appropriate (if we get bad input; if we have a non-reloadable tab, I think we want to fail silently rather than throw a script error).
3) Reload!
Attachment #500632 - Attachment is obsolete: true
Attached patch WIP fix, with some logging (obsolete) — Splinter Review
Well, I have this all implemented, with two problems:

1) Edge cases like "reload every tab of every browser window" and "reload tabs 1 through 2 of browser windows 2 through 3" don't yet work, because they are nested arrays, and I haven't unpacked the second level yet (thankfully, we can only have two levels!).

2) Whenever multiple (N) items are asked to reload, our method gets called N times, every single time *with the same array(s) of N items*!  So "reload tabs 1 through 5 of browser window 1" gets called 5 times, each time listing the same 5 tabs to be reloaded!  Bill Cheeseman basically describes the problem in http://lists.apple.com/archives/applescript-implementors/2005/May/msg00043.html, though he seems to believe it was tied to a specific 10.3 AppleScript behavior.  

He does outline a solution, though, which is to somehow keep a count of the number of items and only process the Nth invocation.  I'm not sure how to code that exactly, but presumably it could be done.

In practical terms, based on watching something like http://www.comptechdoc.org/independent/web/cgi/javamanual/javaihit.html, it doesn't seem like we're actually reloading N times, even though we're clearly calling reload N times, so I'm not sure how much of a problem that might be in reality.

3) The arrays we're being sent are apparently NSCFArrays, but ScriptingSupport doesn't import whatever header is needed to know what the class of NSCFArray is (and the internet is surprisingly light on useful info about said array type); however, testing to see if an object is an NSArray seems to work OK here.

The good news is that KVC appears to handle, in the single-tab and single-window cases, the index-out-of-bounds ("valid tab") problem before those instances ever hit our reload handler, and alerts the user in Script Editor.  (However, in a multi-tab or multi-window case, those commands just fail silently, e.g. "reload tabs 1 through 5 of browser window 1" where said window has only 4 tabs.)
Attachment #592284 - Attachment is obsolete: true
(In reply to comment #8)
> In practical terms, based on watching something like
> http://www.comptechdoc.org/independent/web/cgi/javamanual/javaihit.html, it
> doesn't seem like we're actually reloading N times, even though we're
> clearly calling reload N times, so I'm not sure how much of a problem that
> might be in reality.

I also put http://www.ardisson.org/smokey/moz/ as one of my test tabs and watched my server logs, and we're definitely not actually reloading twice, at least not fast enough for my server to tell.  I haven't put any logging down the pipes beyond BrowserWrapper's reload: (which is being called twice), so I don't know if something in CHBV or further down in Gecko is stopping the spurious additional reloads, or what.

This WIP iteration also tweaks the .sdef a little and implements support for an "ignoring cache" parameter (which I've also verified works as expected, based on response size in my server logs).

Still to do: deal with the nested arrays, and possibly try to short-circuit the extra calls.
Assignee: nobody → alqahira
Attachment #592374 - Attachment is obsolete: true
Status: NEW → ASSIGNED
This patch properly handles nested arrays (and always type-checks the output of an enumerated array's nextObject to make sure it's the expected type for the given code branch, so that we never assume what class of object we're being given.).

I walked well into the guts of Gecko trying to see where (or verify if) Gecko is stopping redundant loads; although there were several promising code-hunks, none of them were being hit.  After spending some time tracing load codepaths with a spinning head, I gave up. :-(

So, I'm happy to try to implement a short-circuit counter à la comment 8 point 2 in our code given some guidance/pointers, but I think, practically speaking (per testing in comment 8 and comment 9), we're safe without it.
Attachment #592474 - Attachment is obsolete: true
Attachment #593733 - Flags: superreview?(stuart.morgan+bugzilla)
Attached file AppleScript used in testing (obsolete) —
Lisa, can you have a look at this script and see if there's anything you can think of that I didn't test but should have (either that should work or that people might try and which we should handle correctly [fail safely] even if we don't support)?

The script obviously won't compile properly in (Apple)Script Editor without a patched Camino, but I can make one if you'd like to take it for a spin.
Attachment #593734 - Flags: feedback?(lthompson.22)
AFAICT you've covered all the permutations for identifying tab/window. I don't know if referring to tab/window by variable needs to be tested independently 

  set theTab to current tab of front browser window
  reload theTab

since it works in other contexts. The second thing that comes to mind as far as what people might try, is to stick the command in a repeat loop. I don't know if that's worth testing for unforeseen consequences, beyond any that would already apply to running "open location" in a repeat loop.

Anyway, if you'd like to provide me a build, I would be happy to hammer on it. :-)
(In reply to Lisa Thompson from comment #12)
> AFAICT you've covered all the permutations for identifying tab/window. I
> don't know if referring to tab/window by variable needs to be tested
> independently 
> 
>   set theTab to current tab of front browser window
>   reload theTab
> 
> since it works in other contexts.

Yeah, that works, since theTab is just a reference to the underlying tab object.

> as what people might try, is to stick the command in a repeat loop. I don't
> know if that's worth testing for unforeseen consequences, beyond any that
> would already apply to running "open location" in a repeat loop.

A loop is just going to send us a series of one of the other underlying commands, so as long as there are not any variations on those we haven't accounted for, a loop won't matter.

> Anyway, if you'd like to provide me a build, I would be happy to hammer on
> it. :-)

http://www.ardisson.org/smokey/moz/Camino-2.1.1pre-i386-applescript.dmg (includes changes for both this bug and bug 718322)
I put the build and your test script through some paces and everything I could think of as far as identifying the tabs to be reloaded worked. 

But I did find one oddity. If I append "ignoring cache no" to any valid "reload" command, I get an error on run with the message "can't make no into type boolean". Event log says: 

error "Camino got an error: Can’t make no into type boolean." number -1700 from no to boolean

If I change the clause to "without ignoring cache", it runs fine. Same thing happens with appending "loading in background no" to an open location command. Now, I never use the "yes/no" syntax, always "true/false" and "with/without", so maybe I'm missing something here?
Hrm, I tested the yes/no syntax with both "reload ignoring cache" and "open location in background" before and they both worked fine.  I wonder if something changed between OS versions?

Thanks for finding this :) I'll try to dig into it later.
Lisa, can I get you to run some tests for me with some logging?  (Also, what OS version is this?)

1) In Terminal:
     defaults write org.mozilla.camino NSScriptingDebugLogLevel 1
2) Launch the Camino that has these commands
3) Open the Console
4) Run an AppleScript command with yes/no syntax and one with true/false (with/without)
5) Console should log something that looks like this:

Command: Camino Suite.reload\n	Direct Parameter: <NSIndexSpecifier: tabs 1 of allBrowserWindows 1>\n	Receivers: <NSIndexSpecifier: tabs 1 of allBrowserWindows 1>\n	Arguments:     {\n        ignoringCache = 0;\n    }

6) When you're done, you can
     defaults delete org.mozilla.camino NSScriptingDebugLogLevel
to keep from logging every script command you ever run against Camino ;-)

If you don't get that logging (I don't know where in the process the error is happening, and where in the process that logging is being generated), can you also try the extremely verbose AEDebugReceives?

1) In Terminal:
     env AEDebugReceives=1 open /path/to/with-reload/copy/of/Camino.app
2) Open the Console
3) Run an AppleScript command with yes/no syntax and one with true/false (with/without)
4) Console will log a giant spammy serialization of the raw AppleEvent:

AE2000 (4340): Received an event:
------oo start of event oo------
{ 1 } 'aevt':  MOZC/reld (i386){
          return id: 276299876 (0x10780064)
     transaction id: 0 (0x0)
  interaction level: 64 (0x40)
     reply required: 1 (0x1)
             remote: 0 (0x0)
      for recording: 0 (0x0)
         reply port: 52231 (0xcc07)
  target:
    { 1 } 'psn ':  8 bytes {
      { 0x0, 0x1c21c2 } (Script Editor)
    }
  fEventSourcePSN: { 0x0,0x1c21c2 } (Script Editor)
  optional attributes:
    { 1 } 'reco':  - 1 items {
      key 'csig' - 
        { 1 } 'magn':  4 bytes {
          65536l (0x10000)
        }
    }
  event data:
    { 1 } 'aevt':  - 2 items {
      key 'iCch' - 
        { 1 } 'fals':  0 bytes {
          false
        }
      key '----' - 
        { 1 } 'obj ':  - 4 items {
          key 'form' - 
            { 1 } 'enum':  4 bytes {
              'indx'
            }
          key 'want' - 
            { 1 } 'type':  4 bytes {
              'BTab'
            }
          key 'seld' - 
            { 1 } 'long':  4 bytes {
              1 (0x1)
            }
          key 'from' - 
            { 1 } 'obj ':  - 4 items {
              key 'form' - 
                { 1 } 'enum':  4 bytes {
                  'indx'
                }
              key 'want' - 
                { 1 } 'type':  4 bytes {
                  'BWin'
                }
              key 'seld' - 
                { 1 } 'long':  4 bytes {
                  1 (0x1)
                }
              key 'from' - 
                { -1 } 'null':  null descriptor
            }
        }
    }
}

I noticed in my testing here that the data for "no" is an 'enum' of 'no  ' whereas the data for "false"/"without" is a 'fals' of 'false' in the raw AppleEvent, but both are properly coerced into '0' by the time we reach Cocoa Scripting (NSScriptDebugLogging).  

So, based on the data I have so far, the working hypothesis is that something is broken on the OS level where yes/no aren't being properly coerced into 1/0 before handing them off to Camino's Cocoa Scripting support.

Oh, crazy question: have you tried rebooting, or using another user account?  I've noticed sometimes (more often than I'd like :/ ) my AppleScript runtime gets b0rked and the most basic commands start failing miserably (I suspect an exception has occurred, hasn't been caught, and has run amok in the runtime).  Do other applications with boolean params also fail? E.g.

tell application "TextEdit" to print document 1 print dialog yes
Oh, Console might already be logging a condensed form of the raw AppleEvent on error when NSScriptingDebugLogLevel is 1 (I just tried with actual 1/0):

> Error converting apple event to script command: -1700 
>     Original event: <NSAppleEventDescriptor: 'MOZC'\'reld'{ 'iCch':0, '----':'obj '{ 'form':'indx', 'want':'BTab', 'seld':1, 'from':'obj '{ 'form':'indx', 'want':'BWin', 'seld':1, 'from':'null'() } }, &'csig':65536 }> 
>     Offending object descriptor: <NSAppleEventDescriptor: 0> 
>     Expected type descriptor: <NSAppleEventDescriptor: 'bool'> 

However, it doesn't tell us what the type is like the big spammy AEDebugReceives logging does, and I think I'd like to know what that type is.
Mac OS X 10.6.8. Installed the latest from Software Update, rebooted, and repaired permissions. Still get the error.

Console output from the first set of tests looked like Comment 17 for the "ignoring cache no" case. Below is the output from the second set of tests for the "ignoring cache no" case. I will attach the full results from both tests for all cases.

Let me know if you still think this needs to be done on a fresh user account, I would have to get to it tomorrow.


env AEDebugReceives=1 open /Applications/Camino.app

"reload current tab of front browser window ignoring cache no"

	{ 1 } 'aevt':  MOZC/reld (i386){
	          return id: 331 (0x14b)
	     transaction id: 0 (0x0)
	  interaction level: 64 (0x40)
	     reply required: 1 (0x1)
	             remote: 0 (0x0)
	      for recording: 0 (0x0)
	         reply port: 433463 (0x69d37)
	  target:
	    { 1 } 'psn ':  8 bytes {
	      { 0x0, 0x1e01e } (AppleScript Editor)
	    }
	  fEventSourcePSN: { 0x0,0x1e01e } (AppleScript Editor)
	  optional attributes:
	    { 1 } 'reco':  - 1 items {
	      key 'csig' - 
	        { 1 } 'magn':  4 bytes {
	          65536l (0x10000)
	        }
	    }
	  event data:
	    { 1 } 'aevt':  - 2 items {
Logging for applescript runs of "reload" command with various usage of "ignoring cache"
Forgot to answer this.

> Do other applications with boolean params also fail?

Yesterday I did the sanity check with 

  tell application "Finder" to duplicate file "x" to "y" replacing yes/no

and it ran as expected. Your TextEdit script

  tell application "TextEdit" to print document 1 print dialog yes/no

also worked both ways with no problems.
(In reply to Lisa Thompson from comment #18)

Thanks!

> 	  event data:
> 	    { 1 } 'aevt':  - 2 items {

The fact this AppleEvent it truncated is never a good sign :-(


Anyway, based on your output, philippe's output on 10.7.3, the output of some additional logging I had philippe do for me, and mountains of heroic remote testing from philippe (we can reproduce this in a stock Apple sample code application, SimpleScriptingVerbs, including the default-compiled-for-10.6 binary Apple shipped) I'm absolutely certain this is an OS bug, and it appears to be related to use of .sdef instead of still(!) using the old-style .scriptTerminology and .scriptSuite files.

Since this is an OS bug, since it's a non-default syntax (according to Apple's docs, the with/without syntax is supposed to be the default), and since it errors inside Cocoa Scripting before it gets to our code (so there's nothing we can do about it), I don't think we should do anything here.

We could go back to converting the .sdef into three pairs of old-style .scriptTerminology and .scriptSuite files and the aete resource, but that's both ugly and a bit of a pain, and if this broke default syntax I'd do it, but for this, I don't think it's worth it.

Big thanks, Lisa and philippe, for all your help tracking down this Apple bug :)

In my copious free time, I'll reduce SimpleScriptingVerbs down to a minimal testcase and file a rdar:// with Apple.
Whiteboard: [needs rdar:// on comment 14]
Comment on attachment 593733 [details] [diff] [review]
Working patch, including force-reload support

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

::: src/appleevents/ScriptingSupport.mm
@@ +317,5 @@
>  
> +- (void)handleReloadScriptCommand:(NSScriptCommand *)command
> +{
> +  id browserWindowsToReload = [command evaluatedReceivers];
> +  BOOL cacheFlag = [[[command evaluatedArguments] objectForKey:@"ignoringCache"] boolValue]; 

cacheFlag is confusing; call it ignoreCache

@@ +318,5 @@
> +- (void)handleReloadScriptCommand:(NSScriptCommand *)command
> +{
> +  id browserWindowsToReload = [command evaluatedReceivers];
> +  BOOL cacheFlag = [[[command evaluatedArguments] objectForKey:@"ignoringCache"] boolValue]; 
> +  BrowserWindow *windowToReload;

Since this isn't actually used at top-level scope, move this to just before the while ((windowToReload = ...)) line, and add the type declaration to the windowToReload in the if.

@@ +328,5 @@
> +  }
> +  else if ([browserWindowsToReload isKindOfClass:[NSArray class]]) {
> +    NSEnumerator *windowsEnumerator = [browserWindowsToReload objectEnumerator];
> +    while ((windowToReload = [windowsEnumerator nextObject])) {
> +      if ([windowToReload isKindOfClass:[BrowserWindow class]]) {

This whole if/else is really just the outer block repeated again. Make a helper:

- (void)handleReloadForObject:(id)objectToReload ignoringCache:(BOOL)ignoreCache;

where the implementation is roughly:

if (<class is array>) {
  id objectToReload;
  while ((objectToReload = <next object>)) {
    [self handleReloadForObject:objectToReload ignoringCache:ignoreCache];
  }
}
else {
  <do the reload/error handling>
}

Then have this script method just call that with the pieces of the script call.

@@ +337,5 @@
> +      }
> +      else {
> +        // This shouldn't ever happen, but if it does, we need to hear about it.
> +        [[NSScriptCommand currentCommand] setScriptErrorNumber:NSArgumentsWrongScriptError];
> +        NSString *scriptingClassName = [(NSScriptClassDescription *)[NSScriptClassDescription classDescriptionForClass:[windowToReload class]] className];

Are these typecasts really necessary? The docs imply that NSScriptClassDescription classDescriptionForClass: should return that type already.

@@ +400,5 @@
>      [[self nativeWindow] performClose:self];
>    return nil;
>  }
>  
> +- (void)handleReloadScriptCommand:(NSScriptCommand *)command

Same restructuring here.

@@ +454,5 @@
> +    [[NSScriptCommand currentCommand] setScriptErrorString:[NSString stringWithFormat:@"A %@ can't be reloaded.", scriptingClassName]];
> +  }
> +}
> +
> +- (void)scriptableReload:(BOOL)cacheFlag

ignoreCache
Attachment #593733 - Flags: superreview?(stuart.morgan+bugzilla) → superreview-
(In reply to Stuart Morgan from comment #22)

> > +  else if ([browserWindowsToReload isKindOfClass:[NSArray class]]) {
> > +    NSEnumerator *windowsEnumerator = [browserWindowsToReload objectEnumerator];
> > +    while ((windowToReload = [windowsEnumerator nextObject])) {
> > +      if ([windowToReload isKindOfClass:[BrowserWindow class]]) {
> 
> This whole if/else is really just the outer block repeated again. Make a
> helper:

It felt like there had to be a better way than the semi-repetitive nested |if|s, but a recursive helper method didn't occur to me :)  I like this a whole lot better; it's clearer and saner, thanks!  I consolidated the comments to the beginning of the helper methods, because even without the nesting, I think it's still very helpful to have a reference to the types of receivers we should expect and what syntaxes produce them.

> > +        [[NSScriptCommand currentCommand] setScriptErrorNumber:NSArgumentsWrongScriptError];
> > +        NSString *scriptingClassName = [(NSScriptClassDescription *)[NSScriptClassDescription classDescriptionForClass:[windowToReload class]] className];
> 
> Are these typecasts really necessary? The docs imply that
> NSScriptClassDescription classDescriptionForClass: should return that type
> already.

It looks like Peter cargo-culted that from haas, and I cargo-culted it from him ;-)  I zapped them and forced the error condition, and it does appear they are not necessary when running on 10.5 and built with 10.4u SDK; perhaps in the past they were required?

Apple's sample code[1,2] does use the cast, but both examples are for initWithContainerClassDescription: rather than classDescriptionForClass:, so they're inconclusive.

[1] https://developer.apple.com/library/mac/#samplecode/SimpleScriptingPlugin/Listings/Element_m.html
[2] https://developer.apple.com/library/mac/#samplecode/SimpleScriptingObjects/Listings/Element_m.html

Also, this hunk:

-- (void)setCurrentTab:(BrowserWrapper *)newTabItemView;
+- (void)setCurrentTab:(BrowserWrapper *)newTabItemView

was a typo I found (how did this ever compile/work before?); I'll check it in separately.
Attachment #593733 - Attachment is obsolete: true
Attachment #597296 - Flags: superreview?(stuart.morgan+bugzilla)
While looking up the various places in Apple's docs that mentioned the NSScriptClassDescription, I came upon the list of object specifier types again, and I added some more.  

Once I got the right syntax down ("whose" always requires a plural subject, even though Script Editor is happy to compile with a singular subject :P), pretty much every one worked.  There are a couple of cases that didn't, but the bug is not in reload, but rather in some object specifier support, which I filed as bug 727374.  They're pretty esoteric cases--far much moreso than yes/no--so I don't think anyone will hit them, in regular use or with "reload".
Attachment #593734 - Attachment is obsolete: true
Attachment #593734 - Flags: feedback?(lthompson.22)
Comment on attachment 597296 [details] [diff] [review]
v4.6, sr comments addressed

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

sr=smorgan with nits

::: src/appleevents/ScriptingSupport.mm
@@ +434,5 @@
> +- (void)scriptableReload:(BOOL)ignoreCache
> +{
> +  unsigned int reloadFlag = NSLoadFlagsNone;
> +  if (ignoreCache)
> +    reloadFlag = NSLoadFlagsBypassCacheAndProxy;

I think these three lines would be more readable as a ternary:
  unsigned int reloadFlag = ignoreCache ?
      NSLoadFlagsBypassCacheAndProxy : NSLoadFlagsNone;

@@ +435,5 @@
> +{
> +  unsigned int reloadFlag = NSLoadFlagsNone;
> +  if (ignoreCache)
> +    reloadFlag = NSLoadFlagsBypassCacheAndProxy;
> +  if ([self canReload]) {

I'd move this to the beginning and invert it to:
if (![self canReload])
  return;

No reason to muck about with flags if you can't use them.
Attachment #597296 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+
(In reply to Stuart Morgan from comment #25)
> I think these three lines would be more readable as a ternary:
>   unsigned int reloadFlag = ignoreCache ?
>       NSLoadFlagsBypassCacheAndProxy : NSLoadFlagsNone;

Agreed.

> I'd move this to the beginning and invert it to:
> if (![self canReload])
>   return;
> 
> No reason to muck about with flags if you can't use them.

Indeed; thanks!

http://hg.mozilla.org/camino/rev/b9eb83be2017 with those two changes.

Someone please remind me in a couple of weeks if I haven't filed the rdar:// yet.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs rdar:// on comment 14] → [camino-2.1.1][needs rdar:// on comment 14]
You need to log in before you can comment on or make changes to this bug.