Closed Bug 515932 Opened 12 years ago Closed 12 years ago

loadOneTab and addTab should accept a hash of parameters, including a parameter for opening the tab next to the current one

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: dev-doc-complete, verified1.9.2)

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
For bug 515635, we need a way to open a tab next to the current one without sending the referrer.

Instead of just adding that parameter, I propose we also accept a hash of parameters, as the number of parameters is getting unreasonable and most callers just need a subset of them. That way, we can happily add more parameters without fearing that, if we added a tenth parameter that many callers would want to use, they would also have to set the eight previous parameters, even though they are optional.

So in bug 515635, instead of:

gBrowser.loadOneTab(submission.uri.spec, null, null, submission.postData, null, false, true);

... we'd do:

gBrowser.loadOneTab(submission.uri.spec, {postData: submission.postData, relatedToCurrent: false});

... which is easier to write, read and maintain.
Attachment #400014 - Flags: superreview?(mconnor)
Attachment #400014 - Flags: review?(gavin.sharp)
Blocks: 515635
(In reply to comment #0)
> gBrowser.loadOneTab(submission.uri.spec, {postData: submission.postData,
> relatedToCurrent: false});

this was meant to be relatedToCurrent: true
Attachment #400014 - Flags: review?(gavin.sharp) → review+
Comment on attachment 400014 [details] [diff] [review]
patch

I like it. Why add the  explicit aRelatedToCurrent parameter, though?
Dão, This patch is similar to Simon Bünzli's patch from bug 465673 of adding another parameter to addTab, which you yourself thought to be complicating things (bug 465673 comment #84).

While this adds the argument in a hash, which is better than just adding more arguments, surely the implied relationship from setting aReferrerURI covers this?

This bug makes the fixes in bug 514310 and bug 465673 redundant to some extent. In short, an extra parameter isn't needed if a referrer can convey the same information. It seems more consistent to use the referrer in this case as it is with middle-clicking or ctrl-clicking.
(In reply to comment #3)
> Dão, This patch is similar to Simon Bünzli's patch from bug 465673 of adding
> another parameter to addTab, which you yourself thought to be complicating
> things (bug 465673 comment #84).

The extra parameter is redundant when there's a referrer. And the number of parameters is inconvenient, but the ability to pass a hash addresses this.

> While this adds the argument in a hash, which is better than just adding more
> arguments, surely the implied relationship from setting aReferrerURI covers
> this?

Because we don't want to send a referrer to google in the case of bug 515635.
Attachment #400089 - Flags: superreview?(mconnor)
No longer blocks: 454518
Attachment #400089 - Flags: superreview?(mconnor) → superreview+
Comment on attachment 400089 [details] [diff] [review]
without an explicit aRelatedToCurrent

Yeah, let's go with this one.  I'm a fan.

Can we file a followup to change our existing callers to use this instead?  That way, extensions copying our code will do the better thing, and we can work on deprecating the giant bundle-o-params list in the future from a better place.
Attachment #400014 - Flags: superreview?(mconnor) → superreview-
We should also update MDC docs for this.
Keywords: dev-doc-needed
http://hg.mozilla.org/mozilla-central/rev/0137837043e0
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
Attachment #400089 - Flags: approval1.9.2?
Blocks: 516603
> +                                  ownerTab: owner,

Should this be aOwner like below?

> +              aOwner                = params.ownerTab;
No, aOwner isn't declared in the former case, but owner is.
Whiteboard: [doc-waiting-1.9.3]
Should loadURI or loadURIWithFlags also take a hash of the parameters for consistency?

loadURI takes 3 parameters and loadURIWithFlags takes 5.
Comment on attachment 400089 [details] [diff] [review]
without an explicit aRelatedToCurrent

Safe on its own, baking for most of a month and, while not exactly blocking bug
514327 (a P1 blocker), makes it substantially easier to land. a=me.

Dao - assuming you're the one landing these - please land them together with bug 514327 so that any regressions caused are clearly associated with the P1 blocker, and they get the appropriate level of immediate attention. In the future, if we need N patches to land a bug, please provide a single rollup patch to approve?
Attachment #400089 - Flags: approval1.9.2? → approval1.9.2+
This and bug 516603 aren't part of bug 514327, they just happen to touch the same code.
Depends on: 521065
Documentation updated:

https://developer.mozilla.org/en/XUL/Method/addTab
https://developer.mozilla.org/en/XUL/Method/loadOneTab

And this change is listed on the Firefox 3.6 overview page.
This signature doesn't exist:
addTab( URL, referrerURI, charset, postData, owner, allowThirdPartyFixup, relatedToCurrent )

It's either:
addTab( URL, referrerURI, charset, postData, owner, allowThirdPartyFixup )

Or:
addTab( URL, {referrerURI: ..., charset: ..., postData: ..., inBackground: ..., allowThirdPartyFixup: ..., relatedToCurrent: ...}), whereas all keys (referrerURI, charset, ...) are optional and the order doesn't matter.
Hm, that's non-obvious from the patch. I have no idea at all how to document that given our current documentation format. I'll have to think about that one.
(In reply to comment #16)
> addTab( URL, {referrerURI: ..., charset: ..., postData: ..., inBackground: ...,
> allowThirdPartyFixup: ..., relatedToCurrent: ...})

I copied this from the wrong method. inBackground is for loadOneTab. addTab has the ownerTab key instead.
I fixed these.
Perfect, thanks.
Attachment #400014 - Attachment is obsolete: true
Verified fixed based on check-ins and passed tests on trunk and 1.9.2.
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
You need to log in before you can comment on or make changes to this bug.