Code review comment for lp:~camptocamp/openerp-web/6.1-lp1055349-url-widget

Revision history for this message
Xavier (Open ERP) (xmo-deactivatedaccount) wrote :

Although I believe the code could use RegExp#test(String) (https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/RegExp/test) rather than RegExp#exec(String), as the result matches are not used, the match is simply used as a boolean.

#test makes purpose clearer, and may be slightly more efficient.

And I don't know if you're shooting for full correctness (this is already an improvement over the current state of affairs) but there are a few potentjial issues:

* In the current code, with its existing semantics, everything after the `:` is redundant, the matchgroup isn't needed and it doesn't filter out anything more than not putting it at all.
* Because the match is not anchored to the start of the string (by `^`) and the colon can be in a fragment, the url `foo.com/#a:3` would pass the test though it should not
* The prepending will fail if the URL is of the form `//foo.com` (same scheme, different domain) (note: this means the match should probably remain a match)

review: Approve

« Back to merge proposal