Last Comment Bug 595483 - Port duplicateTabIn introduced in bug 448546
: Port duplicateTabIn introduced in bug 448546
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b1
Assigned To: Philip Chee
:
:
Mentors:
Depends on: 448546
Blocks: SMtabAPI 636110
  Show dependency treegraph
 
Reported: 2010-09-11 04:31 PDT by Misak Khachatryan
Modified: 2011-02-23 02:13 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 add Firefox compatible duplicateTabIn() (2.12 KB, patch)
2010-09-23 08:58 PDT, Philip Chee
misak.bugzilla: feedback+
Details | Diff | Splinter Review
Patch 1.1 fix issues. (2.67 KB, patch)
2010-09-24 03:28 PDT, Philip Chee
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Patch v1.1a Patch as pushed. r=Neil sr=Neil (4.05 KB, patch)
2010-09-24 08:56 PDT, Philip Chee
no flags Details | Diff | Splinter Review

Description Misak Khachatryan 2010-09-11 04:31:31 PDT
bug 448546 introduces duplicateTabIn method, which will be nice to have for API compatibility.
Comment 1 Philip Chee 2010-09-23 08:58:13 PDT
Created attachment 477954 [details] [diff] [review]
Patch v1.0 add Firefox compatible duplicateTabIn()

Purely for Firefox compatibility (otherwise totally unnecessary)
Comment 2 neil@parkwaycc.co.uk 2010-09-23 09:45:18 PDT
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 2010-09-23 10:36:18 PDT
>>+  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 2010-09-24 03:28:34 PDT
Created attachment 478223 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-09-24 05:18:28 PDT
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] 2010-09-24 08:09:24 PDT
(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 2010-09-24 08:56:47 PDT
Created attachment 478281 [details] [diff] [review]
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 2010-09-24 09:10:05 PDT
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

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