672190 - consider removing expandEntityReferences from NodeIterator and TreeWalker

Status: RESOLVED FIXED

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

Description

[Anne (:annevk)]

This attribute has never made much sense.

Comment 1

[Matthew Schranz [:mjschranz]]

This is done in Bug 698384.

Comment 2

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

The actual attribute wasn't removed in that bug.

Comment 3

[Anne (:annevk)]

http://mxr.mozilla.org/mozilla-central/search?string=expandEntityReferences shows it's still alive. This was about the attribute on the interfaces.

Comment 4

[Gaurav Pruthi]

Hi, Can you please assign this bug to me.

Comment 5

[Gaurav Pruthi]

Attachment: Removed the field "expandEntityReferences" from nsIDOMTreeWalker.idl and nsIDOMNodeIterator.idl

Please find attach the diff file for the changes made in files nsIDOMTreeWalker.idl and nsIDOMNodeIterator.idl.I have made the changes , tested and build the mozilla-central after pushing the changes to my local rep.
pls chk...its my first :)

Comment 6

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

Comment on attachment 699038 [details] [diff] [review]
Removed the field "expandEntityReferences" from nsIDOMTreeWalker.idl and nsIDOMNodeIterator.idl

First, looks like you changed the files you touched to use windows line endings (CR+LF); we use unix line endings (LF). IIRC, Mozillabuild includes a dos2unix script that can fix that.

Second, you'll need to remove the implementation of those attributes too; these are at <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsNodeIterator.cpp#213> and <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsTreeWalker.cpp#90>.

Could you fix those issues and request review from me?

Comment 7

[Gaurav Pruthi]

Attachment: Modified patch including the expandEntityReferences attribute removed from nsIDOMNodeIterator.idl and nsIDOMTreeWalker.idl as well as getter method removed from nsTreeWalker.cpp and nsNodeIterator.cpp

Uploading the new patch...pls check again.

Comment 8

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

Comment on attachment 699130 [details] [diff] [review]
Modified patch including the expandEntityReferences attribute removed from nsIDOMNodeIterator.idl and nsIDOMTreeWalker.idl as well as getter method removed from nsTreeWalker.cpp and nsNodeIterator.cpp

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

Looks like I forgot an implementation at <http://mxr.mozilla.org/mozilla-central/source/layout/inspector/src/inDeepTreeWalker.cpp#109>, which you'll need to remove as well; sorry about that.

It seems you still need to fix the windows line endings; looks good otherwise!

::: dom/interfaces/traversal/nsIDOMNodeIterator.idl
@@ +9,5 @@
> +interface nsIDOMNodeIterator;
> +interface nsIDOMNodeFilter;
> +
> +
> +[scriptable, uuid(5af83f50-c8d5-4824-be29-1aa9d640bacb)]

You'll need to update this uuid, for example with one from <http://mozilla.pettay.fi/cgi-bin/mozuuid.pl>.

::: dom/interfaces/traversal/nsIDOMTreeWalker.idl
@@ +9,5 @@
> +interface nsIDOMTreeWalker;
> +interface nsIDOMNodeFilter;
> +
> +
> +[scriptable, uuid(400af3ca-1dd2-11b2-a50a-887ecca2e63a)]

And this one, too

Comment 9

[Gaurav Pruthi]

Attachment: Updated diff file with removed getter method for expandEntityReferences from '/mozilla-central/source/layout/inspector/src/inDeepTreeWalker.cpp' in continuation with earlier attached patch.

Removed the implementation at /mozilla-central/source/layout/inspector/src/inDeepTreeWalker.cpp and I've inserted the uuid for each change after re-running the script mentioned @http://mozilla.pettay.fi/cgi-bin/mozuuid.pl .
pls check

Comment 10

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

Gaurav, the patch looks good, except for the line ending issue. Do you need help fixing that?

Comment 11

[Gaurav Pruthi]

I have used dos2unix tool also, in Vi ':set ff' cmd on file shows fileformat=unix, Do tell if there is any other solution.

Comment 12

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

Comment on attachment 699624 [details] [diff] [review]
Updated diff file with removed getter method for expandEntityReferences from '/mozilla-central/source/layout/inspector/src/inDeepTreeWalker.cpp' in continuation with earlier attached patch.

I'll fix the CRs on landing.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 699624 [details] [diff] [review]
Updated diff file with removed getter method for expandEntityReferences from '/mozilla-central/source/layout/inspector/src/inDeepTreeWalker.cpp' in continuation with earlier attached patch.

Would be great to have this in correct format (unix line endings and only one commit message), but if Ms2ger can fix that before landing...

This is a tiny bit regression risky since some silly web page
may use expandEntityReferences property, even though it returns always false.
But we're quite early in the cycle so we can back this out if it is causes
problems.

Comment 14

[Gaurav Pruthi]

Attachment: Uploading the new patch with unix endings using MQ.

Adding the new patch. I build mozilla on linux and have used MQ for this patch, hope line endings in this one will be corrected.

Comment 15

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

Comment on attachment 703967 [details] [diff] [review]
Uploading the new patch with unix endings using MQ.

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

Thanks! Looks like you forgot to update the uuids in this patch, though.

Comment 16

[Gaurav Pruthi]

Attachment: Uploading the new patch with updated 'uuid' in IDL files.

Comment 17

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

Comment on attachment 704318 [details] [diff] [review]
Uploading the new patch with updated 'uuid' in IDL files.

Thanks!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56efd49e70dd

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/56efd49e70dd

Comment 20

[Kohei Yoshino [:kohei]]

I've added this bug to the compatibility doc. Please correct the info if I'm wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21

Comment 21

[Jean-Yves Perrier [:teoli]]

I've also updated:
https://developer.mozilla.org/en-US/docs/Web/API/TreeWalker.expandEntityReferences
https://developer.mozilla.org/en-US/docs/Web/API/NodeIterator.expandEntityReferences
and
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/21