Closed
Bug 1433566
Opened 6 years ago
Closed 6 years ago
Remove nsIDOMText
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(13 files, 2 obsolete files)
1.94 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
3.41 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
4.72 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
3.19 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
4.21 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
6.03 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
1.07 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
4.43 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
6.44 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
3.36 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
4.98 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
14.86 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
2.85 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•6 years ago
|
||
Attachment #8945950 -
Flags: review?(kyle)
Updated•6 years ago
|
Attachment #8945950 -
Flags: review?(kyle) → review+
Updated•6 years ago
|
Priority: -- → P2
Assignee | ||
Comment 2•6 years ago
|
||
Ehsan, is there something blocking this landing?
Flags: needinfo?(ehsan)
Reporter | ||
Comment 4•6 years ago
|
||
Yes, when I pushed this to try a long time ago it broke tests. Unfortunately I haven't had time to look into why yet. :-/
Flags: needinfo?(ehsan)
Assignee | ||
Comment 5•6 years ago
|
||
OK. Do you mind if I push to try the patches I wrote in bug 1445354?
Assignee | ||
Comment 6•6 years ago
|
||
Those patches are green on try, fwiw. Mind if I just steal the bug?
Flags: needinfo?(ehsan)
Reporter | ||
Comment 7•6 years ago
|
||
Not at all, please go ahead.
Assignee: ehsan → bzbarsky
Flags: needinfo?(ehsan)
Assignee | ||
Comment 8•6 years ago
|
||
MozReview-Commit-ID: Ik5hGubUDqX
Attachment #8959416 -
Flags: review?(nika)
Assignee | ||
Comment 9•6 years ago
|
||
MozReview-Commit-ID: LtAhn223y0s
Attachment #8959417 -
Flags: review?(nika)
Assignee | ||
Comment 10•6 years ago
|
||
MozReview-Commit-ID: 1jghu75LKDw
Attachment #8959418 -
Flags: review?(nika)
Assignee | ||
Comment 11•6 years ago
|
||
MozReview-Commit-ID: RUJSiOt7Av
Attachment #8959419 -
Flags: review?(nika)
Assignee | ||
Comment 12•6 years ago
|
||
MozReview-Commit-ID: 255TGkYJd4q
Attachment #8959420 -
Flags: review?(nika)
Assignee | ||
Comment 13•6 years ago
|
||
This is mostly a straightforward copy. The only change is what happens with the return value of CloneDataNode: it's cast to Text directly, and the null-check is replaced by an assert. MozReview-Commit-ID: B5rnhEOneZn
Attachment #8959421 -
Flags: review?(nika)
Assignee | ||
Comment 14•6 years ago
|
||
MozReview-Commit-ID: I8ZB56a82ze
Attachment #8959422 -
Flags: review?(nika)
Assignee | ||
Comment 15•6 years ago
|
||
MozReview-Commit-ID: 1ueFgT3ANRS
Attachment #8959423 -
Flags: review?(nika)
Assignee | ||
Comment 16•6 years ago
|
||
Just code motion, no changes. MozReview-Commit-ID: 98LkyuU4mya
Attachment #8959424 -
Flags: review?(nika)
Assignee | ||
Comment 17•6 years ago
|
||
MozReview-Commit-ID: 4M4q41mEpgb
Attachment #8959425 -
Flags: review?(nika)
Assignee | ||
Comment 18•6 years ago
|
||
MozReview-Commit-ID: 2EY6w5YlLCH
Attachment #8959426 -
Flags: review?(nika)
Assignee | ||
Comment 19•6 years ago
|
||
MozReview-Commit-ID: KliYtj5U8jK
Attachment #8959427 -
Flags: review?(nika)
Assignee | ||
Comment 20•6 years ago
|
||
MozReview-Commit-ID: DvaZ96j5exf
Attachment #8959428 -
Flags: review?(nika)
Assignee | ||
Updated•6 years ago
|
Attachment #8945950 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #8959416 -
Flags: review?(nika) → review+
Updated•6 years ago
|
Attachment #8959417 -
Flags: review?(nika) → review+
Comment 21•6 years ago
|
||
Comment on attachment 8959418 [details] [diff] [review] part 3. Remove nsIDOMText::SplitText Review of attachment 8959418 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/core/nsIDOMText.idl @@ +11,2 @@ > * > * For more information on this interface please see I'd complain about the trailing whitespace, but we all know how this patchset is going to end ^_^
Attachment #8959418 -
Flags: review?(nika) → review+
Updated•6 years ago
|
Attachment #8959419 -
Flags: review?(nika) → review+
Updated•6 years ago
|
Attachment #8959420 -
Flags: review?(nika) → review+
Comment 22•6 years ago
|
||
Comment on attachment 8959421 [details] [diff] [review] part 6. Inline SplitData into its one caller Review of attachment 8959421 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Text.cpp @@ +33,5 @@ > + mozAutoDocUpdate updateBatch(document, UPDATE_CONTENT_MODEL, true); > + > + // Use Clone for creating the new node so that the new node is of same class > + // as this node! > + nsGenericDOMDataNode* clone = CloneDataNode(mNodeInfo, false); Eek - this is returning a fresh pointer to a refcounted object without addrefing it? Can you file a follow-up to change CloneDataNode to return an already_AddRefed?
Attachment #8959421 -
Flags: review?(nika) → review+
Updated•6 years ago
|
Attachment #8959422 -
Flags: review?(nika) → review+
Comment 23•6 years ago
|
||
Comment on attachment 8959423 [details] [diff] [review] part 8. Convert nsGenericDOMDataNode::GetWholeText to webidl-style calling convention Review of attachment 8959423 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Text.h @@ +27,5 @@ > using nsGenericDOMDataNode::GetWholeText; > > // WebIDL API > already_AddRefed<Text> SplitText(uint32_t aOffset, ErrorResult& rv); > void GetWholeText(nsAString& aWholeText, ErrorResult& rv) Why can't we toss this method completely, and just use the `using` statement 4 lines above (or even toss them both?) ::: dom/base/nsGenericDOMDataNode.cpp @@ +749,5 @@ > nsIContent* parent = GetParent(); > > // Handle parent-less nodes > + if (!parent) { > + GetData(aWholeText); GetData always returns NS_OK - but it might be good to have an ALWAYS_SUCCEEDS here?
Attachment #8959423 -
Flags: review?(nika) → review+
Comment 24•6 years ago
|
||
Comment on attachment 8959424 [details] [diff] [review] part 9. Move GetWholeText over into Text.cpp Review of attachment 8959424 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Text.h @@ +27,3 @@ > // WebIDL API > already_AddRefed<Text> SplitText(uint32_t aOffset, ErrorResult& rv); > + void GetWholeText(nsAString& aWholeText, ErrorResult& rv); ah. this is fine then.
Attachment #8959424 -
Flags: review?(nika) → review+
Comment 25•6 years ago
|
||
Comment on attachment 8959425 [details] [diff] [review] part 10. Stop using nsIDOMText in GetWholeText Review of attachment 8959425 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Text.cpp @@ +66,1 @@ > FirstLogicallyAdjacentTextNode(nsIContent* aNode) I think this would be cleaner if you change the argument type from nsIContent* to Text*. @@ +66,4 @@ > FirstLogicallyAdjacentTextNode(nsIContent* aNode) > { > do { > + MOZ_ASSERT(aNode->IsText()); This assert would be unnecessary. @@ +71,3 @@ > nsIContent* sibling = aNode->GetPreviousSibling(); > if (!sibling || !sibling->IsText()) { > + return static_cast<Text*>(aNode); This cast would be unnecessary, and. @@ +74,2 @@ > } > aNode = sibling; You could just cast here because you just checked it was a Text on the previous line. @@ +80,1 @@ > LastLogicallyAdjacentTextNode(nsIContent* aNode) Same with this one. @@ +111,5 @@ > aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR); > return; > } > > + Text* first = FirstLogicallyAdjacentTextNode(this); We even always pass in a `this`, which we already know is Text :-)
Attachment #8959425 -
Flags: review?(nika) → review-
Updated•6 years ago
|
Attachment #8959426 -
Flags: review?(nika) → review+
Updated•6 years ago
|
Attachment #8959427 -
Flags: review?(nika) → review+
Updated•6 years ago
|
Attachment #8959428 -
Flags: review?(nika) → review+
Assignee | ||
Comment 26•6 years ago
|
||
> Can you file a follow-up to change CloneDataNode to return an already_AddRefed? Bug 1446530. > GetData always returns NS_OK - but it might be good to have an ALWAYS_SUCCEEDS here? I'm just going to work on bug 1446533 instead. ;) > I think this would be cleaner if you change the argument type from nsIContent* to Text*. Good point. Fixed.
Assignee | ||
Comment 27•6 years ago
|
||
Attachment #8959697 -
Flags: review?(nika)
Assignee | ||
Updated•6 years ago
|
Attachment #8959425 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #8959697 -
Flags: review?(nika) → review+
Comment 28•6 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/708098d3b655 part 1. Add a non-virtual nsINode::IsText(). r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f45612dea4 part 2. Remove use of nsIDOMText from JS. r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/808ac1023bd6 part 3. Remove nsIDOMText::SplitText. r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/de3b0870addd part 4. Remove unused aCloneAfterOriginal arg of nsGenericDOMDataNode::SplitData. r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/5933f9858344 part 5. Convert nsGenericDOMDataNode::SplitData to webidl calling conventions. r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/aabc687a81eb part 6. Inline SplitData into its one caller. r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/a9b70292b553 part 7. Remove nsIDOMText::GetWholeText. r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/db5a1e68d3b1 part 8. Convert nsGenericDOMDataNode::GetWholeText to webidl-style calling convention. r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/c6db91d55c9c part 9. Move GetWholeText over into Text.cpp. r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/e91f19e48e76 part 10. Stop using nsIDOMText in GetWholeText. r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/28033c867851 part 11. Stop using nsIDOMText in range code. r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/f568bf6abac5 part 12. Stop using nsIDOMText in layout. r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/e693589dd8d4 part 13. Remove nsIDOMText. r=mystor
Comment 29•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/708098d3b655 https://hg.mozilla.org/mozilla-central/rev/a7f45612dea4 https://hg.mozilla.org/mozilla-central/rev/808ac1023bd6 https://hg.mozilla.org/mozilla-central/rev/de3b0870addd https://hg.mozilla.org/mozilla-central/rev/5933f9858344 https://hg.mozilla.org/mozilla-central/rev/aabc687a81eb https://hg.mozilla.org/mozilla-central/rev/a9b70292b553 https://hg.mozilla.org/mozilla-central/rev/db5a1e68d3b1 https://hg.mozilla.org/mozilla-central/rev/c6db91d55c9c https://hg.mozilla.org/mozilla-central/rev/e91f19e48e76 https://hg.mozilla.org/mozilla-central/rev/28033c867851 https://hg.mozilla.org/mozilla-central/rev/f568bf6abac5 https://hg.mozilla.org/mozilla-central/rev/e693589dd8d4
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•