This will have a small perf implication because XPCOM (until we move uriFixup into JS...), but I don't believe this method is hot anyway, so I think we should do that rather than continue to risk making wrong decisions whenever we alter the fixup code.

Does this look sane to you, Dão? Marco told me you'd better review this. :-) Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=adf2b8e83426

... and once more, bzexport messes with what I want to do and uploads the empty trypush...

(In reply to :Gijs Kruitbosch from comment #0) > This will have a small perf implication because XPCOM (until we move > uriFixup into JS...), Twice... once for makeURI and once for createFixupURI. > but I don't believe this method is hot anyway, It's called during pageload. It sounds like this isn't urgent, so I think we should postpone this until uriFixup is implemented in JS and chrome and content are in separate processes.

(In reply to Dão Gottwald [:dao] from comment #3) > (In reply to :Gijs Kruitbosch from comment #0) > > This will have a small perf implication because XPCOM (until we move > > uriFixup into JS...), > > Twice... once for makeURI and once for createFixupURI. Yes. The only reason we need the makeURI call here is encodings; the URI creation process normalizes these, and so the result for mözilla or similar things doesn't match the input. It's possible to rearch this to mostly avoid that path by just checking against aURL first. I didn't think that was worth it. > > > but I don't believe this method is hot anyway, > > It's called during pageload. I'm fairly sure that at this point, xpcom overhead is such that a single call to this method will make no discernible difference (ie on the order of less than 10ms, quite likely less than 1ms, obviously depending on hardware etc.). I don't think this is enough overhead to justify not doing it. > > It sounds like this isn't urgent, so I think we should postpone this until > uriFixup is implemented in JS I could live with this, although I do think this is likely to come back to bite us sooner rather than later. I don't know how quickly uriFixup will move to JS, but it may still take months. > and chrome and content are in separate > processes. This is at least a year off, if not more, AIUI. I don't think we should live with the technical debt that long.

Comment on attachment 8517452 [details] [diff] [review] use uriFixup for trimURL, (In reply to :Gijs Kruitbosch from comment #4) > I'm fairly sure that at this point, xpcom overhead is such that a single > call to this method will make no discernible difference (ie on the order of > less than 10ms, quite likely less than 1ms, obviously depending on hardware > etc.). I don't think this is enough overhead to justify not doing it. I'm having a hard time justifying even a small overhead as long as it doesn't fix something that's clearly broken. > > It sounds like this isn't urgent, so I think we should postpone this until > > uriFixup is implemented in JS > > I could live with this, although I do think this is likely to come back to > bite us sooner rather than later. Seems rather unlikely to me since we lived with the regular expression for years. But when it comes to that, we can always still decide to take this patch.

(In reply to Dão Gottwald [:dao] from comment #5) > Comment on attachment 8517452 [details] [diff] [review] > use uriFixup for trimURL, > > (In reply to :Gijs Kruitbosch from comment #4) > > I'm fairly sure that at this point, xpcom overhead is such that a single > > call to this method will make no discernible difference (ie on the order of > > less than 10ms, quite likely less than 1ms, obviously depending on hardware > > etc.). I don't think this is enough overhead to justify not doing it. > > I'm having a hard time justifying even a small overhead as long as it > doesn't fix something that's clearly broken. I think duplicating the same algorithm in different parts of the code base is pretty clearly broken. I'd prefer correct over a tiny bit of speed and sometimes making the wrong decision. > > > It sounds like this isn't urgent, so I think we should postpone this until > > > uriFixup is implemented in JS > > > > I could live with this, although I do think this is likely to come back to > > bite us sooner rather than later. > > Seems rather unlikely to me since we lived with the regular expression for > years. And it was broken for a long time (also years)! And then we broke it some more for 33. There are still more changes to the url bar under consideration (like making a typed trailing slash make a difference in how we treat "foo" vs. "foo/", because other browsers do that and it's breaking people's muscle memory that we don't). I'd like to avoid accidentally breaking it again. Anyway, I went ahead and measured this. Measuring a 100-times repeated run of all the testVal tests (~35) in the browser_urlbarTrimURLs tests, it seems this change added less than 0.2ms per call to testVal (which implicitly, somewhere down the line, calls trimURL). And that's before bothering to optimize the existing patch any further. There's no disk IO involved here, so I don't think perf is really a very strong argument against this change. Yes, it's slightly slower than the 2 regex replaces that are there now, but by far not enough to be noticed.

Egh, I decided against unassigning after measuring.

Comment on attachment 8517452 [details] [diff] [review] use uriFixup for trimURL, >+ // We don't want to remove 'http://' if the host starts with "ftp\d*\." or contains "@" >+ let httpURLWithFTPOrAtSignRegex = /^http:\/\/(?:ftp\d*\.|[^\/]*@)/i; As I understand it, the URI fixup check should take care of this. >+ } catch (ex) { /* Don't care if this fails */ } Why not? Shouldn't we return url rather than urlWithoutProtocol if this fails?

(In reply to Dão Gottwald [:dao] from comment #8) > Comment on attachment 8517452 [details] [diff] [review] > use uriFixup for trimURL, > > >+ // We don't want to remove 'http://' if the host starts with "ftp\d*\." or contains "@" > >+ let httpURLWithFTPOrAtSignRegex = /^http:\/\/(?:ftp\d*\.|[^\/]*@)/i; > > As I understand it, the URI fixup check should take care of this. ftp, yes, because the fixup service will give you ftp://ftp instead of http://ftp, and so they won't match, so we'll keep http that way. usernames, no, because: Services.uriFixup.createFixupURI("foo@www.mozilla.org", Services.uriFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP | Services.uriFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS).spec returns the correct URL (with http) so that will allow the removal of the protocol to happen. I'll add a test for this (the current ones all specify passwords, too, which triggers different behaviour). > >+ } catch (ex) { /* Don't care if this fails */ } > > Why not? Shouldn't we return url rather than urlWithoutProtocol if this > fails? I don't think there are cases where these wouldn't be the same where the next check would (wrongly) let us return urlWithoutProtocol, but sure. (if you know of such cases, please let me know and I can add them to the test, too)

Now with fewer non-capturing groups.

(In reply to :Gijs Kruitbosch from comment #9) > usernames, no, because: > > Services.uriFixup.createFixupURI("foo@www.mozilla.org", > Services.uriFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP | > Services.uriFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS).spec > > returns the correct URL (with http) so that will allow the removal of the > protocol to happen. That's fine, as hitting enter in the location bar when foo@www.mozilla.org is displayed without the protocol will correctly restore http://. Where http:// must not be removed is http://user:pass@mozilla.org, and createFixupURI should take care of that too.

Hmm, I thought leaving the protocol for urls of type foo@domain.org was also done to reduce confusion with e.g. email addresses and such, but OK.

Comment on attachment 8522186 [details] [diff] [review] use uriFixup for trimURL, >+ // remove http:// >+ let urlWithoutProtocol = url.substring("http://".length); I'd prefer: let urlWithoutProtocol = url.substring(7); // remove http:// >+ let expectedURLSpec = aURL; let expectedURLSpec;

(In reply to Dão Gottwald [:dao] from comment #14) > >+ // remove http:// > >+ let urlWithoutProtocol = url.substring("http://".length); > > I'd prefer: > > let urlWithoutProtocol = url.substring(7); // remove http:// or even: // remove single trailing slash for http/https/ftp URLs let url = aURL.replace(/^((?:http|https|ftp):\/\/[^/]+)\/$/, "$1"); // remove http:// if (!url.startsWith("http://")) { return url; } let urlWithoutProtocol = url.substring(7);

with the suggestion from comment #14, remote: https://hg.mozilla.org/integration/fx-team/rev/5b4fe2faa70d

Marco, can you add this to the spreadsheet as well, please?

Added to IT 36.3