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

Status: RESOLVED FIXED

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

Description

[Matteo Ferretti [:zer0] [:matteo]]

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…).

Comment 1

[Dave Townsend [:mossop]]

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.

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

It looks more explicit, and would be consistent with the way we're using relative path in `require`.

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 4

[Dave Townsend [:mossop]]

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?

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

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?

Comment 6

[Dave Townsend [:mossop]]

(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.

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

(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.

Comment 8

[Dave Townsend [:mossop]]

(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.

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

(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.

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: pull/1551

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8456803 [details]
pull/1551

>https://github.com/mozilla/addon-sdk/pull/1551/files

Comment 13

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1551/files

Comment 14

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 15

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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!

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

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

[[github robot]]

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

Comment 18

[Matteo Ferretti [:zer0] [:matteo]]

(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.

Comment 20

[[github robot]]

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

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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.

Comment 22

[Matteo Ferretti [:zer0] [:matteo]]

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.