drop requirement that tabs "url" parameters be URL objects

RESOLVED FIXED

Status

Add-on SDK
General
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: myk, Assigned: myk)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
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)

Comment 2

8 years ago
Created attachment 452198 [details] [diff] [review]
patch v1: drops requirement
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+
(Assignee)

Comment 4

8 years ago
Dietrich: are you cool with us landing this for 0.5 and respinning for it?

Comment 5

8 years ago
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).

Updated

8 years ago
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.
(Assignee)

Comment 7

8 years ago
Fixed by changeset https://hg.mozilla.org/labs/jetpack-sdk/rev/3236e68e6f2e.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

7 years ago
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.