866532 - expose prePath property on URL instances

Status: RESOLVED FIXED

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

Description

[Erik Vold [:erikvold] (please needinfo? me)]

I wrote https://github.com/erikvold/image-blocker-jetpack/blob/master/lib/main.js yesterday and I needed the prePath, so that I could do prePath + path to get the url without the hash and querystring, but I couldn't use the URL module because it didn't expose this.

If you have a better way then please let me know.

Comment 1

[riadh.chtara]

Attachment: expose prePath from uri

Comment 2

[riadh.chtara]

Hey guys,
I tried to fix the problem.

Comment 3

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

Erik, Irakli, one of you want to take a look at this?

Comment 4

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

API change is fine by me, although I'd prefer to stick to established standard by node: http://nodejs.org/api/url.html

Comment 5

[Kris Maglione [:kmag]]

If you're considering an API change, you might want to consider making the properties mutable like they are for nsIURI objects. A simple way to accomplish Erik's use case would be to set the query and hash to null strings and read back the spec.

Comment 6

[riadh.chtara]

Attachment: node url style + making the properties mutable

Comment 7

[riadh.chtara]

Attachment: node url style + making the properties mutable

Comment 8

[riadh.chtara]

Hey guys
In the last patch, I tried making properties mutable and respect the node url style.
I hope I succeeded in that

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to riadh.chtara from comment #8)
> Hey guys
> In the last patch, I tried making properties mutable and respect the node
> url style.
> I hope I succeeded in that

This patch appears to be removing the host, scheme, and userPass properties, which it shouldn't do.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Kris Maglione [:kmag] from comment #5)
> If you're considering an API change, you might want to consider making the
> properties mutable like they are for nsIURI objects. A simple way to
> accomplish Erik's use case would be to set the query and hash to null
> strings and read back the spec.

I think it'd be most simple to expose the prePath, and we can make this another bug.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to riadh.chtara from comment #7)
> Created attachment 754104 [details] [diff] [review]
> node url style + making the properties mutable

We'll need some tests before landing, so let us know if you'd like some help with this.

Comment 12

[riadh.chtara]

OK

Comment 13

[riadh.chtara]

Attachment: tests

Comment 14

[riadh.chtara]

So what do you think?

Comment 15

[Jorge Villalobos [:jorgev] (he/him)]

Comment on attachment 754848 [details] [diff] [review]
tests

When posting a patch it's always a good idea to ask for review.

Comment 16

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #9)
> (In reply to riadh.chtara from comment #8)
> > Hey guys
> > In the last patch, I tried making properties mutable and respect the node
> > url style.
> > I hope I succeeded in that
> 
> This patch appears to be removing the host, scheme, and userPass properties,
> which it shouldn't do.

This is still an issue

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 754848 [details] [diff] [review]
tests

Review of attachment 754848 [details] [diff] [review]:
-----------------------------------------------------------------

We can't remove host, scheme, and userPass properties. The best we can do is deprecate them, by logging a deprecation warning when a developer tries to use the deprecated property; that should be done in a separate branch if we decide it's worth doing.

Comment 18

[riadh.chtara]

Attachment: adding deleted properties

Comment 19

[riadh.chtara]

Attachment: adding deleted properties

Comment 20

[riadh.chtara]

Attachment: adding deleted properties

Comment 21

[riadh.chtara]

Hey Erik,
I didn't know I should keep them
I should have thought about it because many addons will be broken because of that. :)
I don't have time to create the log message. Sorry for that
But i'm will keep working on this bug until every thing becomes fine

Comment 22

[riadh.chtara]

Hey Erik,
I didn't know I should keep them
I should have thought about it because many addons will be broken because of that. :)
I don't have time to create the log message. Sorry for that
But i will keep working on this bug until every thing becomes fine

Comment 23

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 755024 [details] [diff] [review]
adding deleted properties

Review of attachment 755024 [details] [diff] [review]:
-----------------------------------------------------------------

Just one remaining issue:

This patch seems to be replacing tests of say userPass with a test for auth, instead of replacing the test you should add a new one so that both properties are tested.

Same goes for scheme -> protocol

Comment 24

[riadh.chtara]

Attachment: testing

Comment 25

[riadh.chtara]

Attachment: testing

Comment 26

[riadh.chtara]

It is time to sleep now,
There still same poblems
So I will fix them tomorrow

Comment 27

[riadh.chtara]

Attachment: tests

Comment 28

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to riadh.chtara from comment #27)
> Created attachment 755228 [details] [diff] [review]
> tests

Alright I think the last thing that needs doing now is to test the added setters, sorry I didn't notice that sooner, this is very near done! :)

Comment 29

[riadh.chtara]

Attachment: url with test

Comment 30

[riadh.chtara]

I hope it will work now

Comment 31

[Erik Vold [:erikvold] (please needinfo? me)]

Am I applying this patch wrong? I'm doing this `patch < ~/bug.patch` and it's not working with the file attached afaict, tho it's worked for me in the past with your page-mod patches.

Comment 32

[Dave Townsend [:mossop]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #31)
> Am I applying this patch wrong? I'm doing this `patch < ~/bug.patch` and
> it's not working with the file attached afaict, tho it's worked for me in
> the past with your page-mod patches.

This is a patch against our code in m-c. YOu probably need pathc -p3 <patch to apply to the SDK

Comment 33

[riadh.chtara]

It's really strange because I used the same way to generate the patch.

Comment 34

[riadh.chtara]

Attachment: tests

Comment 35

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1018

Pointer to Github pull-request

Comment 36

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 757792 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1018

Irakli there is an api change in this patch that I don't think you have seen yet.

Comment 37

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

Erik I did notice API change, I don't really like it but I can leave with it. To be honest I would really prefer to just have an API compatible with http://nodejs.org/api/url.html and maybe some other `url/util` module for exposing additional features like this one via function.

That being said I don't really think this is worth bike-shedding and I'd rather let you decide and review this.

Comment 38

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/86b60e76f56795b4096bdbd1bd3c57b21d5ae6c7
Bug 866532: expose prePath property on URL instances and making URL properties writable

Conflicts:
	lib/sdk/url.js
	test/test-url.js

https://github.com/mozilla/addon-sdk/commit/f903227c9922d32939e96046dfec0bc46289637b
Bug 866532: Fixing broken tests from the previous merge

https://github.com/mozilla/addon-sdk/commit/6faf1d06dc0ede94c9bfcc38858b1889d5ec7233
Merge pull request #1018 from erikvold/866532v2

Bug 866532: expose prePath property on URL instances and making URL properties writable a=@gozala r=@erikvold

Comment 39

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/98e6f8cd5efdc8d78dcaaeba6f7d36f7fd93b215
Bug 866532: expose prePath property on URL instances and making URL properties writable a=@gozala r=@erikvold

Comment 40

[[github robot]]

Commits pushed to australis at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/86b60e76f56795b4096bdbd1bd3c57b21d5ae6c7
Bug 866532: expose prePath property on URL instances and making URL properties writable

https://github.com/mozilla/addon-sdk/commit/f903227c9922d32939e96046dfec0bc46289637b
Bug 866532: Fixing broken tests from the previous merge

https://github.com/mozilla/addon-sdk/commit/6faf1d06dc0ede94c9bfcc38858b1889d5ec7233
Merge pull request #1018 from erikvold/866532v2

https://github.com/mozilla/addon-sdk/commit/98e6f8cd5efdc8d78dcaaeba6f7d36f7fd93b215
Bug 866532: expose prePath property on URL instances and making URL properties writable a=@gozala r=@erikvold

Comment 41

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

I'm going through the list of open bugs that github robot has commented in. Is this bug fixed, Erik?