Last Comment Bug 631770 - Implement onBeforeLinkTraversal in MailNews 3pane window
: Implement onBeforeLinkTraversal in MailNews 3pane window
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b3
Assigned To: Philip Chee
:
:
Mentors:
Depends on: 610736
Blocks: 649344
  Show dependency treegraph
 
Reported: 2011-02-05 02:43 PST by Philip Chee
Modified: 2011-04-13 05:16 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 dummy onBeforeLinkTraversal implementation. (1.20 KB, patch)
2011-02-05 02:47 PST, Philip Chee
mnyromyr: review+
neil: superreview+
Details | Diff | Splinter Review
Patch as checked in. (1.23 KB, patch)
2011-02-15 02:41 PST, Philip Chee
no flags Details | Diff | Splinter Review

Description Philip Chee 2011-02-05 02:43:50 PST
Bug 610736 implemented a dummy onBeforeLinkTraversal to keep the backend happy but we forgot that MailNews also has a tabbrowser. When testing the CTRL-F4 patch I kept getting this error:

Error: 'JavaScript component does not have a method named:
"onBeforeLinkTraversal"' when calling method: [nsIXULBrowserWindow::onBeforeLinkTraversal] =
NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED

Jens suggested:

> Just a guess: mailWindow.js (assignment on line 129, impl. starting on line 345).
Comment 1 Philip Chee 2011-02-05 02:47:59 PST
Created attachment 510003 [details] [diff] [review]
Patch v1.0 dummy onBeforeLinkTraversal implementation.

Just a SMOP.
Comment 2 neil@parkwaycc.co.uk 2011-02-05 06:47:12 PST
Comment on attachment 510003 [details] [diff] [review]
Patch v1.0 dummy onBeforeLinkTraversal implementation.

[Indentation style doesn't match existing weird style...]
Comment 3 Karsten Düsterloh 2011-02-14 15:03:15 PST
Comment on attachment 510003 [details] [diff] [review]
Patch v1.0 dummy onBeforeLinkTraversal implementation.

First of all: I wasn't able to reproduce the problem under Linux at all, hence I just do a formal text-based review. While I trust you it exists, I'd be glad to see some str… :-) 

(In reply to comment #2)
> [Indentation style doesn't match existing weird style...]

Well, it's called "GNU style".
Okay, agreed, it *is* weird. :-D
Nevertheless, we should not add more weird looking styled stuff, hence …

>     },
>+  // Called before links are navigated to to allow us to retarget them if needed.
>+  onBeforeLinkTraversal: function(originalTarget, linkURI, linkNode, isAppTab)
>+  {
>+    return originalTarget;
>+  },
>   QueryInterface : function(iid)
>     {

… add a blank line before and after your new block and keep indention as you have it.
Also, please use the 'a' prefix for function parameters.

r=me with that.
Comment 4 Philip Chee 2011-02-15 02:41:44 PST
Created attachment 512433 [details] [diff] [review]
Patch as checked in.

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/21879c945223

> First of all: I wasn't able to reproduce the problem under Linux at all, hence
> I just do a formal text-based review. While I trust you it exists, I'd be glad
> to see some str… :-) 

Open a few tabs, tab around, close a few tabs (CTRL-F4).

> … add a blank line before and after your new block and keep indention as you
> have it.
Fixed.

> Also, please use the 'a' prefix for function parameters.
Fixed.

> r=me with that.

Note You need to log in before you can comment on or make changes to this bug.