Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1180793002:
Document attribute charset and its aliases to return UTF-8 (Closed)
|
Created:
5 years, 6 months ago by Habib Virji Modified:
5 years, 5 months ago CC:
arv+blink, blink-reviews, blink-reviews-dom_chromium.org, Inactive, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, vivekg_samsung, vivekg Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDocument attribute charset and its aliases to return UTF-8
Document::charset returns value of the document::encodingName.
The encodingName value is of type DocumentEncodingData, it has
m_encoding of type TextEncoding which holds the encoding type.
This type was not initialized and now has been updated to always
initialize to UTF-8 as per
<https://dom.spec.whatwg.org/#concept-document-encoding>;.
::characterSet replicates the functionality of the charset.
::inputEncoding uses characterSet. It has been updated to not
return nullable value.
document-attribute-js-null has been updated to reflect the
changes. Also updated documentgetinputencoding02.js as it is
based on old DOM3 and behavior does not match with DOM4
specification.
In Firefox only characterSet is supported and it returns an
"utf-8". It does not implement charset.
In IE all characterSet and charset are supported. It returns
for characterSet and charset UTF-8.
BUG=497982
R=philipj, haraken
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198563
Patch Set 1 #
Total comments: 2
Patch Set 2 : Clean up further Document.idl to not return undefined for null string #Patch Set 3 : Sets m_encodingData as UTF-8 by default. If DocumentEncodingData is empty in setEncoding, default v… #Patch Set 4 : Updated layout tests and defaultCharset to return UTF-8 #Patch Set 5 : Updated expectation file with UTF-8 #
Total comments: 11
Patch Set 6 : Removed defaultCharset changes #
Total comments: 6
Patch Set 7 : Add utf-8, utf8 and unicode-1-1-utf-8 as the return type #
Total comments: 2
Patch Set 8 : Updated as per philipj suggestions #
Messages
Total messages: 70 (19 generated)
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180793002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please take a look.
Given that these are non-standard attributes, would you describe the behavior of other browsers in the CL description?
In the description, please also describe the conditions under which these getters will return null, as that will help us make an educated guess about the risk of the change. https://codereview.chromium.org/1180793002/diff/1/LayoutTests/fast/dom/docume... File LayoutTests/fast/dom/document-attribute-js-null.html (right): https://codereview.chromium.org/1180793002/diff/1/LayoutTests/fast/dom/docume... LayoutTests/fast/dom/document-attribute-js-null.html:69: {name: 'charset', expectedNull: ''}, Can you also add characterSet, inputEncoding and defaultCharset here? Per spec characterSet and inputEncoding should actually never be null, but until we fix that it's nice to see that they all behave the same.
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180793002/20001
I have added a small description about what's supported. Let me know if more information should be added.
Thanks philip, updated test and also description of where it can return null. https://codereview.chromium.org/1180793002/diff/1/LayoutTests/fast/dom/docume... File LayoutTests/fast/dom/document-attribute-js-null.html (right): https://codereview.chromium.org/1180793002/diff/1/LayoutTests/fast/dom/docume... LayoutTests/fast/dom/document-attribute-js-null.html:69: {name: 'charset', expectedNull: ''}, On 2015/06/11 11:42:54, philipj wrote: > Can you also add characterSet, inputEncoding and defaultCharset here? Per spec > characterSet and inputEncoding should actually never be null, but until we fix > that it's nice to see that they all behave the same. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
Hmm, so the changes to characterSet and inputEncoding make it clear that this perhaps is trickier than it first seemed. What's really important here is what other browsers do, since ideally we should only change this one, directly to the final state. I tried things like |document.implementation.createHTMLDocument().characterSet| in Firefox and couldn't get an empty string, rather it seems to always be "UTF-8" for script-created documents. Did you try something else? The DOM spec <https://dom.spec.whatwg.org/#concept-document-encoding> says that "Unless stated otherwise, a document’s encoding is the utf-8 encoding" and that does seem to be the default in Gecko and IE11 alike. Probably the best thing for us would be to change all of these to always return "UTF-8" in any case where these currently return null. Then the [TreatReturnedNullStringAs=undefined] can be removed with no observable change. Possibly it's best to actually change how Document::m_encodingData is initialized, could you see what you think will work best here?
I had a look in the Gecko code in regards to characterSet value, particularly looking into GetDocumentCharacterSet and SetDocumentCharacterSet, but it does not say it returns an empty string. Apologise for creating the confusion. I agree the best way to change m_encodingDaqta initialization and check its usage. I will work on this today and upload the changes.
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180793002/40001
The CQ bit was unchecked by habib.virji@samsung.com
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180793002/60001
The CQ bit was unchecked by habib.virji@samsung.com
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180793002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks philip, it has been updated now to initialize m_encodingData by default to UTF-8. Since m_encodingData is of type DocumentEncodingData, it has been updated to initialize to UTF-8.
https://codereview.chromium.org/1180793002/diff/80001/LayoutTests/dom/xhtml/l... File LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js (right): https://codereview.chromium.org/1180793002/diff/80001/LayoutTests/dom/xhtml/l... LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js:107: assertEquals("documentgetinputencoding02", "UTF-8".toLowerCase(), encodingName.toLowerCase()); Why not compare the encodings case-sensitively? https://codereview.chromium.org/1180793002/diff/80001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1180793002/diff/80001/Source/core/dom/Documen... Source/core/dom/Document.cpp:1130: return settings->defaultTextEncodingName(); Is it possible that defaultTextEncodingName() is a null string? I think that's the default state, right? https://codereview.chromium.org/1180793002/diff/80001/Source/core/dom/Documen... Source/core/dom/Document.cpp:1131: return String("UTF-8"); It seems a bit strange return "UTF-8" here if that isn't the encoding that's going to be used by other callers to defaultTextEncodingName(). For |document.implementation.createHTMLDocument('').defaultCharset|, which code path is being taken, is settings null? https://codereview.chromium.org/1180793002/diff/80001/Source/core/dom/Documen... Source/core/dom/Document.cpp:4220: m_encodingData = DocumentEncodingData(); Is this code path reachable? Is there a test that would fail if it wasn't here?
Thanks philips. replied to your comments below. https://codereview.chromium.org/1180793002/diff/80001/LayoutTests/dom/xhtml/l... File LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js (right): https://codereview.chromium.org/1180793002/diff/80001/LayoutTests/dom/xhtml/l... LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js:107: assertEquals("documentgetinputencoding02", "UTF-8".toLowerCase(), encodingName.toLowerCase()); Saw this practice in other encoding file in test directory, so followed same method. Should I add more tests? https://codereview.chromium.org/1180793002/diff/80001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1180793002/diff/80001/Source/core/dom/Documen... Source/core/dom/Document.cpp:1130: return settings->defaultTextEncodingName(); Yes it is null string, with your below code snippet ( document.implementation.createHTMLDocument('').defaultCharset) it returns undefined in the current code. With the change it returns UTF-8. https://codereview.chromium.org/1180793002/diff/80001/Source/core/dom/Documen... Source/core/dom/Document.cpp:1131: return String("UTF-8"); |document.implementation.createHTMLDocument('').defaultCharset| will be null without this change as settings will be null. document-attribute-js-null.html without this string will be null and thus updated it to return correct value. https://codereview.chromium.org/1180793002/diff/80001/Source/core/dom/Documen... Source/core/dom/Document.cpp:4220: m_encodingData = DocumentEncodingData(); With the change in DocumentEncoderData it should not be.
@philip: do let me know if comments are okay or should work on any of the comment.
So defaultCharset was trickier than I had hoped, but let's deal with that separately. https://codereview.chromium.org/1180793002/diff/80001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1180793002/diff/80001/Source/core/dom/Documen... Source/core/dom/Document.cpp:1131: return String("UTF-8"); On 2015/06/15 16:14:55, Habib Virji wrote: > |document.implementation.createHTMLDocument('').defaultCharset| will be null > without this change as settings will be null. document-attribute-js-null.html > without this string will be null and thus updated it to return correct value. This case is tricky, I'm unsure what to do. It's strange to return anything other than the encoding that will actually be used, but in order to not depend on the settings object we'd have to make the default encoding a part of Document, and make createDocument() and createHTMLDocument() get the default encoding from the creating context. However, if the same logic isn't used in TextResourceDecoderBuilder::createDecoderInstance that would just be a fancy lie... Document.defaultCharset is non-standard so there's nothing to guide us here. I think the best thing is probably to leave it out of this CL and only deal with Document.characterSet and its many aliases here. When Document.defaultCharset is the last remaining user of [TreatReturnedNullStringAs=Undefined] let's try again. https://codereview.chromium.org/1180793002/diff/80001/Source/core/dom/Documen... Source/core/dom/Document.cpp:4220: m_encodingData = DocumentEncodingData(); On 2015/06/15 16:14:55, Habib Virji wrote: > With the change in DocumentEncoderData it should not be. OK, so would an ASSERT(newData.encoding().isValid()) here always pass? There are a few callers to Document::setEncodingData and it's not obvious at first glance. https://codereview.chromium.org/1180793002/diff/80001/Source/core/dom/Documen... File Source/core/dom/DocumentEncodingData.cpp (right): https://codereview.chromium.org/1180793002/diff/80001/Source/core/dom/Documen... Source/core/dom/DocumentEncodingData.cpp:39: : m_encoding("UTF-8") Could this use UTF8Encoding() instead of "UTF-8"?
Actually it looks like Document.defaultCharset and Document.charset are the only two remaining uses of [TreatReturnedNullStringAs=Undefined].
Actually it looks like Document.defaultCharset and Document.charset are the only two remaining uses of [TreatReturnedNullStringAs=Undefined].
philipj@opera.com changed reviewers: + jl@opera.com
haraken@, jl@, do you have any thoughts on what to do with Document.defaultCharset, and for that matter the rest of this CL? Usage is pretty high so just removing it isn't so easy: https://www.chromestatus.com/metrics/feature/timeline/popularity/428
On 2015/06/16 22:16:49, philipj wrote: > haraken@, jl@, do you have any thoughts on what to do with > Document.defaultCharset, and for that matter the rest of this CL? Usage is > pretty high so just removing it isn't so easy: > https://www.chromestatus.com/metrics/feature/timeline/popularity/428 Just to clarify: > In firefox only characterSet is supported and it returns an > "utf-8".It does not implement charset and defaultCharset. > > In IE all characterSet, charset and defaultCharset are > supported. What is the behavior of IE? It is great that we can remove [TreatReturnedNullStringAs=undefined], but we don't need to (shouldn't) do that if it breaks compatibility.
On 2015/06/16 22:21:06, haraken wrote: > On 2015/06/16 22:16:49, philipj wrote: > > haraken@, jl@, do you have any thoughts on what to do with > > Document.defaultCharset, and for that matter the rest of this CL? Usage is > > pretty high so just removing it isn't so easy: > > https://www.chromestatus.com/metrics/feature/timeline/popularity/428 > > Just to clarify: > > > In firefox only characterSet is supported and it returns an > > "utf-8".It does not implement charset and defaultCharset. > > > > In IE all characterSet, charset and defaultCharset are > > supported. > > What is the behavior of IE? > > It is great that we can remove [TreatReturnedNullStringAs=undefined], but we > don't need to (shouldn't) do that if it breaks compatibility. IE11: document.implementation.createDocument(null, null, null).characterSet => "utf-8" document.implementation.createDocument(null, null, null).charset => "utf-8" document.implementation.createDocument(null, null, null).defaultCharset => "windows-1252" document.implementation.createDocument(null, null, null).inputEncoding => "UTF-8" The results are the same for document.implementation.createHTMLDocument('').* I'm not sure if there are any other cases where IE might return null or undefined for these, but I don't know how to create a document which is more "orphaned" than these.
On 2015/06/16 22:43:52, philipj wrote: > On 2015/06/16 22:21:06, haraken wrote: > > On 2015/06/16 22:16:49, philipj wrote: > > > haraken@, jl@, do you have any thoughts on what to do with > > > Document.defaultCharset, and for that matter the rest of this CL? Usage is > > > pretty high so just removing it isn't so easy: > > > https://www.chromestatus.com/metrics/feature/timeline/popularity/428 > > > > Just to clarify: > > > > > In firefox only characterSet is supported and it returns an > > > "utf-8".It does not implement charset and defaultCharset. > > > > > > In IE all characterSet, charset and defaultCharset are > > > supported. > > > > What is the behavior of IE? > > > > It is great that we can remove [TreatReturnedNullStringAs=undefined], but we > > don't need to (shouldn't) do that if it breaks compatibility. > > IE11: > > document.implementation.createDocument(null, null, null).characterSet => "utf-8" > document.implementation.createDocument(null, null, null).charset => "utf-8" > document.implementation.createDocument(null, null, null).defaultCharset => > "windows-1252" > document.implementation.createDocument(null, null, null).inputEncoding => > "UTF-8" > > The results are the same for document.implementation.createHTMLDocument('').* > > I'm not sure if there are any other cases where IE might return null or > undefined for these, but I don't know how to create a document which is more > "orphaned" than these. Doesn't that mean that this CL makes Blink more conformant to Firefox and IE? (If the change aligns with Firefox, IE and the spec, I think it would be reasonable to land it and see what happens even if the usage rate is high.)
On 2015/06/16 23:00:56, haraken wrote: > On 2015/06/16 22:43:52, philipj wrote: > > On 2015/06/16 22:21:06, haraken wrote: > > > On 2015/06/16 22:16:49, philipj wrote: > > > > haraken@, jl@, do you have any thoughts on what to do with > > > > Document.defaultCharset, and for that matter the rest of this CL? Usage is > > > > pretty high so just removing it isn't so easy: > > > > https://www.chromestatus.com/metrics/feature/timeline/popularity/428 > > > > > > Just to clarify: > > > > > > > In firefox only characterSet is supported and it returns an > > > > "utf-8".It does not implement charset and defaultCharset. > > > > > > > > In IE all characterSet, charset and defaultCharset are > > > > supported. > > > > > > What is the behavior of IE? > > > > > > It is great that we can remove [TreatReturnedNullStringAs=undefined], but we > > > don't need to (shouldn't) do that if it breaks compatibility. > > > > IE11: > > > > document.implementation.createDocument(null, null, null).characterSet => > "utf-8" > > document.implementation.createDocument(null, null, null).charset => "utf-8" > > document.implementation.createDocument(null, null, null).defaultCharset => > > "windows-1252" > > document.implementation.createDocument(null, null, null).inputEncoding => > > "UTF-8" > > > > The results are the same for document.implementation.createHTMLDocument('').* > > > > I'm not sure if there are any other cases where IE might return null or > > undefined for these, but I don't know how to create a document which is more > > "orphaned" than these. > > Doesn't that mean that this CL makes Blink more conformant to Firefox and IE? > (If the change aligns with Firefox, IE and the spec, I think it would be > reasonable to land it and see what happens even if the usage rate is high.) Yes, bringing us closer to Firefox and IE is indeed the intention. The bit about this CL that is troubling is defaultCharset for a newly created document where we don't have access to settings.
On 2015/06/17 07:22:20, philipj wrote: > On 2015/06/16 23:00:56, haraken wrote: > > On 2015/06/16 22:43:52, philipj wrote: > > > On 2015/06/16 22:21:06, haraken wrote: > > > > On 2015/06/16 22:16:49, philipj wrote: > > > > > haraken@, jl@, do you have any thoughts on what to do with > > > > > Document.defaultCharset, and for that matter the rest of this CL? Usage > is > > > > > pretty high so just removing it isn't so easy: > > > > > https://www.chromestatus.com/metrics/feature/timeline/popularity/428 > > > > > > > > Just to clarify: > > > > > > > > > In firefox only characterSet is supported and it returns an > > > > > "utf-8".It does not implement charset and defaultCharset. > > > > > > > > > > In IE all characterSet, charset and defaultCharset are > > > > > supported. > > > > > > > > What is the behavior of IE? > > > > > > > > It is great that we can remove [TreatReturnedNullStringAs=undefined], but > we > > > > don't need to (shouldn't) do that if it breaks compatibility. > > > > > > IE11: > > > > > > document.implementation.createDocument(null, null, null).characterSet => > > "utf-8" > > > document.implementation.createDocument(null, null, null).charset => "utf-8" > > > document.implementation.createDocument(null, null, null).defaultCharset => > > > "windows-1252" > > > document.implementation.createDocument(null, null, null).inputEncoding => > > > "UTF-8" > > > > > > The results are the same for > document.implementation.createHTMLDocument('').* > > > > > > I'm not sure if there are any other cases where IE might return null or > > > undefined for these, but I don't know how to create a document which is more > > > "orphaned" than these. > > > > Doesn't that mean that this CL makes Blink more conformant to Firefox and IE? > > (If the change aligns with Firefox, IE and the spec, I think it would be > > reasonable to land it and see what happens even if the usage rate is high.) > > Yes, bringing us closer to Firefox and IE is indeed the intention. The bit about > this CL that is troubling is defaultCharset for a newly created document where > we don't have access to settings. Thanks, now I understand the point. You're an expert of these compatibility issues, so I'll defer the decision to you. There is no strong reason we must remove [TreatReturnedNullStringAs] from the IDL compiler, so I'm fine with the either way from the perspective of the IDL compiler. The compatibility should be the first.
Habib, my advice is to back out the defaultCharset change from this CL. The long-term status of defaultCharset is uncertain so let's leave it alone until we have an idea. I'm checking in httparchive data how it's actually used, to determine if we should try to standardize it or remove it.
Thanks, I was looking to find a way of addressing a "default encoding for the creating context". I was looking into setting defaultCharset by default as UTF-8, as per specification and if settings is initialized it sets the value based on defaultTextEncodingName. I will now stop this current activity and will send without defaultCharset.
On 2015/06/17 11:10:38, Habib Virji wrote: > Thanks, I was looking to find a way of addressing a "default encoding for the > creating context". > > I was looking into setting defaultCharset by default as UTF-8, as per > specification and if settings > is initialized it sets the value based on defaultTextEncodingName. > > I will now stop this current activity and will send without defaultCharset. If you have ideas for how to fix this then let's continue discussing in a follow-up CL. In principle there's nothing difficult going on here, it's just tricky because the document isn't attached to a frame at this point.
Please also update the title and description of this CL to be more accurate about what is being done, since this now involves more than just the charset change.
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180793002/100001
Thanks philip, have uploaded patch without defaultCharset.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
philipj@opera.com changed reviewers: + pdr@chromium.org
OK, this now LGTM, but since this does poke at some very central code I'd like a second opinion. pdr@, do you think this seems sounds?
pdr@chromium.org changed reviewers: + dominicc@chromium.org
On 2015/06/17 at 20:50:14, philipj wrote: > OK, this now LGTM, but since this does poke at some very central code I'd like a second opinion. pdr@, do you think this seems sounds? I also think this makes sense, but I'm not familiar with how this is used in the wild. +cc dominicc/dom team, LGTY?
On 2015/06/17 at 20:56:36, pdr wrote: > On 2015/06/17 at 20:50:14, philipj wrote: > > OK, this now LGTM, but since this does poke at some very central code I'd like a second opinion. pdr@, do you think this seems sounds? > > I also think this makes sense, but I'm not familiar with how this is used in the wild. > > +cc dominicc/dom team, LGTY? If I'm reading the specs right, we have the case of "utf-8" wrong. I think it would be good to file a bug for that and add failure expectations with the lowercase name while you're here; some of the assertions these tests add look wrong to me. Comments inline.
Oops, and here are the comments. https://codereview.chromium.org/1180793002/diff/100001/LayoutTests/dom/xhtml/... File LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js (right): https://codereview.chromium.org/1180793002/diff/100001/LayoutTests/dom/xhtml/... LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js:107: assertEquals("documentgetinputencoding02", "UTF-8".toLowerCase(), encodingName.toLowerCase()); The spec your change mentions links to this to explain what the name of UTF-8 is: https://encoding.spec.whatwg.org/#names-and-labels And what you're asserting here accepts invalid names and doesn't accept all of the valid ones (you're missing "unicode-1-1-utf-8".) Write an accurate assertion. Unless there's a spec I'm missing... link? https://codereview.chromium.org/1180793002/diff/100001/LayoutTests/fast/dom/d... File LayoutTests/fast/dom/document-attribute-js-null.html (right): https://codereview.chromium.org/1180793002/diff/100001/LayoutTests/fast/dom/d... LayoutTests/fast/dom/document-attribute-js-null.html:69: {name: 'charset', expectedNull: 'UTF-8'}, I think the case of this is wrong per https://encoding.spec.whatwg.org/#names-and-labels
https://codereview.chromium.org/1180793002/diff/100001/LayoutTests/dom/xhtml/... File LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js (right): https://codereview.chromium.org/1180793002/diff/100001/LayoutTests/dom/xhtml/... LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js:107: assertEquals("documentgetinputencoding02", "UTF-8".toLowerCase(), encodingName.toLowerCase()); The spec link I mentioned was to emphasise it does not return null but rather utf-8. Both Gecko and Blink set the value as UTF-8 and not utf-8, utf8 or unicode-1-1-utf-8. I have updated the test to reflect this. https://codereview.chromium.org/1180793002/diff/100001/LayoutTests/fast/dom/d... File LayoutTests/fast/dom/document-attribute-js-null.html (right): https://codereview.chromium.org/1180793002/diff/100001/LayoutTests/fast/dom/d... LayoutTests/fast/dom/document-attribute-js-null.html:69: {name: 'charset', expectedNull: 'UTF-8'}, On 2015/06/22 01:23:31, dominicc wrote: > I think the case of this is wrong per > https://encoding.spec.whatwg.org/#names-and-labels Yes it does not match it as utf-8, but these test was to show it does not return nullvalue. I can work on following CL to make UTF8Encoding() handle possible different values as per the specification.
https://codereview.chromium.org/1180793002/diff/100001/LayoutTests/dom/xhtml/... File LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js (right): https://codereview.chromium.org/1180793002/diff/100001/LayoutTests/dom/xhtml/... LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js:107: assertEquals("documentgetinputencoding02", "UTF-8".toLowerCase(), encodingName.toLowerCase()); On 2015/06/22 14:07:30, Habib Virji wrote: > The spec link I mentioned was to emphasise it does not return null but rather > utf-8. Both Gecko and Blink set the value as UTF-8 and not utf-8, utf8 or > unicode-1-1-utf-8. I have updated the test to reflect this. "utf8" and "unicode-1-1-utf-8" don't matter here, those are labels that map to the internal enum or atomic string representing UTF-8, while this should be testing how that enum or atomic string is represented when converted back to a string. https://codereview.chromium.org/1180793002/diff/100001/LayoutTests/fast/dom/d... File LayoutTests/fast/dom/document-attribute-js-null.html (right): https://codereview.chromium.org/1180793002/diff/100001/LayoutTests/fast/dom/d... LayoutTests/fast/dom/document-attribute-js-null.html:69: {name: 'charset', expectedNull: 'UTF-8'}, On 2015/06/22 01:23:31, dominicc wrote: > I think the case of this is wrong per > https://encoding.spec.whatwg.org/#names-and-labels Actually https://dom.spec.whatwg.org/#dom-document-characterset special-cases a bunch of encoding to use uppercase labels instead. I'm pretty sure Anne would be happy to always return a lowercase value if there's implementor interest to make that change. Not in this CL, though. https://codereview.chromium.org/1180793002/diff/120001/LayoutTests/dom/xhtml/... File LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js (right): https://codereview.chromium.org/1180793002/diff/120001/LayoutTests/dom/xhtml/... LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js:107: assertEquals("documentgetinputencoding02", "utf-8", encodingName); I think that this should simply be |assertEquals("documentgetinputencoding02", "UTF-8", encodingName)| as the single assertion.
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com Link to the patchset: https://codereview.chromium.org/1180793002/#ps140001 (title: "Updated as per philipj suggestions")
https://codereview.chromium.org/1180793002/diff/120001/LayoutTests/dom/xhtml/... File LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js (right): https://codereview.chromium.org/1180793002/diff/120001/LayoutTests/dom/xhtml/... LayoutTests/dom/xhtml/level3/core/documentgetinputencoding02.js:107: assertEquals("documentgetinputencoding02", "utf-8", encodingName); On 2015/06/23 08:32:57, philipj (away until June 23) wrote: > I think that this should simply be |assertEquals("documentgetinputencoding02", > "UTF-8", encodingName)| as the single assertion. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180793002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60311)
@philipj/dominicc: The failure in mac and win are unrelated to my changes. Are changes good to go?
LGTM for me, but I'll give dominicc@ the final say.
On 2015/06/26 13:05:23, philipj wrote: > LGTM for me, but I'll give dominicc@ the final say. @dominicc: Are changes okay?
On 2015/07/08 at 08:51:02, habib.virji wrote: > On 2015/06/26 13:05:23, philipj wrote: > > LGTM for me, but I'll give dominicc@ the final say. > > @dominicc: Are changes okay? Sorry for the slow reply--I was on vacation. Back now. Returning UTF-8 sounds fine if it's what Gecko does. What do other browsers do? Maybe we should update the HTML spec.
The CQ bit was checked by dominicc@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180793002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198563 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
