Closed
Bug 1347818
Opened 7 years ago
Closed 7 years ago
Clean up documentCharacterSet not to use nsIDOM*
Categories
(Core :: DOM: Editor, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla55
| Tracking | Status | |
|---|---|---|
| firefox55 | --- | fixed |
People
(Reporter: m_kato, Assigned: m_kato)
Details
Attachments
(2 files)
there is no documentCharacterSet test. We should add it.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 3•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8847942 [details] Bug 1347818 - Part 1. Clean up documentCharacterSet not to use nsIDOM*. https://reviewboard.mozilla.org/r/120880/#review122836 ::: commit-message-ff04d:5 (Diff revision 1) > +Bug 1347818 - Part 1. Clean up documentCharacterSet not to use nsIDOM*. r?masayuki > + > +I want to remove nsIDOMNodeList usages from editor excepting old debug code. > + > +(BTW, we might have to change to <meta charset> instead of <meta http-equive>, but it should handle by another issue) Do you mean "it should be handled by..."? ::: editor/libeditor/TextEditor.cpp:249 (Diff revision 1) > - nsCOMPtr<nsIDOMNode> headNode; > - headList->Item(0, getter_AddRefs(headNode)); > - NS_ENSURE_TRUE(headNode, NS_OK); > + nsCOMPtr<nsIContent> headNode = headList->Item(0); > + if (NS_WARN_IF(!headNode)) { > + return NS_OK; > + } Hmm, not scope of this bug, though, I don't understand this. If the document doesn't have <head> element, e.g., "data:text/html,<div contenteditable></div>", this gives up to add <meta>? Or is there always a <head> element node even in such document? ::: editor/libeditor/TextEditor.cpp:282 (Diff revision 1) > - getter_AddRefs(list)); > - NS_ENSURE_SUCCESS(rv, false); > - NS_ENSURE_TRUE(list, false); > + if (!metaList) { > + return false; > + } Even if there is no element, GetElementsByTagName() returns non-null, so, I think that you should keep using NS_WARN_IF here. ::: editor/libeditor/TextEditor.cpp:286 (Diff revision 1) > - uint32_t listLength = 0; > + uint32_t listLength = metaList->Length(true); > - metaList->GetLength(&listLength); > > for (uint32_t i = 0; i < listLength; ++i) { nit: you can remove lineLength if you use it in the for's condition.
Attachment #8847942 -
Flags: review?(masayuki) → review+
|
|
||
Comment 4•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8847943 [details] Bug 1347818 - Part 2. Add documentCharacterSet test. https://reviewboard.mozilla.org/r/120882/#review122858 ::: editor/libeditor/tests/test_documentCharacterSet.html:31 (Diff revision 1) > + let editdoc = window.frames[0].document; > + editdoc.designMode = 'on'; > + let editor = getEditor(); > + > + editor.documentCharacterSet = "us-ascii"; > + let meta = editdoc.getElementsByTagName("meta")[0]; > + is(meta.getAttribute("http-equiv"), "Content-Type", > + "meta element should have http-equiv"); > + is(meta.getAttribute("content"), "text/html;charset=us-ascii", > + "charset should be set as us-ascii"); > + > + editor.documentCharacterSet = "utf-8"; > + meta = editdoc.getElementsByTagName("meta")[0]; > + is(meta.getAttribute("http-equiv"), "Content-Type", > + "meta element should have http-equiv"); > + is(meta.getAttribute("content"), "text/html;charset=utf-8", > + "charset should be set as utf-8"); Nice! But I'd like you to check <meta http-equiv="Content-Type"> is 2nd <meta> element case. (I think that insert another <meta> element at first of the <head> and run a test is enough.)
Attachment #8847943 -
Flags: review?(masayuki) → review+
Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/mozilla-inbound/rev/2e2cc094e6a6 Part 1. Clean up documentCharacterSet not to use nsIDOM*. r=masayuki https://hg.mozilla.org/integration/mozilla-inbound/rev/fa4734723b87 Part 2. Add documentCharacterSet test. r=masayuki
|
||
Comment 6•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/2e2cc094e6a6 https://hg.mozilla.org/mozilla-central/rev/fa4734723b87
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•