Last Comment Bug 672190 - consider removing expandEntityReferences from NodeIterator and TreeWalker
: consider removing expandEntityReferences from NodeIterator and TreeWalker
Status: RESOLVED FIXED
[good first bug][mentor=Ms2ger][lang=...
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Gaurav Pruthi
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-18 07:06 PDT by Anne (:annevk)
Modified: 2013-08-11 23:48 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Removed the field "expandEntityReferences" from nsIDOMTreeWalker.idl and nsIDOMNodeIterator.idl (5.18 KB, patch)
2013-01-08 01:00 PST, Gaurav Pruthi
no flags 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 (6.58 KB, patch)
2013-01-08 04:41 PST, Gaurav Pruthi
Ms2ger: feedback+
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. (7.35 KB, patch)
2013-01-09 01:39 PST, Gaurav Pruthi
bugs: review+
Ms2ger: feedback+
Details | Diff | Review
Uploading the new patch with unix endings using MQ. (3.12 KB, patch)
2013-01-18 10:21 PST, Gaurav Pruthi
no flags Details | Diff | Review
Uploading the new patch with updated 'uuid' in IDL files. (3.59 KB, patch)
2013-01-20 04:29 PST, Gaurav Pruthi
Ms2ger: review+
Details | Diff | Review

Description Anne (:annevk) 2011-07-18 07:06:39 PDT
This attribute has never made much sense.
Comment 1 Matthew Schranz [:mjschranz] 2012-02-16 20:24:15 PST
This is done in Bug 698384.
Comment 2 :Ms2ger 2012-12-21 04:30:01 PST
The actual attribute wasn't removed in that bug.
Comment 3 Anne (:annevk) 2012-12-21 04:32:49 PST
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 2013-01-02 04:59:05 PST
Hi, Can you please assign this bug to me.
Comment 5 Gaurav Pruthi 2013-01-08 01:00:01 PST
Created attachment 699038 [details] [diff] [review]
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 2013-01-08 01:13:23 PST
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 2013-01-08 04:41:12 PST
Created 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

Uploading the new patch...pls check again.
Comment 8 :Ms2ger 2013-01-08 06:10:20 PST
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 2013-01-09 01:39:49 PST
Created 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.

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 2013-01-11 00:56:58 PST
Gaurav, the patch looks good, except for the line ending issue. Do you need help fixing that?
Comment 11 Gaurav Pruthi 2013-01-11 01:17:35 PST
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 2013-01-17 09:31:42 PST
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] (high review load, please consider other reviewers) 2013-01-18 02:31:06 PST
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 2013-01-18 10:21:57 PST
Created attachment 703967 [details] [diff] [review]
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 2013-01-18 10:36:14 PST
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 2013-01-20 04:29:09 PST
Created attachment 704318 [details] [diff] [review]
Uploading the new patch with updated 'uuid' in IDL files.
Comment 17 :Ms2ger 2013-01-20 04:34:05 PST
Comment on attachment 704318 [details] [diff] [review]
Uploading the new patch with updated 'uuid' in IDL files.

Thanks!
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-01-20 05:13:12 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/56efd49e70dd
Comment 19 Phil Ringnalda (:philor) 2013-01-20 12:48:31 PST
https://hg.mozilla.org/mozilla-central/rev/56efd49e70dd
Comment 20 Kohei Yoshino [:kohei] 2013-02-23 23:31:03 PST
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

Note You need to log in before you can comment on or make changes to this bug.