If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Adding './' as shortcut for 'data' folder in existing API

RESOLVED FIXED

Status

Add-on SDK
General
P2
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: zer0, Assigned: zer0)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
In our new UI Components, we're introducing relative path ('./') as alias for 'data' folder:

    Button({
      id: "foo",
      label: "foo",
      image: "./foo.png"
    });

So the user doesn't have to include `self` module all the time just for that, and use `data.url`.

We should use the same mechanism, that is backward compatible anyway, in all our high level API that can have a local URL (widgets, panels, contentScriptFile, contentStyleFile…).
Why do we even need to use "./"? Why not just take the string and interpret it relative to the data directory, if it includes a scheme the url code will automatically assume it is an absolute url.
(Assignee)

Comment 2

4 years ago
It looks more explicit, and would be consistent with the way we're using relative path in `require`.
(Assignee)

Comment 3

4 years ago
And, it's less error prone IMVHO: if the developer pass a generic string by mistake (e.g. a `toString` of some object) it won't be interpreted as relative path automatically, because it doesn't have the relative identifier, but it will try to resolve that string as scheme URL, and throw an exception that the developer can immediately see; instead of start to debug the code and try to figured out what's wrong.
It matches how the rest of the web works though, and presumably we'll log an error (with the attempted url) if an image can't be loaded?
(Assignee)

Comment 5

4 years ago
In the web you can still use "./", we're not breaking anything on that side.

We don't pre-load image so we don't log any error in SDK if the image can't be loaded (e.g. doesn't exist or it's not an image), we pass directly the URL to the XUL node. Firefox seems not logging any information too.

I'm the first one that want to have a more "web friendly" environment in jetpack; however in this case it seems just to be more safer and robust API have the relative path identifier, that is also consistent with all our relative path we have in jetpack.

Anyway, I have no particular strong feeling about it. We can add a check to see if the resource is existing and then pass to the XUL node.

If everyone agrees the steps will be:

- The string is a valid URL (non resource://), use it. Or…

- Normalize the string as resource:// URI via `data.url` (if it's not already a resource://)
- Open a channel to that URI in order to check if the resource is accessible
- If it's not, throw an exception / log an error
- If it is, use it

Irakli, what do you think about it?
Flags: needinfo?(rFobic)
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #5)
> In the web you can still use "./", we're not breaking anything on that side.

But on the web you can also just use the simpler "image.png" and expect it to work. It's much more common.

> We don't pre-load image so we don't log any error in SDK if the image can't
> be loaded (e.g. doesn't exist or it's not an image), we pass directly the
> URL to the XUL node. Firefox seems not logging any information too.

You can add error listeners to XUL image nodes.

> I'm the first one that want to have a more "web friendly" environment in
> jetpack; however in this case it seems just to be more safer and robust API
> have the relative path identifier, that is also consistent with all our
> relative path we have in jetpack.
> 
> Anyway, I have no particular strong feeling about it. We can add a check to
> see if the resource is existing and then pass to the XUL node.
> 
> If everyone agrees the steps will be:
> 
> - The string is a valid URL (non resource://), use it. Or…
> 
> - Normalize the string as resource:// URI via `data.url` (if it's not
> already a resource://)
> - Open a channel to that URI in order to check if the resource is accessible
> - If it's not, throw an exception / log an error
> - If it is, use it

That seems overkill.
(Assignee)

Comment 7

4 years ago
(In reply to Dave Townsend (:Mossop) from comment #6)

> > We don't pre-load image so we don't log any error in SDK if the image can't
> > be loaded (e.g. doesn't exist or it's not an image), we pass directly the
> > URL to the XUL node. Firefox seems not logging any information too.

> You can add error listeners to XUL image nodes.

I was trying to do something more generic and agnostic about the concrete implementation. What I proposed works for any kind of resources, not only images, and for images it doesn't really needs to know which kind of XUL element – and anonymous ones – is going to be created. IMVHO if we can avoid to deal with XUL is better.

> > Anyway, I have no particular strong feeling about it. We can add a check to
> > see if the resource is existing and then pass to the XUL node.
> > 
> > If everyone agrees the steps will be:
> > 
> > - The string is a valid URL (non resource://), use it. Or…
> > 
> > - Normalize the string as resource:// URI via `data.url` (if it's not
> > already a resource://)
> > - Open a channel to that URI in order to check if the resource is accessible
> > - If it's not, throw an exception / log an error
> > - If it is, use it
 
> That seems overkill.

I thought this was what you proposed. If we haven't a valid URL, then we're use the string as data's filename and log an error if it doesn't exist.
It seems to me less an overkill than deal with XUL, so we can have a generic function that makes this check for all the resources URL of our APIs.
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #7)
> (In reply to Dave Townsend (:Mossop) from comment #6)
> > > Anyway, I have no particular strong feeling about it. We can add a check to
> > > see if the resource is existing and then pass to the XUL node.
> > > 
> > > If everyone agrees the steps will be:
> > > 
> > > - The string is a valid URL (non resource://), use it. Or…
> > > 
> > > - Normalize the string as resource:// URI via `data.url` (if it's not
> > > already a resource://)
> > > - Open a channel to that URI in order to check if the resource is accessible
> > > - If it's not, throw an exception / log an error
> > > - If it is, use it
>  
> > That seems overkill.
> 
> I thought this was what you proposed. If we haven't a valid URL, then we're
> use the string as data's filename and log an error if it doesn't exist.
> It seems to me less an overkill than deal with XUL, so we can have a generic
> function that makes this check for all the resources URL of our APIs.

I was just proposing treating any string as relative to the data folder. If we particularly care about providing errors when a developer enters something incorrect then we should make use of whatever is doing the loading to provide that. Off the top of the head the only places I can think there we're loading these urls into are XUL or HTML image elements, browser/iframes or sandboxes. In all those cases we can detect load failures without having to do any additional steps ourselves.
(Assignee)

Comment 9

4 years ago
(In reply to Dave Townsend (:Mossop) from comment #8)

> I was just proposing treating any string as relative to the data folder.

But you also said that you expected that we already log an error if the image can't be loaded – and we don't atm.

> If
> we particularly care about providing errors when a developer enters
> something incorrect then we should make use of whatever is doing the loading
> to provide that.

That's to me seems an overkill: We would have to deal with different XUL/elements listeners, make that approach depends by the implementation, so the code won't be in one single place. It seems more reasonable to me limit to check if the resource exists – that is already better of what we have now – and make it generic and implementation agnostic.

> Off the top of the head the only places I can think there
> we're loading these urls into are XUL or HTML image elements,
> browser/iframes or sandboxes. In all those cases we can detect load failures
> without having to do any additional steps ourselves.

Bu those are additional steps: adding all the event listeners and make them per implementation is resulting in more code split in several place to maintain IMVHO, instead having one single check localized in one function.

Anyway, those are really implementation details, I think we should focus on the functionality. The two questions here are:

1. Should "./" mandatory or not – I'm ok to don't have it, I think it's more robust with, but I understand your point.

2. Should we notify the dev if the resource can't be loaded - I think we should, but in my opinion we should limit to check the if the resource exists.
> 1. Should "./" mandatory or not – I'm ok to don't have it, I think it's more robust with, but I understand your point.

I also prefer more explicit "./" form as it's non-brainer to test and resolve. Note contenScriptFile and others can point to other resource not but images. I do understand point that Mossop is making, and I would totally support it if we used some markup, which makes it little of an issue IMO.

I also don't feel very strong about about it, if Mossop does I'm fine with compromising ahead of time validation.

> 2. Should we notify the dev if the resource can't be loaded - I think we should, but in my opinion we should limit to check the if the resource exists.

I think we should just replace given string with `data.url(givenString)` if it's not absolute URL. Adding better validation and error handling may be a good idea, but that's orthogonal to this change.
Flags: needinfo?(rFobic)

Updated

4 years ago
Priority: -- → P2
(Assignee)

Updated

4 years ago
Assignee: nobody → zer0
(Assignee)

Comment 11

3 years ago
Created attachment 8456803 [details]
pull/1551
Attachment #8456803 - Flags: review?(jsantell)
(Assignee)

Comment 12

3 years ago
Comment on attachment 8456803 [details]
pull/1551

>https://github.com/mozilla/addon-sdk/pull/1551/files
Attachment #8456803 - Attachment mime type: text/plain → text/x-github-pull-request
Attachment #8456803 - Flags: review?(jsantell) → review?(rFobic)
(Assignee)

Comment 13

3 years ago
Created attachment 8456804 [details]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1551/files
Attachment #8456803 - Attachment is obsolete: true
Attachment #8456803 - Flags: review?(rFobic)
Attachment #8456804 - Flags: review?(rFobic)
(Assignee)

Comment 14

3 years ago
Not sure why it doesn't perform the redirect anymore, tried several time with different mime type too; but I think I'll just leave as is for the moment.
(Assignee)

Updated

3 years ago
Blocks: 1039446
Comment on attachment 8456804 [details]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1551/files

Thanks Zer0, I'm really happy this is actually happening!
Attachment #8456804 - Flags: review?(rFobic) → review+
I think with the move to jpm, where directory structures are more transparent, I'd prefer removing all the magic around the `data` directory, and opt to something closer to how node does it.

```
// in ./index.js
fs.readFile(__dirname + '/data/some.json')

PageMod({
  contentURL: __dirname + '/content-scripts/script.js'
});
```

There would be some magic on our end since these aren't file paths, and you couldn't `../` your way out of the path, as these would be resource URIs -- so something like `__dirname` global that is the resourceURI for the current file would still allow these paths to remain.

Some magic in our `fs` and `path` implementations would be necessary (path would `join` using OS separators, rather than file separators), and `fs` would have to work on resource URIs if given a resource URI in the first place.

Maybe these thoughts aren't mutually exclusive, and if given a `./local-url.js`, this'll work, and having a more node like path structure would still work.

Comment 17

3 years ago
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/21edd1154a365fedbfbf5ae5ccb3d01b48721c6f
Bug 870677 - Adding './' as shortcut for 'data' folder in existing AP

- Added shortcut and test cases

https://github.com/mozilla/addon-sdk/commit/43384d6c23b921561758a8b4be360bf0057defe3
Merge pull request #1551 from ZER0/data-shortcut/870677

fix Bug 870677 - Adding './' as shortcut for 'data' folder in existing AP r=@gozala

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

3 years ago
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #16)

> Maybe these thoughts aren't mutually exclusive, and if given a
> `./local-url.js`, this'll work, and having a more node like path structure
> would still work.

Yeah, I don't think they're mutually exclusive. Plus, we have this behavior already implemented in the UI APIs, this patch just makes the old APIs consistent.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 614894

Comment 20

3 years ago
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/7ecc24d33640774bde5a802a687065ad51988bf2
Bug 870677 - Adding './' as shortcut for 'data' folder in existing API

- Fixed bug introduced
- Removed priviled-panel add-on (now the test is in test-panel)

https://github.com/mozilla/addon-sdk/commit/e870add58c54e02cc1c92ef9c5d728b472b5e71e
Merge pull request #1552 from ZER0/data-shortcut/870677

Bug 870677 - Adding './' as shortcut for 'data' folder in existing API a=@gozala
This didn't get uplifted to Firefox until Firefox 34 branched off. 

Any chance we can get this pushed to at least Aurora 33? It'd make some addon code I'm working on a lot simpler to write, and waiting another release cycle for this change to get in would be annoying.
Flags: needinfo?(zer0)
(Assignee)

Comment 22

3 years ago
This patch is really across several different files; I'm not totally comfortable to cherry pick it; if some files are not exactly as expected, we could have some issue. We could try to apply to Aurora 33 and run the tests and see what happen.
Flags: needinfo?(zer0)
Depends on: 1195589
You need to log in before you can comment on or make changes to this bug.