1118741 - Implement Document.inputEncoding per spec.

Status: RESOLVED DUPLICATE of bug 647621

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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/.)

Comment 1

[guddu.guha80]

I'am interested on working on this bug, can you provide me with some pointers ?

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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!

Comment 3

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 4

[Anne (:annevk)]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK.  Do we have any indication from the IE team what their plans are here?

Comment 6

[Anne (:annevk)]

No, I asked on Twitter: https://twitter.com/annevk/status/554662116331110400

Comment 7

[Anne (:annevk)]

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...

Comment 8

[Anirudh GP(:anirudhgp)]

Can i work on this bug??

Comment 9

[Anirudh GP(:anirudhgp)]

There are 2 definitions for WarnOnceAbout() method. Which one do we have to remove here?

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Just the call in GetInputEncoding should be removed.

Comment 11

[Anirudh GP(:anirudhgp)]

should i make getInputEncoding a const function?

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

That's probably a good idea, yes.

Comment 13

[Anirudh GP(:anirudhgp)]

Ok I'll get on it right awa . Thanks .

Comment 14

[Anirudh GP(:anirudhgp)]

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.

Comment 15

[Anirudh GP(:anirudhgp)]

Attachment: bug-1118741-fix.patch

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 17

[Jinank Jain[:jj]]

Is this bug still open then I would like to work on this ?

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 19

[Thomas Wisniewski]

It seems this is a dupe, as inputEncoding was already fixed in bug 647621.

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks!