Closed Bug 1118741 Opened 6 years ago Closed 5 years ago

Implement Document.inputEncoding per spec.

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 647621

People

(Reporter: Ms2ger, Unassigned, Mentored)

Details

(Keywords: dev-doc-needed, Whiteboard: [lang=c++])

Attachments

(1 file)

GetInputEncoding should just call GetCharacterSet directly.

mHaveInputEncoding should be removed from nsIDocument and the InputEncoding warning should be removed from nsDeprecatedOperationList.h and dom/locales/en-US/chrome/dom/dom.properties.

This should fix failures in at least the following web-platform-tests:

* dom/interfaces.html
* dom/nodes/Document-constructor.html
* dom/nodes/DOMImplementation-createDocument.html
* dom/nodes/DOMImplementation-createHTMLDocument.html
* dom/nodes/Node-properties.html
* html/dom/interfaces.html

(To adjust the expectations for those tests, remove the entries for the passing tests from the corresponding .ini file at under testing/web-platform/meta/.)
Keywords: dev-doc-needed
I'am interested on working on this bug, can you provide me with some pointers ?
Sure. The implementation of Document.inputEncoding is in the nsIDocument::GetInputEncoding function here: <https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#7468>.

Right now, it calls GetCharacterSet if mHaveInputEncoding is true, while it should just call that function unconditionally.

Also, we log a warning to the developer console whenever you first access the inputEncoding attribute; this is what the WarnOnceAbout call is for. Since this attribute has been added to the specification, there's no longer a need to warn about its usage, so we can remove that call and the definition of the warning.

Good luck!
> GetInputEncoding should just call GetCharacterSet directly.

Is there consensus on this spec?  What do other implementations do?

I'd rather not change behavior here if we have to change it back later.
Flags: needinfo?(Ms2ger)
Per the discussion in https://www.w3.org/Bugs/Public/show_bug.cgi?id=27435 it seems WebKit/Blink alias. IE reportedly has casing differences.
Flags: needinfo?(Ms2ger)
OK.  Do we have any indication from the IE team what their plans are here?
Per Travis they might be interested in aligning with the specification if the casing differences won't cause compatibility problems. We don't have the casing differences to begin with...
Can i work on this bug??
There are 2 definitions for WarnOnceAbout() method. Which one do we have to remove here?
Just the call in GetInputEncoding should be removed.
should i make getInputEncoding a const function?
That's probably a good idea, yes.
Ok I'll get on it right awa . Thanks .
should the SetDOMStringToNull(aInputEncoding); in <https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#7487> be moved above the return line? Otherwise it will be ignored.
Attachment #8554249 - Flags: review?(Ms2ger)
Comment on attachment 8554249 [details] [diff] [review]
bug-1118741-fix.patch

Review of attachment 8554249 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good; please make sure it builds and passes the tests I mentioned in comment #0.

::: dom/base/nsDocument.cpp
@@ +7454,5 @@
>    return NS_OK;
>  }
>  
>  void
> +nsIDocument::GetInputEncoding(nsAString& aInputEncoding) const

Did this build without a change to the declaration?

@@ -7458,5 @@
> -nsIDocument::GetInputEncoding(nsAString& aInputEncoding)
> -{
> -  // Not const function, because WarnOnceAbout is not a const method
> -  WarnOnceAbout(eInputEncoding);
> -  if (mHaveInputEncoding) {

You should also remove the other references to mHaveInputEncoding
Attachment #8554249 - Flags: review?(Ms2ger) → feedback+
Assignee: nobody → anirudh.gp
Is this bug still open then I would like to work on this ?
Hi Jinank,

It looks like Anirudh hasn't had time to address my comments in comment #16. It would be great if you could take the patch (attachment 8554249 [details] [diff] [review]) and address those comments. Let me know if there's anything I can help with.
Assignee: anirudh.gp → nobody
It seems this is a dupe, as inputEncoding was already fixed in bug 647621.
Thanks!
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 647621
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.