Issue 29333522: Issue 3515 - Adapted polyfill for URL class for consistency (Closed)
|
Created:
Jan. 14, 2016, 5:22 p.m. by Sebastian Noack Modified:
Jan. 19, 2016, 4:41 p.m. Reviewers:
kzar Visibility:
Public. |
DescriptionIssue 3515 - Adapted polyfill for URL class for consistency
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebased, fixed IIFE #Patch Set 3 : Use strict mode #
Total comments: 18
Patch Set 4 : Rebased and addressed comments #
MessagesTotal messages: 7
https://codereview.adblockplus.org/29333522/diff/29333523/safari/ext/backgrou... File safari/ext/background.js (left): https://codereview.adblockplus.org/29333522/diff/29333523/safari/ext/backgrou... safari/ext/background.js:170: var Frame = function(url, parent) Note that all changes in this file are the result of reverting the changes from #2172. This workaround isn't necessary anymore as the polyfill lives in an own file now which is loaded before ext.
https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js File lib/polyfill/url.js (right): https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:18: "use strict"; Again shouldn't this be inside the IIFE? https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:26: // Chrome <32 didn't implement any of those properties Couldn't we avoid checking these properties exist for versions 35 onwards? Something like: if ("URL" in global) return; else if ("webkitURL" in global) { global.URL = global.webkitURL; var URLProperties = ... https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:27: var URLProperties = ["href", "protocol", "host", "hostname", "pathname", "search"]; Nit: this line is too long https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:41: global.URL = function(url, baseUrl) A few things that I noticed behave differently with this: - Calling with a relative URL but no base URL does not throw an exception. - The hash part of the URL does not seem to be parsed. - The username and password parts of the URL also aren't parsed. - Calling with no arguments at all does not throw an exception. https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:43: if (baseUrl instanceof URL) should be `... instanceof global.URL)`?
https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js File lib/polyfill/url.js (right): https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:18: "use strict"; On 2016/01/16 16:05:28, kzar wrote: > Again shouldn't this be inside the IIFE? Again, it shouldn't. ;) Putting "use strict" on the top-level will turn on strict mode for the whole file, but not for other files even if they share the same context. https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:26: // Chrome <32 didn't implement any of those properties On 2016/01/16 16:05:28, kzar wrote: > Couldn't we avoid checking these properties exist for versions 35 onwards? > Something like: > > if ("URL" in global) > return; > else if ("webkitURL" in global) > { > global.URL = global.webkitURL; > var URLProperties = ... Note that Safari 6 and Chrome 32-34 uses the non-standard name webkitURL while it already provides these properties. And we need that list of properties anyway below. https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:27: var URLProperties = ["href", "protocol", "host", "hostname", "pathname", "search"]; On 2016/01/16 16:05:28, kzar wrote: > Nit: this line is too long Done. https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:41: global.URL = function(url, baseUrl) On 2016/01/16 16:05:28, kzar wrote: > A few things that I noticed behave differently with this: > > - The hash part of the URL does not seem to be parsed. > - The username and password parts of the URL also aren't parsed. As for the other two polyfills, it's not the objective to be 100% complete with the standard. But to only implement the properties we need. In particular with the approach here adding additional properties would increase the memory footprint. > - Calling with a relative URL but no base URL does not throw an exception. > - Calling with no arguments at all does not throw an exception. It's probably not worth adding complexity to handle that scenario. The reason this polyfill is as simple as it is is that we merely delegate to an <a> element. Also since we don't catch any exceptions thrown here, that inconsistency wouldn't make anything worse. And after all, the behavior doesn't change with this patch. https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:43: if (baseUrl instanceof URL) On 2016/01/16 16:05:28, kzar wrote: > should be `... instanceof global.URL)`? As in the other review, that isn't necessary, but let's put it into a local variable.
https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js File lib/polyfill/url.js (right): https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:18: "use strict"; On 2016/01/19 14:13:21, Sebastian Noack wrote: > On 2016/01/16 16:05:28, kzar wrote: > > Again shouldn't this be inside the IIFE? > > Again, it shouldn't. ;) > > Putting "use strict" on the top-level will turn on strict mode for the whole > file, but not for other files even if they share the same context. Acknowledged. https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:26: // Chrome <32 didn't implement any of those properties On 2016/01/19 14:13:21, Sebastian Noack wrote: > On 2016/01/16 16:05:28, kzar wrote: > > Couldn't we avoid checking these properties exist for versions 35 onwards? > > Something like: > > > > if ("URL" in global) > > return; > > else if ("webkitURL" in global) > > { > > global.URL = global.webkitURL; > > var URLProperties = ... > > Note that Safari 6 and Chrome 32-34 uses the non-standard name webkitURL while > it already provides these properties. And we need that list of properties anyway > below. Yea I understand that, just suggesting changing the logic so we don't check the properties if URL already exists. If URL already exists we know that we're running on a new enough version of Chrome / Safari as to provide URL with the correct properties, right? As far as I see it, the only time we're not sure if the correct properties are given is when webkitURL exists but not URL. https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:41: global.URL = function(url, baseUrl) On 2016/01/19 14:13:21, Sebastian Noack wrote: > On 2016/01/16 16:05:28, kzar wrote: > > A few things that I noticed behave differently with this: > > > > - The hash part of the URL does not seem to be parsed. > > - The username and password parts of the URL also aren't parsed. > > As for the other two polyfills, it's not the objective to be 100% complete with > the standard. But to only implement the properties we need. In particular with > the approach here adding additional properties would increase the memory > footprint. > > > - Calling with a relative URL but no base URL does not throw an exception. > > - Calling with no arguments at all does not throw an exception. > > It's probably not worth adding complexity to handle that scenario. The reason > this polyfill is as simple as it is is that we merely delegate to an <a> > element. Also since we don't catch any exceptions thrown here, that > inconsistency wouldn't make anything worse. > > And after all, the behavior doesn't change with this patch. I think with the missing properties, especially hash, we should include them. Or if not we should at least leave a comment here, I can imagine someone writing code that uses the hash of a URL, testing it on a modern browser and not thinking to check that the polyfill also supports the properties used. We could end up with undefined instead of the hash string, which depending on how it's used might not even throw an exception. https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:43: if (baseUrl instanceof URL) On 2016/01/19 14:13:21, Sebastian Noack wrote: > On 2016/01/16 16:05:28, kzar wrote: > > should be `... instanceof global.URL)`? > > As in the other review, that isn't necessary, but let's put it into a local > variable. Acknowledged.
https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js File lib/polyfill/url.js (right): https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:26: // Chrome <32 didn't implement any of those properties On 2016/01/19 15:24:02, kzar wrote: > On 2016/01/19 14:13:21, Sebastian Noack wrote: > > On 2016/01/16 16:05:28, kzar wrote: > > > Couldn't we avoid checking these properties exist for versions 35 onwards? > > > Something like: > > > > > > if ("URL" in global) > > > return; > > > else if ("webkitURL" in global) > > > { > > > global.URL = global.webkitURL; > > > var URLProperties = ... > > > > Note that Safari 6 and Chrome 32-34 uses the non-standard name webkitURL while > > it already provides these properties. And we need that list of properties > anyway > > below. > > Yea I understand that, just suggesting changing the logic so we don't check the > properties if URL already exists. If URL already exists we know that we're > running on a new enough version of Chrome / Safari as to provide URL with the > correct properties, right? > > As far as I see it, the only time we're not sure if the correct properties are > given is when webkitURL exists but not URL. That seems to be true. But skipping that check if window.URL exist doesn't make the code here any simpler. In fact it would be two additional lines, without much simplifying the remainder. However, if the check is always executed, that code path is always tested, even when running on the latest Chrome version. And performance-wise, it's only executed once at startup and the check is quite fast. So not worth any optimization. https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:41: global.URL = function(url, baseUrl) On 2016/01/19 15:24:02, kzar wrote: > On 2016/01/19 14:13:21, Sebastian Noack wrote: > > On 2016/01/16 16:05:28, kzar wrote: > > > A few things that I noticed behave differently with this: > > > > > > - The hash part of the URL does not seem to be parsed. > > > - The username and password parts of the URL also aren't parsed. > > > > As for the other two polyfills, it's not the objective to be 100% complete > with > > the standard. But to only implement the properties we need. In particular with > > the approach here adding additional properties would increase the memory > > footprint. > > > > > - Calling with a relative URL but no base URL does not throw an exception. > > > - Calling with no arguments at all does not throw an exception. > > > > It's probably not worth adding complexity to handle that scenario. The reason > > this polyfill is as simple as it is is that we merely delegate to an <a> > > element. Also since we don't catch any exceptions thrown here, that > > inconsistency wouldn't make anything worse. > > > > And after all, the behavior doesn't change with this patch. > > I think with the missing properties, especially hash, we should include them. Or > if not we should at least leave a comment here, I can imagine someone writing > code that uses the hash of a URL, testing it on a modern browser and not > thinking to check that the polyfill also supports the properties used. > > We could end up with undefined instead of the hash string, which depending on > how it's used might not even throw an exception. The way URL objects are currently used in Adblock Plus username, password and hash are explicitly ignored in stringifyURL anyway. That behavior is even documented there. And if in the future, URL objects should be used in another way, a comment wouldn't help either, if one only tests in the latest version of any browser. And still, this is unrelated of the change here, which is about unifying the polyfills, not about changing their behavior.
LGTM https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js File lib/polyfill/url.js (right): https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:26: // Chrome <32 didn't implement any of those properties On 2016/01/19 16:03:39, Sebastian Noack wrote: > On 2016/01/19 15:24:02, kzar wrote: > > On 2016/01/19 14:13:21, Sebastian Noack wrote: > > > On 2016/01/16 16:05:28, kzar wrote: > > > > Couldn't we avoid checking these properties exist for versions 35 onwards? > > > > Something like: > > > > > > > > if ("URL" in global) > > > > return; > > > > else if ("webkitURL" in global) > > > > { > > > > global.URL = global.webkitURL; > > > > var URLProperties = ... > > > > > > Note that Safari 6 and Chrome 32-34 uses the non-standard name webkitURL > while > > > it already provides these properties. And we need that list of properties > > anyway > > > below. > > > > Yea I understand that, just suggesting changing the logic so we don't check > the > > properties if URL already exists. If URL already exists we know that we're > > running on a new enough version of Chrome / Safari as to provide URL with the > > correct properties, right? > > > > As far as I see it, the only time we're not sure if the correct properties are > > given is when webkitURL exists but not URL. > > That seems to be true. But skipping that check if window.URL exist doesn't make > the code here any simpler. In fact it would be two additional lines, without > much simplifying the remainder. > > However, if the check is always executed, that code path is always tested, even > when running on the latest Chrome version. > > And performance-wise, it's only executed once at startup and the check is quite > fast. So not worth any optimization. Acknowledged. https://codereview.adblockplus.org/29333522/diff/29333626/lib/polyfill/url.js... lib/polyfill/url.js:41: global.URL = function(url, baseUrl) On 2016/01/19 16:03:39, Sebastian Noack wrote: > On 2016/01/19 15:24:02, kzar wrote: > > On 2016/01/19 14:13:21, Sebastian Noack wrote: > > > On 2016/01/16 16:05:28, kzar wrote: > > > > A few things that I noticed behave differently with this: > > > > > > > > - The hash part of the URL does not seem to be parsed. > > > > - The username and password parts of the URL also aren't parsed. > > > > > > As for the other two polyfills, it's not the objective to be 100% complete > > with > > > the standard. But to only implement the properties we need. In particular > with > > > the approach here adding additional properties would increase the memory > > > footprint. > > > > > > > - Calling with a relative URL but no base URL does not throw an > exception. > > > > - Calling with no arguments at all does not throw an exception. > > > > > > It's probably not worth adding complexity to handle that scenario. The > reason > > > this polyfill is as simple as it is is that we merely delegate to an <a> > > > element. Also since we don't catch any exceptions thrown here, that > > > inconsistency wouldn't make anything worse. > > > > > > And after all, the behavior doesn't change with this patch. > > > > I think with the missing properties, especially hash, we should include them. > Or > > if not we should at least leave a comment here, I can imagine someone writing > > code that uses the hash of a URL, testing it on a modern browser and not > > thinking to check that the polyfill also supports the properties used. > > > > We could end up with undefined instead of the hash string, which depending on > > how it's used might not even throw an exception. > > The way URL objects are currently used in Adblock Plus username, password and > hash are explicitly ignored in stringifyURL anyway. That behavior is even > documented there. > > And if in the future, URL objects should be used in another way, a comment > wouldn't help either, if one only tests in the latest version of any browser. > > And still, this is unrelated of the change here, which is about unifying the > polyfills, not about changing their behavior. Acknowledged. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
