Clean up documentCharacterSet not to use nsIDOM*

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

Trunk
mozilla55
Unspecified
All
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

a year ago
there is no documentCharacterSet test.  We should add it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

a year 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

a year 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+

Comment 5

a year ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2e2cc094e6a6
https://hg.mozilla.org/mozilla-central/rev/fa4734723b87
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.