595483 - Port duplicateTabIn introduced in bug 448546

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Misak Khachatryan]

bug 448546 introduces duplicateTabIn method, which will be nice to have for API compatibility.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 add Firefox compatible duplicateTabIn()

Purely for Firefox compatibility (otherwise totally unnecessary)

Comment 2

[[e-mail]]

Comment on attachment 477954 [details] [diff] [review]
Patch v1.0 add Firefox compatible duplicateTabIn()

>+  aTab = aTab ? aTab : getBrowser().selectedTab;
At a quick glance, you could use aTab || getBrowser().selectedTab instead.

>+  var delta = aHistoryIndex == null ? currentIndex :
>+                                      aHistoryIndex - currentIndex;
Surely if aHistoryIndex is null then delta should be zero?

Comment 3

[Philip Chee]

>>+  var delta = aHistoryIndex == null ? currentIndex :
>>+                                      aHistoryIndex - currentIndex;
> Surely if aHistoryIndex is null then delta should be zero?

Yeah that's what I assumed at first too, but I couldn't match Firefox observed behaviour that way. Unfortunately I have to be compatible with their brain damaged implementation, so this silly construct.

Comment 4

[Philip Chee]

Attachment: Patch 1.1 fix issues.

>>+  aTab = aTab ? aTab : getBrowser().selectedTab;
> At a quick glance, you could use aTab || getBrowser().selectedTab instead.
Fixed.

>>+  var delta = aHistoryIndex == null ? currentIndex :
>>+                                      aHistoryIndex - currentIndex;
> Surely if aHistoryIndex is null then delta should be zero?
Sigh. Brain fade at 2am. Cut and pasted the wrong block into my active patch.
Fixed.

Comment 5

[[e-mail]]

Comment on attachment 478223 [details] [diff] [review]
Patch 1.1 fix issues.

>+/* Firefox compatibility shim *
>+ * duplicateTabIn duplicates tab in a place specified by the parameter |where|.
[This should have been a doc comment of course. Silly Firefox.]

>+ *  "tabfocused"  same as "tab" but override any background preferences and
>+                  focus the new tab
Nit: missing * on this line.

>+  var currentIndex = aTab.linkedBrowser.sessionHistory.index;
>+  var delta = aHistoryIndex == null ? 0 :
>+                                      aHistoryIndex - currentIndex;
Nit: fits in 80 characters without wrapping.

[duplicateTabIn(null, where) would have been nice, but not required for r+]

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #3)
> >>+  var delta = aHistoryIndex == null ? currentIndex :
> >>+                                      aHistoryIndex - currentIndex;
> > Surely if aHistoryIndex is null then delta should be zero?
> 
> Yeah that's what I assumed at first too, but I couldn't match Firefox observed
> behaviour that way. Unfortunately I have to be compatible with their brain
> damaged implementation, so this silly construct.

But you do realize that you can file bugs on Firefox code, don't you?

Comment 7

[Philip Chee]

Attachment: Patch v1.1a Patch as pushed. r=Neil sr=Neil

>>+ *  "tabfocused"  same as "tab" but override any background preferences and
>>+                  focus the new tab
> Nit: missing * on this line.
Fixed.

>>+  var currentIndex = aTab.linkedBrowser.sessionHistory.index;
>>+  var delta = aHistoryIndex == null ? 0 :
>>+                                      aHistoryIndex - currentIndex;
> Nit: fits in 80 characters without wrapping.
Fixed.

> [duplicateTabIn(null, where) would have been nice, but not required for r+]
Fixed using:
+  aTab = aTab || getBrowser().selectedTab;

> Dão Gottwald      2010-09-24 08:09:24 PDT
> 
> (In reply to comment #3)
>> behaviour that way. Unfortunately I have to be compatible with their brain
>> damaged implementation, so this silly construct.
Oops. Apologies for this rude comment.

> But you do realize that you can file bugs on Firefox code, don't you?
I think I've done that before, but not recently.

Comment 8

[Philip Chee]

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

Also reapplied the patch from Bug 593683 which I accidentally clobbered.
http://hg.mozilla.org/comm-central/rev/4d36594ea0e8