Closed Bug 181137 Opened 20 years ago Closed 3 years ago

DeCOMtaminate nsIContentIterator

Categories

(Core :: DOM: Core & HTML, enhancement, P5)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: kinmoz, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

Attachments

(9 files)

Bug 181137 - part 1: Move nsContentIterator and nsContentSubtreeIterator into mozilla namespace
47 bytes, text/x-phabricator-request
Details | Review
Bug 181137 - part 2: Make nsContentIterator class is a base class of other concrete classes
47 bytes, text/x-phabricator-request
Details | Review
Bug 181137 - part 3: Make all users of ContentSutreeIterator treat it directly rather than via nsIContentIterator
47 bytes, text/x-phabricator-request
Details | Review
Bug 181137 - part 4: Make all users of PreContentIterator treat it directly rather than via nsIContentIterator
47 bytes, text/x-phabricator-request
Details | Review
Bug 181137 - part 5: Make all users of PostContentIterator treat it directly rather than via nsIContentIterator
47 bytes, text/x-phabricator-request
Details | Review
Bug 181137 - part 6: Make nsFilteredContentIterator not derived from nsIContentIterator
47 bytes, text/x-phabricator-request
Details | Review
Bug 181137 - part 7: Get rid of nsIContentIterator interface
47 bytes, text/x-phabricator-request
Details | Review
Bug 181137 - part 8: Make ContentIteratorBase and its subclasses non-refcountable
47 bytes, text/x-phabricator-request
Details | Review
Bug 181137 - part 9: Move kNSURIs in Document.cpp and FragmentOrElement.cpp into their users
47 bytes, text/x-phabricator-request
Details | Review 
This bug was split off from bug 176251, and is supposed to address the following
cleanup issues with the content iterator:

1. Remove the iterator CIDs from nsLayoutCID.h since they are already
   defined in nsContentCID.h.

2. Define ContractID strings for each iterator type.

3. Convert *all* CreateInstance() calls that currently use CIDs, to use
   do_CreateInstance(ContractID) so that we can get rid of the macros used
   to create useable CIDs in each file that creates an iterator. This
   also means we may be able to remove the #includes that suck in nsLayoutCID.h
   and nsContentCID.h if they were only needed to define the iterator CIDs.

4. Rename NS_CONTENTITERATOR_CID to NS_POSTCONTENTITERATOR_CID so that
   it reflects what iterator traversal type it actually creates.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.3alpha
Target Milestone: mozilla1.3alpha → mozilla1.3beta
Target Milestone: mozilla1.3beta → mozilla1.5alpha
Target Milestone: mozilla1.5alpha → mozilla1.5beta
QA Contact: nobody → traversal-range
Component: DOM: Traversal-Range → DOM: Core & HTML
Bulk priority change, per :mdaly
Priority: P3 → P5
Currently, nsIContentIterator instances can be in stack in most cases. However, we still need to create them into heap. Additionally, each user calls every method via nsIContentIterator. I.e., each call is a virtual call and the number of calls may be too many. Therefore, we should expose each concrete classes under "mozilla" and we shouldn't use them as XPCOM object as far as possible.
Assignee: kinmoz → nobody
Blocks: 1365874, 1367744
Status: ASSIGNED → NEW
Depends on: 1325850
Target Milestone: mozilla1.5beta → ---
Looks like that we can get rid of nsIContentIterator interface completely. Taking.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Summary: ContentIterator Code needs cleanup. → DeCOMtaminate nsIContentIterator
First, we should move nsContentIterator and nsContentSubtreeIterator into
mozilla namespace and then, remove "ns" prefix.

Additionally, this patch separates the definition of the classes into
ContentIterator.h and exposes it as "mozilla/ContentIterator.h".  This allows
everybody access those concrete classes.
Currently, ContentIterator is created with a bool flag to decide whether the
instance lists up post-order or pre-order.  However, this is not clear.  For
example:

nsCOMPtr<nsIContentIterator> preOrderIter = new ContentIterator(false);

This is not clear whether this does right thing or not.

This patch makes any users can create PostContentIterator for post-order
iterator, and creates PreContentIterator for pre-order iterator.  So, now,
each creator needs to writhe above as:

nsCOMPtr<nsIContentIterator> preOrderIter = new PreContentIterator();

or

nsCOMPtr<nsIContentIterator> postOrderIter = new PostContentIterator();

Additionally, with this change, if each user starts to use concrete classes
directly, compiler can stop using virtual calls because of all concrete
classes are now marked as "final".
Now, all users of ContentSubtreeIterator can access it directly.  This patch
makes them use the concrete class directly.
Now, all users of PreContentIterator can access it directly.  This patch makes
them use the concrete class directly.
Now, all users of PostContentIterator can access it directly.  This patch
makes them use the concrete class directly.
nsFilteredContentIterator is used only by TextServicesDocument and there is
no reason that it should be derived from nsIContentIterator except consistency.
Additionally, it's now only class which is derived from nsIContentIterator
except ContentIteratorBase.  So, after this change, we can get rid of
nsIContentIterator completely.

This patch moves nsFilteredContentIterator into mozilla namespace and
makes TextServicesDocument treat FilteredContentIterator directly instead of
nsIContentIterator interface.
Now, nobody requires nsIContentIterator interface.  So, we can get rid of it.

Unfortunately, there is no macro to keep the inherited class,
ContentSubtreeIterator, in the cycle collection to make it keep managing
ContentSubtreeIterator::mRange without nsISupports interface.  Therefore, this
patch moves it into ContentIteratorBase temporarily.  Anyway, the following
patch makes those classes not refcountable.  At that time, this issue will be
fixed.
This patch makes ContentIteratorBase, PostContentIterator, PreContentIterator
and ContentSubtreeIterator classes non-refcountable because most users can
create their instances in stack and such users may be in a hot path.  So,
we can save a lot of cost of instantiation.

Unfortunately, only ScriptableContentIterator creates one of the concrete
classes and needs to destroy it properly.  Therefore, its
EnsureContentIterator(), destructor, traverse and unlink code becomes messy.
However, ScriptableContentIterator was designed for automated tests and we
need to maintain it not so many times.  Therefore, improvement of other
users must be worthwhiler than this demerit.

Probably, starting to review from part 8 must be easier to understand what I want to do this here.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/90a1ff49b6b1
part 1: Move nsContentIterator and nsContentSubtreeIterator into mozilla namespace r=smaug
https://hg.mozilla.org/integration/autoland/rev/654fdbad67db
part 2: Make nsContentIterator class is a base class of other concrete classes r=smaug
https://hg.mozilla.org/integration/autoland/rev/b6fc7a332db7
part 3: Make all users of ContentSutreeIterator treat it directly rather than via nsIContentIterator r=smaug
https://hg.mozilla.org/integration/autoland/rev/b7ab59bf545e
part 4: Make all users of PreContentIterator treat it directly rather than via nsIContentIterator r=smaug
https://hg.mozilla.org/integration/autoland/rev/83bec02c21d9
part 5: Make all users of PostContentIterator treat it directly rather than via nsIContentIterator r=smaug
https://hg.mozilla.org/integration/autoland/rev/5385d5fd9b8b
part 6: Make nsFilteredContentIterator not derived from nsIContentIterator r=smaug
https://hg.mozilla.org/integration/autoland/rev/65a4b245e851
part 7: Get rid of nsIContentIterator interface r=smaug
https://hg.mozilla.org/integration/autoland/rev/99a977d519a0
part 8: Make ContentIteratorBase and its subclasses non-refcountable r=smaug

Backed out for bustage on FragmentOrElement.cpp:1751

backout: https://hg.mozilla.org/integration/autoland/rev/5562d6967f3d6a7d4a5f9a16e7a492452163ff14

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=99a977d519a0b78e267d3dce4afb009b8d3be769

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=221045111&repo=autoland&lineNumber=24572

[task 2019-01-10T09:27:03.632Z] 09:27:03 INFO - /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/gcc/bin/g++ -o Unified_cpp_dom_base2.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DHAVE_SIDEBAR -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/obj-firefox/dom/base -I/builds/worker/workspace/build/src/dom/battery -I/builds/worker/workspace/build/src/dom/events -I/builds/worker/workspace/build/src/dom/media -I/builds/worker/workspace/build/src/dom/network -I/builds/worker/workspace/build/src/caps -I/builds/worker/workspace/build/src/docshell/base -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/dom/file -I/builds/worker/workspace/build/src/dom/geolocation -I/builds/worker/workspace/build/src/dom/html -I/builds/worker/workspace/build/src/dom/ipc -I/builds/worker/workspace/build/src/dom/storage -I/builds/worker/workspace/build/src/dom/svg -I/builds/worker/workspace/build/src/dom/u2f -I/builds/worker/workspace/build/src/dom/xbl -I/builds/worker/workspace/build/src/dom/xml -I/builds/worker/workspace/build/src/dom/xslt/xpath -I/builds/worker/workspace/build/src/dom/xul -I/builds/worker/workspace/build/src/gfx/2d -I/builds/worker/workspace/build/src/image -I/builds/worker/workspace/build/src/js/xpconnect/loader -I/builds/worker/workspace/build/src/js/xpconnect/src -I/builds/worker/workspace/build/src/js/xpconnect/wrappers -I/builds/worker/workspace/build/src/layout/base -I/builds/worker/workspace/build/src/layout/forms -I/builds/worker/workspace/build/src/layout/generic -I/builds/worker/workspace/build/src/layout/style -I/builds/worker/workspace/build/src/layout/svg -I/builds/worker/workspace/build/src/layout/xul -I/builds/worker/workspace/build/src/netwerk/base -I/builds/worker/workspace/build/src/netwerk/url-classifier -I/builds/worker/workspace/build/src/security/manager/ssl -I/builds/worker/workspace/build/src/widget -I/builds/worker/workspace/build/src/xpcom/ds -I/builds/worker/workspace/build/src/netwerk/sctp/datachannel -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wformat -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -freorder-blocks -Os -fno-omit-frame-pointer -funwind-tables -Werror -I/builds/worker/workspace/build/src/widget/gtk/compat-gtk3 -pthread -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/freetype2 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/libpng12 -I/usr/include/gtk-3.0/unix-print -Wno-error=shadow -MD -MP -MF .deps/Unified_cpp_dom_base2.o.pp /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base2.cpp
[task 2019-01-10T09:27:03.633Z] 09:27:03 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base2.cpp:65:0:
[task 2019-01-10T09:27:03.636Z] 09:27:03 ERROR - /builds/worker/workspace/build/src/dom/base/FragmentOrElement.cpp:1751:20: error: 'kNSURIs' defined but not used [-Werror=unused-variable]
[task 2019-01-10T09:27:03.637Z] 09:27:03 INFO - static const char* kNSURIs[] = {" ([none])", " (xmlns)", " (xml)",
[task 2019-01-10T09:27:03.637Z] 09:27:03 INFO - ^~~~~~~
[task 2019-01-10T09:27:03.637Z] 09:27:03 INFO - In file included from /builds/worker/workspace/build/src/dom/base/Document.cpp:36:0,
[task 2019-01-10T09:27:03.637Z] 09:27:03 INFO - from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base2.cpp:2:
[task 2019-01-10T09:27:03.637Z] 09:27:03 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Telemetry.h: In member function 'void mozilla::dom::Document::ReportUseCounters(mozilla::dom::Document::UseCounterReportKind)':
[task 2019-01-10T09:27:03.638Z] 09:27:03 WARNING - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Telemetry.h:111:3: warning: 'label' may be used uninitialized in this function [-Wmaybe-uninitialized]
[task 2019-01-10T09:27:03.638Z] 09:27:03 INFO - Accumulate(static_cast<HistogramID>(CategoricalLabelId<E>::value),
[task 2019-01-10T09:27:03.638Z] 09:27:03 INFO - ^~~~~~~~~~
[task 2019-01-10T09:27:03.638Z] 09:27:03 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base2.cpp:2:0:
[task 2019-01-10T09:27:03.638Z] 09:27:03 INFO - /builds/worker/workspace/build/src/dom/base/Document.cpp:11337:42: note: 'label' was declared here
[task 2019-01-10T09:27:03.638Z] 09:27:03 INFO - LABELS_HIDDEN_VIEWPORT_OVERFLOW_TYPE label;
[task 2019-01-10T09:27:03.638Z] 09:27:03 INFO - ^~~~~
[task 2019-01-10T09:27:03.638Z] 09:27:03 INFO - cc1plus: all warnings being treated as errors
[task 2019-01-10T09:27:03.640Z] 09:27:03 INFO - /builds/worker/workspace/build/src/config/rules.mk:1119: recipe for target 'Unified_cpp_dom_base2.o' failed
[task 2019-01-10T09:27:03.641Z] 09:27:03 ERROR - make[4]: *** [Unified_cpp_dom_base2.o] Error 1
[task 2019-01-10T09:27:03.642Z] 09:27:03 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/base'
[task 2019-01-10T09:27:03.643Z] 09:27:03 INFO - make[4]: *** Waiting for unfinished jobs....
[task 2019-01-10T09:27:03.644Z] 09:27:03 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/dom/html'

Flags: needinfo?(masayuki)

Sigh. There is a hidden bug similar to bug 1518384...

Flags: needinfo?(masayuki)
Due to renaming nsContentIterator.cpp to ContentIterator.cpp, Document.cpp
and FragmentOrElement.cpp are compiled in a unified cpp file now.  However,
both of them have same name constant, kNSURIs and some build systems claims
that it in FragmentOrElement.cpp is never used.

Fortunately, each of them is used only by one method.  Therefore, this patch
moves the each declaration into each user method.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/2eaf630d9925
part 1: Move nsContentIterator and nsContentSubtreeIterator into mozilla namespace r=smaug
https://hg.mozilla.org/integration/autoland/rev/a29d33c6fca1
part 2: Make nsContentIterator class is a base class of other concrete classes r=smaug
https://hg.mozilla.org/integration/autoland/rev/5fb4b3194d8e
part 3: Make all users of ContentSutreeIterator treat it directly rather than via nsIContentIterator r=smaug
https://hg.mozilla.org/integration/autoland/rev/7b087e0123bd
part 4: Make all users of PreContentIterator treat it directly rather than via nsIContentIterator r=smaug
https://hg.mozilla.org/integration/autoland/rev/0d3c97c78d7c
part 5: Make all users of PostContentIterator treat it directly rather than via nsIContentIterator r=smaug
https://hg.mozilla.org/integration/autoland/rev/61f2c3b1b368
part 6: Make nsFilteredContentIterator not derived from nsIContentIterator r=smaug
https://hg.mozilla.org/integration/autoland/rev/2a320da37ca0
part 7: Get rid of nsIContentIterator interface r=smaug
https://hg.mozilla.org/integration/autoland/rev/dbf919528d97
part 8: Make ContentIteratorBase and its subclasses non-refcountable r=smaug
https://hg.mozilla.org/integration/autoland/rev/897a97a4931d
part 9: Move kNSURIs in Document.cpp and FragmentOrElement.cpp into their users r=smaug

Wow, this improves the performance of bug 1346723 than I've expected.

Sounds great!

Memory allocation and deallocation tends to be always rather slow.

Type: defect → enhancement
You need to log in before you can comment on or make changes to this bug.