FWIW, I agree with tbroyer's assessment and the suggestion to use a helper sounds reasonable.
On Thu, Nov 3, 2016 at 10:40 AM, Thomas Broyer <[email protected]> wrote: > > > On Thursday, November 3, 2016 at 5:33:31 PM UTC+1, Colin Alworth wrote: >> >> With 3.0 on the horizon, we've promised consistency of a sort in 2.x, and >> without 3.0 actually in sight, 2.x is going to need to see active >> development. Encouraging a third generation of url tools is not a bad >> thing, but only switching over half-way leaves something to be desired. >> >> I'll play devil's advocate a bit - I'm not addressing the patch >> specifically (since I haven't fully read all of it or the discussion around >> it, and if it was a bad patch, I'm sure it would have been shot down on its >> own merits) but the thinking to use around what goes into the 2.x branch: >> * (point 1) We're not ready for maintenance mode, at least until we have >> timelines and completed APIs for GWT 3. If we are, we should be forbidding >> all merge requests to gwt-user that don't fix critical bugs. >> >> * (point 2 and 3) Can't argue with new tools arriving that solve the >> problem better, especially if they are going into 3 as a cross-platform way >> of solving the problem (gwt/java/j2cl). Obvious caveat here (even for the >> devil's advocate) that with the release of 3, breaking changes off of 2 are >> acceptable and expected. >> >> * (point 4) Without transitioning the existing GWT Safe* tools, >> SafeHtmlTemplates is stuck in the past, as is UiBinder, I18n Messages, and >> any HasSafeHtml widget (both in GWT and in the general ecosystem). This is >> a lot to leave behind in the name of "but the tool didn't belong in GWT in >> the first place". We've had our chance to properly deprecate it for the 2.8 >> release, and if our past timelines are any measure, it is going to be at >> least a year before we finally ship a release with SafeHtml, and all that >> use it, deprecated. And once we've reached that point, unless gwt-user >> depends on (or rebases, etc) safe-html-types or any other successor, all of >> the above downstream classes which use SafeHtml are left effectively broken >> (not unlike the unported Generators and Linkers we have today). >> >> Obviously ClientBundle/GssResource isn't actually deprecated, but we have >> officially said that new tools should not use the features that it takes >> advantage of. SafeHtml takes this a step further - GssResource got several >> upgrades within the 2.8 release (unlike other generators), despite it being >> deprecated, but with SafeHtml going out, it takes out features within many >> GWT Widgets themselves. There will be no officially supported way to >> correctly assign safe html content into an HTML widget. >> >> Now yes, a bit of hyperbole there - you can of course (as Thomas noted in >> his email) simply asString() the not-gwt SafeHtml and use it, or provide >> your own wrapper, but depending on your project size (and GWT users out >> there have some big ones) that could be hundreds or thousands of sites to >> fix in code. Yet another change would be needed for any widgets that the >> project has which implement HasSafeHtml (so it can still be used in >> UiBinder), UiBinder @UiFields or getters (which return SafeUri for use in >> its embedded SafeHtml handling), Messages (to wrap any incoming url). This >> ignores any use of Messages/Constants in a SafeHtml-using uibinder itself - >> I'm not seeing a clear path there without the use of default methods in >> I18n interfaces (which admittedly, would probably work, but isn't going to >> be pretty). >> > > You forgot one major point here: this patch is proposing a new API (new > overloads to existing methods), without actually changing the current API > (at least in terms of its contract). > So it's not about changing "every call sites for SafeHtml/UriUtils API", > it's about being able to take advantage of a new behavior by selectively > calling a new API. > If you want that behavior everywhere, then yes you'll have to update all > call sites; but the patch doesn't change that in any way (except what you > change your code *to*) > All those existing usage of SafeHtml you cited (SafeHtmlTemplates, > UiBinder, I18N and HasSafeHtml) would be unchanged and keeping their > current behavior (and only 2 of them deal with SafeUri: SafeHtmlTemplates > and UiBinder). And the only two cases where UriUtils is called by those > (vs. receiving a SafeUri value from the "user") are discouraged and > generate a warning (and would continue to only use the default whitelist > and reject "tel:" URIs; if you'd like to allow "tel:" URIs there, you'd > have to change to SafeUri anyway, and then the responsibility of converting > a String to a SafeUri falls on you, so no change to UriUtils or other > APIs/generators is actually *required*): > https://github.com/gwtproject/gwt/blob/3a41a8067767965ff72f4e49eb015f > 587fcfee16/user/src/com/google/gwt/uibinder/attributeparsers/ > SafeUriAttributeParser.java#L41 > https://github.com/gwtproject/gwt/blob/3a41a8067767965ff72f4e49eb015f > 587fcfee16/user/src/com/google/gwt/safehtml/rebind/ > SafeHtmlTemplatesImplMethodCreator.java#L183 (warning at: > https://github.com/gwtproject/gwt/blob/3a41a8067767965ff72f4e49eb015f > 587fcfee16/user/src/com/google/gwt/safehtml/rebind/ > SafeHtmlTemplatesImplMethodCreator.java#L364) > > After the security review, we're not in a situation of "I think GWT's > behavior should be changed (and this will affect everyone in many places)", > we're rather in a situation of "I'd like to add this new feature to GWT; > and this is something I could do in my own code" (delegating to the > existing behavior after I checked whether the new behavior I want would > apply). > > To put it in precise terms for this patch, it's a matter of changing > UriUtils.fromString(…) to either TelAwareUriUtils.fromString(…) > (application-specific factory that eventually delegate to > UriUtils.fromString) or UriUtils.fromString(…, > Collections.singleton("tel")) (new proposed API). That TelAwareUriUtils > would look like: > > public class TelAwareUriUtils { > public static SafeUri fromString(String uri) { > if (isTelUri(uri)) { > return UriUtils.fromTrustedString(uri); > } > return UriUtils.fromString(uri); > } > > // add similar isSafeUri and sanitizeUri methods > > private boolean isTelUri(String uri) { > return uri.regionMatches(true, 0, "tel:", 0, 4); // this is a > case-insensitive startsWith > } > } > > Speaking for myself again, I am not prepared to outright "-2" any >> contribution to gwt-user that isn't a fix for a critical bug. I don't think >> we're ready today to deprecate everything inside of gwt-user that isn't JRE >> emulation or jsinterop-based. Ideally of course aspects of GWT will be spun >> out as community projects and maintained separately, but until we pull it >> off (or even start), any new GWT user will assume that anything in gwt-user >> is the best basis for a GWT project. Actively abandoning most of gwt-user >> is not going to help the already difficult situation we're in there. >> > > +1, but we're not in that exact situation here. > > -- > You received this message because you are subscribed to the Google Groups > "GWT Contributors" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To view this discussion on the web visit https://groups.google.com/d/ > msgid/google-web-toolkit-contributors/cede6c7f-7428- > 42f0-bc42-8b1cb03cbecf%40googlegroups.com > <https://groups.google.com/d/msgid/google-web-toolkit-contributors/cede6c7f-7428-42f0-bc42-8b1cb03cbecf%40googlegroups.com?utm_medium=email&utm_source=footer> > . > > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "GWT Contributors" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/google-web-toolkit-contributors/CAN%3DyUA0c2wjzwsZnS0hj3Z27VBpZDTCdf9z_tCjsXbyB05E_qw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
