Closed Bug 572977 Opened 14 years ago Closed 14 years ago

drop requirement that tabs "url" parameters be URL objects

Categories

(Add-on SDK Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: myk, Assigned: myk)

References

Details

Attachments

(1 file)

Tabs is currently the only API that requires "url" arguments to be URL objects, which makes its API anomalous. But it isn't the kind of API about which there is the security concern that prompted the requirement, since it doesn't accept HTML content, so such a requirement is also unwarranted in its case. Meanwhile, the requirement significantly complicates the simple case of loading a URL in a tab, which currently looks like this:

  require("tabs").open(require("url").URL("http://www.example.com/"));

when, if the API accepted strings, it could look like this much simpler and more readable call instead:

  require("tabs").open("http://www.example.com/");

This is totally my bad for not paying enough attention during the review phase and thinking through the ramifications of my earlier decision to standardize on URL objects across the API set.  And it's very late in the 0.5 cycle to be taking changes like this.

Nevertheless, I think it's worth taking a late-breaking fix to drop the requirement that these URL arguments be URL objects.  Then, as part of the 0.6 cycle, I'll take another look at the plan to standardize on URL objects across the API set and figure out what else (if anything) makes sense to do here.
Hmm, I just noticed that the tabs docs actually say "open" takes a string rather than a URL object, and they give several examples of that usage, which makes this more a blocker (whether we fix it by dropping the requirement or by fixing the docs) than a late-breaking change, since the docs are very out-of-sync with the code.

Dietrich: do you agree?
Assignee: nobody → myk
Status: NEW → ASSIGNED
Attachment #452198 - Flags: review?(dietrich)
Version: unspecified → Trunk
Comment on attachment 452198 [details] [diff] [review]
patch v1: drops requirement

yeah, strings are *way* nicer on the eyes. for a param that can only be a URL, strings should be fine. r=me.
Attachment #452198 - Flags: review?(dietrich) → review+
Dietrich: are you cool with us landing this for 0.5 and respinning for it?
Agreed. Aside from being hard on the eyes, it also complicates things for newcomers to the platform: in the tutorial in bug 566261, when we want to simply open a tab to a website, adding require("url").URL() requires us to explain what this mysterious URL() function is--and begs the question of why a simple string can't be passed in (which as Myk mentioned in comment 1 we don't actually have a good answer to).
Blocks: 566261
yes, land and respin!

Yeah, i misunderstood when we talked about this way-back-when in MV. i thought you guys wanted all URLs everywhere in the SDK to be URL objects, so was embarking on that mission. but it's a flawed mission.
Fixed by changeset https://hg.mozilla.org/labs/jetpack-sdk/rev/3236e68e6f2e.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: