expose prePath property on URL instances

RESOLVED FIXED

Status

Add-on SDK
General
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: erikvold, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 11 obsolete attachments)

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.
Hardware: x86 → All
Whiteboard: [good first bug]

Updated

5 years ago
Priority: -- → P3

Comment 1

5 years ago
Created attachment 751388 [details] [diff] [review]
expose prePath  from uri

Comment 2

5 years ago
Hey guys,
I tried to fix the problem.
Erik, Irakli, one of you want to take a look at this?
Flags: needinfo?(rFobic)
Flags: needinfo?(evold)
API change is fine by me, although I'd prefer to stick to established standard by node: http://nodejs.org/api/url.html
Flags: needinfo?(rFobic)
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

5 years ago
Created attachment 754103 [details] [diff] [review]
node url style + making the properties mutable
Attachment #751388 - Attachment is obsolete: true
Attachment #751388 - Flags: review?(evold)

Comment 7

5 years ago
Created attachment 754104 [details] [diff] [review]
node url style + making the properties mutable
Attachment #754103 - Attachment is obsolete: true

Comment 8

5 years ago
Hey guys
In the last patch, I tried making properties mutable and respect the node url style.
I hope I succeeded in that
(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.
Flags: needinfo?(evold)
(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.
(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

5 years ago
OK

Comment 13

5 years ago
Created attachment 754848 [details] [diff] [review]
tests
Attachment #754104 - Attachment is obsolete: true

Comment 14

5 years ago
So what do you think?
Comment on attachment 754848 [details] [diff] [review]
tests

When posting a patch it's always a good idea to ask for review.
Attachment #754848 - Flags: review?(evold)
(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 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.
Attachment #754848 - Flags: review?(evold) → review-

Comment 18

5 years ago
Created attachment 755017 [details] [diff] [review]
adding deleted properties
Attachment #754848 - Attachment is obsolete: true
Attachment #755017 - Flags: review?(evold)

Comment 19

5 years ago
Created attachment 755019 [details] [diff] [review]
adding deleted properties
Attachment #755019 - Flags: review?(evold)

Comment 20

5 years ago
Created attachment 755024 [details] [diff] [review]
adding deleted properties
Attachment #755017 - Attachment is obsolete: true
Attachment #755019 - Attachment is obsolete: true
Attachment #755017 - Flags: review?(evold)
Attachment #755019 - Flags: review?(evold)
Attachment #755024 - Flags: review?(evold)

Comment 21

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

5 years ago
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 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
Attachment #755024 - Flags: review?(evold) → review+

Comment 24

5 years ago
Created attachment 755063 [details] [diff] [review]
testing
Attachment #755024 - Attachment is obsolete: true
Attachment #755063 - Flags: review?(evold)

Comment 25

5 years ago
Created attachment 755064 [details] [diff] [review]
testing
Attachment #755063 - Attachment is obsolete: true
Attachment #755063 - Flags: review?(evold)
Attachment #755064 - Flags: review?(evold)

Comment 26

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

Comment 27

5 years ago
Created attachment 755228 [details] [diff] [review]
tests
Attachment #755228 - Flags: review?(evold)
(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

5 years ago
Created attachment 756010 [details] [diff] [review]
url with test
Attachment #755064 - Attachment is obsolete: true
Attachment #755228 - Attachment is obsolete: true
Attachment #755064 - Flags: review?(evold)
Attachment #755228 - Flags: review?(evold)
Attachment #756010 - Flags: review?(evold)

Comment 30

5 years ago
I hope it will work now
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.
(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

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

Comment 34

5 years ago
Created attachment 756121 [details] [diff] [review]
tests
Attachment #756010 - Attachment is obsolete: true
Attachment #756010 - Flags: review?(evold)
Attachment #756121 - Flags: review?(evold)
Created attachment 757792 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1018

Pointer to Github pull-request
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.
Attachment #757792 - Flags: review?(rFobic)
Attachment #756121 - Flags: review?(evold) → review+
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.
Attachment #757792 - Flags: review?(rFobic) → review?(evold)

Comment 38

5 years ago
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
Attachment #757792 - Flags: review?(evold) → review+

Comment 39

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

5 years ago
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
I'm going through the list of open bugs that github robot has commented in. Is this bug fixed, Erik?
Flags: needinfo?(evold)
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(evold)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.