Closed Bug 1251332 Opened 4 years ago Closed 4 years ago

PAC: functions for time based conditions don't support date/day/time "wrapping"

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: dmcdonald999, Assigned: xeonchen, NeedInfo)

Details

(Keywords: dev-doc-complete, Whiteboard: [necko-active])

Attachments

(2 files)

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
Whiteboard: [necko-backlog]
I'm going to work on this.
Assignee: nobody → xeonchen
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?
Attachment #8744819 - Flags: feedback?(mcmanus)
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
Attachment #8744819 - Flags: feedback?(mcmanus) → feedback+
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 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
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.
Flags: needinfo?(dmcdonald999)
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.
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!
Attachment #8745932 - Flags: review?(daniel)
Attachment #8745932 - Flags: review+
(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.
Whiteboard: [necko-backlog] → [necko-active]
Keywords: checkin-needed
Whiteboard: [necko-active]
Whiteboard: [necko-active]
https://hg.mozilla.org/mozilla-central/rev/cd913073f87c
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
make sure to mark this dev-doc-needed and provide a comment so the documentation team can update MDN. thanks.
Flags: needinfo?(xeonchen)
Now weekdayRange, dateRange, and timeRange support "reversed range", for example, weekdayRange("SAT", "MON") will evaluate true if current day is Saturday, Sunday, and Monday.
Flags: needinfo?(xeonchen)
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.