Closed Bug 181137 Opened 22 years ago Closed 6 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
(deleted), text/x-phabricator-request
Details
Bug 181137 - part 2: Make nsContentIterator class is a base class of other concrete classes
(deleted), text/x-phabricator-request
Details
Bug 181137 - part 3: Make all users of ContentSutreeIterator treat it directly rather than via nsIContentIterator
(deleted), text/x-phabricator-request
Details
Bug 181137 - part 4: Make all users of PreContentIterator treat it directly rather than via nsIContentIterator
(deleted), text/x-phabricator-request
Details
Bug 181137 - part 5: Make all users of PostContentIterator treat it directly rather than via nsIContentIterator
(deleted), text/x-phabricator-request
Details
Bug 181137 - part 6: Make nsFilteredContentIterator not derived from nsIContentIterator
(deleted), text/x-phabricator-request
Details
Bug 181137 - part 7: Get rid of nsIContentIterator interface
(deleted), text/x-phabricator-request
Details
Bug 181137 - part 8: Make ContentIteratorBase and its subclasses non-refcountable
(deleted), text/x-phabricator-request
Details
Bug 181137 - part 9: Move kNSURIs in Document.cpp and FragmentOrElement.cpp into their users
(deleted), text/x-phabricator-request
Details
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.

Attachment

General

Creator:
Created:
Updated:
Size: