DeCOMtaminate nsIContentIterator
Categories
(Core :: DOM: Core & HTML, enhancement, P5)
Tracking
()
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: kinmoz, Assigned: masayuki)
References
(Blocks 2 open bugs)
Details
Attachments
(9 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
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.
Updated•13 years ago
|
Updated•9 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
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 | ||
Comment 3•3 years ago
|
||
Looks like that we can get rid of nsIContentIterator interface completely. Taking.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 4•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec33fd60c8cd53f32be4b4ceee9b723f04c2a24a
Assignee | ||
Comment 5•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96dd92a4d25594a6bc2c14388ebfd2602f0248bf
Assignee | ||
Comment 6•3 years ago
|
||
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.
Assignee | ||
Comment 7•3 years ago
|
||
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".
Assignee | ||
Comment 8•3 years ago
|
||
Now, all users of ContentSubtreeIterator can access it directly. This patch makes them use the concrete class directly.
Assignee | ||
Comment 9•3 years ago
|
||
Now, all users of PreContentIterator can access it directly. This patch makes them use the concrete class directly.
Assignee | ||
Comment 10•3 years ago
|
||
Now, all users of PostContentIterator can access it directly. This patch makes them use the concrete class directly.
Assignee | ||
Comment 11•3 years ago
|
||
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.
Assignee | ||
Comment 12•3 years ago
|
||
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.
Assignee | ||
Comment 13•3 years ago
|
||
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.
Assignee | ||
Comment 14•3 years ago
|
||
Probably, starting to review from part 8 must be easier to understand what I want to do this here.
Assignee | ||
Comment 15•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b7a957f6f5ef71f429353b73e866d2e047b8caf
Assignee | ||
Comment 16•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fea0814f541b7dc7415de49bbf86168e8403d387
Comment 17•3 years ago
|
||
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
Comment 18•3 years ago
|
||
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'
Assignee | ||
Comment 19•3 years ago
|
||
Sigh. There is a hidden bug similar to bug 1518384...
Assignee | ||
Comment 20•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f56947859894d7b7b5665c574841d349337afea
Assignee | ||
Comment 21•3 years ago
|
||
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.
Assignee | ||
Comment 22•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0e32175b6542bbbad6186c6f59ccf6b5f2b2457
Comment 23•3 years ago
|
||
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
Comment 24•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2eaf630d9925
https://hg.mozilla.org/mozilla-central/rev/a29d33c6fca1
https://hg.mozilla.org/mozilla-central/rev/5fb4b3194d8e
https://hg.mozilla.org/mozilla-central/rev/7b087e0123bd
https://hg.mozilla.org/mozilla-central/rev/0d3c97c78d7c
https://hg.mozilla.org/mozilla-central/rev/61f2c3b1b368
https://hg.mozilla.org/mozilla-central/rev/2a320da37ca0
https://hg.mozilla.org/mozilla-central/rev/dbf919528d97
https://hg.mozilla.org/mozilla-central/rev/897a97a4931d
Assignee | ||
Comment 25•3 years ago
|
||
Wow, this improves the performance of bug 1346723 than I've expected.
Comment 26•3 years ago
|
||
Sounds great!
Memory allocation and deallocation tends to be always rather slow.
Updated•3 years ago
|
Description
•