Duncan McDonald Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160210153822

Steps to reproduce:

For any PAC time-based function:

1. weekdayRange("SUN", "WED") versus weekdayRange("WED", "SUN")
2. dateRange("JAN", "DEC") versus DateRange("DEC", "JAN")
3. timeRange(0, 23) versus timeRange(23, 0)



Actual results:

1. weekdayRange("SUN", "WED") will evaluate to true for the days SUN, MON, TUES, WED.  WeekdayRange("WED", "SUN") will only evaluate true if the current day is WED or SUN.

2. dateRange("JAN", "DEC") will always evaluate to true. DateRange("DEC", "JAN") will only evaluate true if the current month is December or January.

3. timeRange(0, 23) will always evaluate to true. TimeRange(23, 0) will only evaluate true if the current hour is 23:00 or midnight.


Expected results:

days, dates, and times should "wrap" so that, for instance, the range WED, SUN should include WED, THURS, FRI, SAT, SUN. See PAC documentation https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Necko/Proxy_Auto-Configuration_%28PAC%29_file

	Gary Chen [:xeonchen] Assignee
	Comment 1 • 8 years ago

I'm going to work on this.

	Gary Chen [:xeonchen] Assignee
	Comment 2 • 8 years ago

Hi Patrick,

I just made some modifications for this bug, but I'm not sure whether we're going to support this or not.
Would you give some feedback on this?

	Patrick McManus [:mcmanus]
	Comment 3 • 8 years ago

Comment on attachment 8744819 [details] [diff] [review]
0001-Bug-1251332-add-PAC-support-for-reversed-ranges-r-mc.patch

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

Thanks - I think this is desirable. Ask bagder to review when you're ready

	Gary Chen [:xeonchen] Assignee
	Comment 4 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/49177/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49177/

	Daniel Stenberg [:bagder]
	Comment 5 • 8 years ago

How's the support for this in the other popular browsers? Are they mostly following the spec as your patch here makes Firefox do?

	Gary Chen [:xeonchen] Assignee
	Comment 6 • 8 years ago

(In reply to Daniel Stenberg [:bagder] from comment #5)
> How's the support for this in the other popular browsers? Are they mostly
> following the spec as your patch here makes Firefox do?

In [1] it mentions the parameter cannot be reversed, and Chromium is simply using our implementation ([2]),
but in [3] and [4] they do support this semantic.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Necko/Proxy_Auto-Configuration_(PAC)_file#weekdayRange()
[2] https://code.google.com/p/chromium/codesearch#chromium/src/net/proxy/proxy_resolver_script.h&q=weekdayRange&sq=package:chromium&l=139
[3] https://searchcode.com/codesearch/view/3158298/
[4] https://docs.oracle.com/cd/E19575-01/821-0053/adysi/index.html

	Daniel Stenberg [:bagder]
	Comment 7 • 8 years ago

Ah, right. The spec actually specifically says they can't be reversed so this patch then goes against the spec while Chrome also has the same functionality as Firefox with Safari and Java supporting the reversed case.

Duncan, how come you ended up wanting this in the first place? What's the need for supporting the reversed?

I'm just generally cautious about violating a spec and changing long established behaviors without getting to know all the factors first.

	Patrick McManus [:mcmanus]
	Comment 8 • 8 years ago

There is no spec here - just documentation we can update. If it helps interop I don't see a reason to not take this. IE would be the most interesting.

	Daniel Stenberg [:bagder]
	Comment 9 • 8 years ago

Comment on attachment 8745932 [details]
MozReview Request: Bug 1251332 - add PAC support for reversed ranges; r?bagder

https://reviewboard.mozilla.org/r/49177/#review46429

looks fine!

	Gary Chen [:xeonchen] Assignee
	Comment 10 • 8 years ago

(In reply to Daniel Stenberg [:bagder] from comment #9)
> Comment on attachment 8745932 [details]
> MozReview Request: Bug 1251332 - add PAC support for reversed ranges;
> r?bagder
> 
> https://reviewboard.mozilla.org/r/49177/#review46429
> 
> looks fine!

Thank you Daniel!
Would you press review and ship buttons on MozReview if it's not troubled?
That will enable the function to land via MozReview.

	Gary Chen [:xeonchen] Assignee
	Comment 11 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6fe46bd9121

	Pulsebot
	Comment 12 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd913073f87c

	Daniel Stenberg [:bagder]
	Comment 13 • 8 years ago

https://reviewboard.mozilla.org/r/49177/#review46685

	Carsten Book [:Tomcat]
	Comment 14 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/cd913073f87c

	Patrick McManus [:mcmanus]
	Comment 15 • 8 years ago

make sure to mark this dev-doc-needed and provide a comment so the documentation team can update MDN. thanks.

	Gary Chen [:xeonchen] Assignee
	Comment 16 • 8 years ago

Now weekdayRange, dateRange, and timeRange support "reversed range", for example, weekdayRange("SAT", "MON") will evaluate true if current day is Saturday, Sunday, and Monday.

	Florian Scholz (Open Web Docs)
	Comment 17 • 8 years ago

Updated docs:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Necko/Proxy_Auto-Configuration_(PAC)_file#weekdayRange()

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Necko/Proxy_Auto-Configuration_(PAC)_file#dateRange()_2

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Necko/Proxy_Auto-Configuration_(PAC)_file#timeRange()_2

Thanks Duncan, xeonchen and galejwnaylor for contributing to MDN.

Developer release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/49#Networking