Closed
Bug 998372
Opened 10 years ago
Closed 10 years ago
modernize the PAC FindProxyForURL function
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla31
People
(Reporter: froydnj, Assigned: froydnj)
Details
Attachments
(6 files, 2 obsolete files)
1.58 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
2.16 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
1.62 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
1.45 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
1.75 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
We can be more efficient and clearer, all at the same time.
Assignee | ||
Comment 1•10 years ago
|
||
We don't need to construct the regex on every call to the function. We only need to construct it once.
Attachment #8409017 -
Flags: review?(jmaher)
Assignee | ||
Comment 2•10 years ago
|
||
Similarly to the regular expression from part 1, we can lift out the construction of the known origins list. While we're at it, let's rename it and make it an object so we can do faster checks for known origins.
Attachment #8409019 -
Flags: review?(jmaher)
Assignee | ||
Comment 3•10 years ago
|
||
We can do everything with some better-named tables and variables that (I think) make it a little more obvious what's going on.
Attachment #8409020 -
Flags: review?(jmaher)
Assignee | ||
Comment 4•10 years ago
|
||
Finally, this comment is just old, and what we have clearly works, so let's just delete it.
Attachment #8409022 -
Flags: review?(jmaher)
Comment 5•10 years ago
|
||
Comment on attachment 8409017 [details] [diff] [review] part 1 - lift regex construction out of FindProxyForURL Review of attachment 8409017 [details] [diff] [review]: ----------------------------------------------------------------- thanks for the really small patch
Attachment #8409017 -
Flags: review?(jmaher) → review+
Comment 6•10 years ago
|
||
Comment on attachment 8409019 [details] [diff] [review] part 2 - lift origins out of FindProxyForURL and make origin lookups more efficient Review of attachment 8409019 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsChannelProperties.h @@ -31,3 @@ > #define NS_CHANNEL_PROP_CHANNEL_POLICY \ > NS_LITERAL_STRING(NS_CHANNEL_PROP_CHANNEL_POLICY_STR) > -#endif why is this in here? ::: testing/mozbase/mozprofile/mozprofile/permissions.py @@ +308,5 @@ > # to (\\\\d+) makes this code work. Not sure why there would be this > # difference between automation.py.in and this file. > pacURL = """data:text/plain, > +var knownOrigins = (function () { > + return [%(origins)s].reduce(function(t, h) { t[h] = true; return t; }, {}) I don't really understand this. are you just assigning true to all values h in t, not checking if it is true?
Attachment #8409019 -
Flags: review?(jmaher) → review-
Comment 7•10 years ago
|
||
Comment on attachment 8409020 [details] [diff] [review] part 3 - remove swathes of conditional logic from FindProxyForURL Review of attachment 8409020 [details] [diff] [review]: ----------------------------------------------------------------- this is nice and more clear!
Attachment #8409020 -
Flags: review?(jmaher) → review+
Comment 8•10 years ago
|
||
Comment on attachment 8409022 [details] [diff] [review] part 4 - remove old comment referring to automation.py.in Review of attachment 8409022 [details] [diff] [review]: ----------------------------------------------------------------- use your judgement ::: testing/mozbase/mozprofile/mozprofile/permissions.py @@ -305,5 @@ > > # TODO: this should live in a template! > - # TODO: So changing the 5th line of the regex below from (\\\\\\\\d+) > - # to (\\\\d+) makes this code work. Not sure why there would be this > - # difference between automation.py.in and this file. I think this is valuable information to retain (sans the automation.py.in stuff)
Attachment #8409022 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #6) > ::: netwerk/base/public/nsChannelProperties.h > @@ -31,3 @@ > > #define NS_CHANNEL_PROP_CHANNEL_POLICY \ > > NS_LITERAL_STRING(NS_CHANNEL_PROP_CHANNEL_POLICY_STR) > > -#endif > > why is this in here? Because I am bad at separating patches. I will remove it. > ::: testing/mozbase/mozprofile/mozprofile/permissions.py > @@ +308,5 @@ > > # to (\\\\d+) makes this code work. Not sure why there would be this > > # difference between automation.py.in and this file. > > pacURL = """data:text/plain, > > +var knownOrigins = (function () { > > + return [%(origins)s].reduce(function(t, h) { t[h] = true; return t; }, {}) > > I don't really understand this. are you just assigning true to all values h > in t, not checking if it is true? Right. I'm making each origin a key/property of an object, and setting all the corresponding values to |true|. Then I can just check "is this thing a property" later. I guess I could use a Set or similar, but this seemed like the easiest way to do things.
Assignee | ||
Comment 10•10 years ago
|
||
Here's part 2 again with the cruft removed. Ideally the explanation I gave previously will suffice.
Attachment #8409019 -
Attachment is obsolete: true
Attachment #8409684 -
Flags: review?(jmaher)
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #8) > Comment on attachment 8409022 [details] [diff] [review] > part 4 - remove old comment referring to automation.py.in > > Review of attachment 8409022 [details] [diff] [review]: > ----------------------------------------------------------------- > > use your judgement > > ::: testing/mozbase/mozprofile/mozprofile/permissions.py > @@ -305,5 @@ > > > > # TODO: this should live in a template! > > - # TODO: So changing the 5th line of the regex below from (\\\\\\\\d+) > > - # to (\\\\d+) makes this code work. Not sure why there would be this > > - # difference between automation.py.in and this file. > > I think this is valuable information to retain (sans the automation.py.in > stuff) If you remove the references to automation.py.in, then you just have this weird comment about changing escaping makes things work, standing alone, without any reference to the outside world. However, the comment bugged me enough that I went to look at why this is so. What happens currently is this: 1. Python reduces \\\\d to \\d when parsing the string. 2. We serialize the pref value via JSON (!) when outputting the prefs. JSON serialization will escape backslashes in strings. The string value therefore now has \\\\d. 3. The preferences reader handles backslash escaping when reading the string. So we're down to \\d. 4. The JavaScript engine also handles backslash escaping when reading the string for constructing the regexp object. So we have \d to pass to the regex engine's parser. 5. The regex engine's parser sees \d as the digit character class. Previously, when using automation.py.in, we did the following: 1. Python reduces \\\\\\\\d to \\\\d when parsing the string. 2. We wrote the string values out to the prefs file directly, without any JSON serialization. So we wrote \\\\d. 3. We can now pick up at step 3 in the sequence above. I guess I could try to distill that down to a comment if you like?
Flags: needinfo?(jmaher)
Comment 12•10 years ago
|
||
I think a simple comment indicating the multiple layers of escaping would be helpful.
Flags: needinfo?(jmaher)
Comment 13•10 years ago
|
||
Comment on attachment 8409684 [details] [diff] [review] part 2 - lift origins out of FindProxyForURL and make origin lookups more efficient Review of attachment 8409684 [details] [diff] [review]: ----------------------------------------------------------------- thanks!
Attachment #8409684 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 14•10 years ago
|
||
OK, r? on the wording.
Attachment #8409022 -
Attachment is obsolete: true
Attachment #8409693 -
Flags: review?(jmaher)
Comment 15•10 years ago
|
||
Comment on attachment 8409693 [details] [diff] [review] part 4 - explain the escape-fest in permissions.py better Review of attachment 8409693 [details] [diff] [review]: ----------------------------------------------------------------- thanks for the comment, that will help in the future.
Attachment #8409693 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 16•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/005a3405219c https://hg.mozilla.org/integration/mozilla-inbound/rev/0a42f8424ccb https://hg.mozilla.org/integration/mozilla-inbound/rev/4029dbea3291 https://hg.mozilla.org/integration/mozilla-inbound/rev/b9abacaf5453 Committed with a small change to part 3: the checking for what proxy to use was originally done on the original scheme from the URL, but in the posted patch, the checking was being done on the scheme used for origin checking. This change made several websocket-related tests fail, as they were being redirected to the HTTP proxy rather than the websocket proxy. Fixed in the obvious way with descriptive variable names.
Flags: in-testsuite+
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nfroyd
Assignee | ||
Comment 17•10 years ago
|
||
Backout because we have tests that care about the exact format for FindProxyForURL and I assumed those test failures were just intermittents: https://hg.mozilla.org/integration/mozilla-inbound/rev/8f28e657fcec
Assignee | ||
Comment 18•10 years ago
|
||
So we have tests for the exact format of FindProxyForURL. This patch goes with part 2 and changes the test to look for (part of) the new definition of knownOrigins. I didn't think it was worth adding the actual function arguments to reduce(). I'll fold this before commit.
Attachment #8410296 -
Flags: review?(jmaher)
Assignee | ||
Comment 19•10 years ago
|
||
The tests had checks for the conditional checks for what we should return as our proxy. To keep the same spirit, I just made the tests check for the key:value entries in the proxyForScheme hashtable. I'll fold this before commit.
Attachment #8410297 -
Flags: review?(jmaher)
Comment 20•10 years ago
|
||
Comment on attachment 8410296 [details] [diff] [review] test fix for origin rearrangement in part 2 Review of attachment 8410296 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mozbase/mozprofile/tests/permissions.py @@ +122,5 @@ > self.assertEqual(len(user_prefs), 2) > self.assertEqual(user_prefs[0], ('network.proxy.type', 2)) > self.assertEqual(user_prefs[1][0], 'network.proxy.autoconfig_url') > > + origins_decl = "var knownOrigins = (function () { return ['http://mochi.test:8888', 'http://127.0.0.1:80', 'http://127.0.0.1:8888'].reduce" are you missing a }) to close off the function and call?
Attachment #8410296 -
Flags: review?(jmaher) → review-
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #20) > Comment on attachment 8410296 [details] [diff] [review] > test fix for origin rearrangement in part 2 > > Review of attachment 8410296 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: testing/mozbase/mozprofile/tests/permissions.py > @@ +122,5 @@ > > self.assertEqual(len(user_prefs), 2) > > self.assertEqual(user_prefs[0], ('network.proxy.type', 2)) > > self.assertEqual(user_prefs[1][0], 'network.proxy.autoconfig_url') > > > > + origins_decl = "var knownOrigins = (function () { return ['http://mochi.test:8888', 'http://127.0.0.1:80', 'http://127.0.0.1:8888'].reduce" > > are you missing a }) to close off the function and call? No. I didn't think it'd be worth it to write out the entire thing. I can do that if you like.
Updated•10 years ago
|
Attachment #8410297 -
Flags: review?(jmaher) → review+
Comment 22•10 years ago
|
||
Comment on attachment 8410296 [details] [diff] [review] test fix for origin rearrangement in part 2 Review of attachment 8410296 [details] [diff] [review]: ----------------------------------------------------------------- resolved via irc, r+
Attachment #8410296 -
Flags: review- → review+
Assignee | ||
Comment 23•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c24e6699b71 https://hg.mozilla.org/integration/mozilla-inbound/rev/43c5d06184bb https://hg.mozilla.org/integration/mozilla-inbound/rev/cbbf7905937b https://hg.mozilla.org/integration/mozilla-inbound/rev/3c39d43169c0 Relanded with tests folded. Tests were green on my local machine.
https://hg.mozilla.org/mozilla-central/rev/0c24e6699b71 https://hg.mozilla.org/mozilla-central/rev/43c5d06184bb https://hg.mozilla.org/mozilla-central/rev/cbbf7905937b https://hg.mozilla.org/mozilla-central/rev/3c39d43169c0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•