Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11571008:
[Android] Add API for specifying a charset for data Urls. (Closed)
|
Created:
8 years ago by benm (inactive) Modified:
8 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Android] Add API for specifying a charset for data Urls.
Currently it's not possible to change the charset of a data URL
via the content.browser.LoadUrlParams API. Add an overload of the
methods that build data URLs to support that.
Android only change and android bots green.
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173487
Patch Set 1 #Patch Set 2 : Fix old tests and add a new one. #Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 2
Patch Set 5 : only apply charset if it's a text/* mime type #Patch Set 6 : #Patch Set 7 : #
Total comments: 4
Patch Set 8 : fix joth's comments. #
Messages
Total messages: 17 (0 generated)
ptal, thanks!
lgtm, just nits: https://codereview.chromium.org/11571008/diff/7001/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java (right): https://codereview.chromium.org/11571008/diff/7001/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:163: nit: spurious \n ? https://codereview.chromium.org/11571008/diff/7001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java (right): https://codereview.chromium.org/11571008/diff/7001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:100: * and virtual url. maybe: since this is public, maybe "uses US-ASCII as encoding" ?
thanks marcus! https://codereview.chromium.org/11571008/diff/7001/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java (right): https://codereview.chromium.org/11571008/diff/7001/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:163: On 2012/12/14 16:30:26, bulach wrote: > nit: spurious \n ? Done. https://codereview.chromium.org/11571008/diff/7001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java (right): https://codereview.chromium.org/11571008/diff/7001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:100: * and virtual url. will do, and on the other one above.
https://codereview.chromium.org/11571008/diff/11001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java (right): https://codereview.chromium.org/11571008/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:86: dataUrl.append(";charset=" + charset); -AIUI the charset is spurious if the mimetype is not text/* -- what would it mean to have a shift-jis charset JPEG or UCS-32 .exe ? (if the payload is base64 encoded, the charset applies to the decoded form, I believe. the base64 encoded form is unavoidably us-ascii.) -the receiver defaults to US-ASCII so we can always omit it in that case too, but it doesn't make much odds if we include it anyway there.
thanks joth! https://codereview.chromium.org/11571008/diff/11001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java (right): https://codereview.chromium.org/11571008/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:86: dataUrl.append(";charset=" + charset); > -AIUI the charset is spurious if the mimetype is not text/* -- what would it > mean to have a shift-jis charset JPEG or UCS-32 .exe ? (if the payload is base64 > encoded, the charset applies to the decoded form, I believe. the base64 encoded > form is unavoidably us-ascii.) Good point. I think the charset is considered a parameter to the MIME type, so in all likelihood the underlying implementation will ignore it if it doesn't make sense given the MIME type (seems chromium does this). But I also agree it does seem a bit odd to append it when the embedder didn't specify it and to do so given the MIME type wouldn't make much sense. I suppose text/* is the only case that a charset does make sense so probably best to only add it in that case. > -the receiver defaults to US-ASCII so we can always omit it in that case too, > but it doesn't make much odds if we include it anyway there.
On 14 December 2012 10:13, <benm@chromium.org> wrote: > thanks joth! > > > > https://codereview.chromium.**org/11571008/diff/11001/** > content/public/android/java/**src/org/chromium/content/** > browser/LoadUrlParams.java<https://codereview.chromium.org/11571008/diff/11001/content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java> > File > content/public/android/java/**src/org/chromium/content/** > browser/LoadUrlParams.java > (right): > > https://codereview.chromium.**org/11571008/diff/11001/** > content/public/android/java/**src/org/chromium/content/** > browser/LoadUrlParams.java#**newcode86<https://codereview.chromium.org/11571008/diff/11001/content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java#newcode86> > content/public/android/java/**src/org/chromium/content/** > browser/LoadUrlParams.java:86: > dataUrl.append(";charset=" + charset); > >> -AIUI the charset is spurious if the mimetype is not text/* -- what >> > would it > >> mean to have a shift-jis charset JPEG or UCS-32 .exe ? (if the payload >> > is base64 > >> encoded, the charset applies to the decoded form, I believe. the >> > base64 encoded > >> form is unavoidably us-ascii.) >> > > Good point. I think the charset is considered a parameter to the MIME > type, so in all likelihood the underlying implementation will ignore it > if it doesn't make sense given the MIME type (seems chromium does this). > > But I also agree it does seem a bit odd to append it when the embedder > didn't specify it and to do so given the MIME type wouldn't make much > sense. I suppose text/* is the only case that a charset does make sense > so probably best to only add it in that case. > > > I think in content layer we should only add the charset if the client passes it in. i.e. not add anything in the overload without charset (or, if charset == null) Any special smarts we add about grepping for text/* or whatever we can then keep in aw/ layer :-) > -the receiver defaults to US-ASCII so we can always omit it in that >> > case too, > >> but it doesn't make much odds if we include it anyway there. >> > > https://codereview.chromium.**org/11571008/<https://codereview.chromium.org/1... >
On 14 December 2012 15:09, Jonathan Dixon <joth@chromium.org> wrote: > > > > On 14 December 2012 10:13, <benm@chromium.org> wrote: > >> thanks joth! >> >> >> >> https://codereview.chromium.**org/11571008/diff/11001/** >> content/public/android/java/**src/org/chromium/content/** >> browser/LoadUrlParams.java<https://codereview.chromium.org/11571008/diff/11001/content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java> >> File >> content/public/android/java/**src/org/chromium/content/** >> browser/LoadUrlParams.java >> (right): >> >> https://codereview.chromium.**org/11571008/diff/11001/** >> content/public/android/java/**src/org/chromium/content/** >> browser/LoadUrlParams.java#**newcode86<https://codereview.chromium.org/11571008/diff/11001/content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java#newcode86> >> content/public/android/java/**src/org/chromium/content/** >> browser/LoadUrlParams.java:86: >> dataUrl.append(";charset=" + charset); >> >>> -AIUI the charset is spurious if the mimetype is not text/* -- what >>> >> would it >> >>> mean to have a shift-jis charset JPEG or UCS-32 .exe ? (if the payload >>> >> is base64 >> >>> encoded, the charset applies to the decoded form, I believe. the >>> >> base64 encoded >> >>> form is unavoidably us-ascii.) >>> >> >> Good point. I think the charset is considered a parameter to the MIME >> type, so in all likelihood the underlying implementation will ignore it >> if it doesn't make sense given the MIME type (seems chromium does this). >> >> But I also agree it does seem a bit odd to append it when the embedder >> didn't specify it and to do so given the MIME type wouldn't make much >> sense. I suppose text/* is the only case that a charset does make sense >> so probably best to only add it in that case. >> >> >> I think in content layer we should only add the charset if the client > passes it in. i.e. not add anything in the overload without charset (or, if > charset == null) > > Any special smarts we add about grepping for text/* or whatever we can > then keep in aw/ layer :-) > > > nicht. we don't have a aw/ layer wrapper for this. so we'd have to push the smarts up to the glue. imo we could do the same with baseURL and historyUrl already: only add them if not null, and put the about:blank and whatnot special smarts into our higher layer. I guess.... we could just add a static factory function in aw/ that encapsulates all those, and keep the core LoadUrlParms class simple and dumb. WDYT? > > >> -the receiver defaults to US-ASCII so we can always omit it in that >>> >> case too, >> >>> but it doesn't make much odds if we include it anyway there. >>> >> >> https://codereview.chromium.**org/11571008/<https://codereview.chromium.org/1... >> > >
> > nicht. we don't have a aw/ layer wrapper for this. so we'd have to push > the smarts up to the glue. > imo we could do the same with baseURL and historyUrl already: only add them > if not null, and put the about:blank and whatnot special smarts into our > higher layer. I guess.... we could just add a static factory function in > aw/ that encapsulates all those, and keep the core LoadUrlParms class > simple and dumb. > WDYT? In my opinion, I think it makes sense for the LoadUrlParams class to be able to specify a charset for the data url - it's part of the spec after all. Agree that it probably makes sense for the overload to do nothing, that way don't need all the test expectation changes and we won't inject a default charset unnecessarily (just adding functionality, not changing old behavior). Then we can keep the weird logic that uses this new functionality in aw/ or the glue layer. I'd rather not touch the base URL and history URL stuff in this patch.
On 17 December 2012 02:41, <benm@chromium.org> wrote: > > nicht. we don't have a aw/ layer wrapper for this. so we'd have to push >> the smarts up to the glue. >> imo we could do the same with baseURL and historyUrl already: only add >> them >> if not null, and put the about:blank and whatnot special smarts into our >> higher layer. I guess.... we could just add a static factory function in >> aw/ that encapsulates all those, and keep the core LoadUrlParms class >> simple and dumb. >> WDYT? >> > > > In my opinion, I think it makes sense for the LoadUrlParams class to be > able to > specify a charset for the data url - it's part of the spec after all. Totally. just to clarify I was thinking in content/ we just have something simple like:- if (charset != null) dataUrl.append(";charset=" + charset); and then our factory function in aw/ would do the special logic working out whether to pass charset down or not. Agree that > it probably makes sense for the overload to do nothing, that way don't > need all > the test expectation changes and we won't inject a default charset > unnecessarily > (just adding functionality, not changing old behavior). Then we can keep > the > weird logic that uses this new functionality in aw/ or the glue layer. I'd > rather not touch the base URL and history URL stuff in this patch. > > https://codereview.chromium.**org/11571008/<https://codereview.chromium.org/1... >
Great, patch set 7 should be exactly that! :) (except weird logic in glue layer downstream not aw/)
On 2012/12/17 17:46:17, benm wrote: > Great, patch set 7 should be exactly that! :) (except weird logic in glue layer > downstream not aw/) (downstream as eventually I'd like to deprecate the weird API and introduce something a little more sensible)
lgtm https://codereview.chromium.org/11571008/diff/18001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java (right): https://codereview.chromium.org/11571008/diff/18001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:82: if (charset != null && !charset.isEmpty()) { Do we like this style? if (!"".equals(charset)) ... https://codereview.chromium.org/11571008/diff/18001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:138: // baseUrl and historyUrl. Out of scope for this patch to change this, but could you add // TODO(joth): we should just append baseURL and historyURL here, and move the WebView specific transform up to a wrapper factory function in android_webview/ layer.
thanks joth! https://codereview.chromium.org/11571008/diff/18001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java (right): https://codereview.chromium.org/11571008/diff/18001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:82: if (charset != null && !charset.isEmpty()) { Not sure there's an official convention, but that's a bit harder to parse IMO, so prefer to leave as is unless you feel strongly... https://codereview.chromium.org/11571008/diff/18001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:138: // baseUrl and historyUrl. will do
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benm@chromium.org/11571008/21002
On 17 December 2012 10:00, <benm@chromium.org> wrote: > thanks joth! > > > > https://codereview.chromium.**org/11571008/diff/18001/** > content/public/android/java/**src/org/chromium/content/** > browser/LoadUrlParams.java<https://codereview.chromium.org/11571008/diff/18001/content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java> > File > content/public/android/java/**src/org/chromium/content/** > browser/LoadUrlParams.java > (right): > > https://codereview.chromium.**org/11571008/diff/18001/** > content/public/android/java/**src/org/chromium/content/** > browser/LoadUrlParams.java#**newcode82<https://codereview.chromium.org/11571008/diff/18001/content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java#newcode82> > content/public/android/java/**src/org/chromium/content/** > browser/LoadUrlParams.java:82: > if (charset != null && !charset.isEmpty()) { > Not sure there's an official convention, but that's a bit harder to > parse IMO, so prefer to leave as is unless you feel strongly... > > I don't have any preference. I've seen both used, feel it would be nice to settle on one or other but I have no strong pref which. (hmm or third option: a TextUtils.isEmpty() style wrapper) > > https://codereview.chromium.**org/11571008/diff/18001/** > content/public/android/java/**src/org/chromium/content/** > browser/LoadUrlParams.java#**newcode138<https://codereview.chromium.org/11571008/diff/18001/content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java#newcode138> > content/public/android/java/**src/org/chromium/content/** > browser/LoadUrlParams.java:**138: > // baseUrl and historyUrl. > will do > > https://codereview.chromium.**org/11571008/<https://codereview.chromium.org/1... >
Message was sent while issue was closed.
Change committed as 173487 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
