Closed Bug 1343978 Opened 7 years ago Closed 7 years ago

ClientRectsAndTexts structure should not use DOMStringList

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(1 file)

DOMStringList is an obsolete type, according to https://developer.mozilla.org/en-US/docs/Web/API/DOMStringList. The Range GetClientRectsAndTexts function returns a structure that contains a DOMStringList. It should instead use a sequence<DOMString>.

https://bugzilla.mozilla.org/show_bug.cgi?id=1314080#c37
Attachment #8843001 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #2)
> Also https://bugzilla.mozilla.org/show_bug.cgi?id=1314080#c9 ;)

Yes, that too. I'm sorry I didn't address this issue in the original code.
Comment on attachment 8843001 [details]
Bug 1343978 Part 1: Change ClientRectsAndTexts usage of DOMStringList to Sequence<DOMString>.

https://reviewboard.mozilla.org/r/116744/#review118412

::: layout/base/nsLayoutUtils.cpp:3975
(Diff revision 1)
>      }
>      mCallback->AddRect(r);
>    }
>  };
>  
>  struct BoxToRectAndText : public BoxToRect {

Random note, this struct could use MOZ_RAII annotation.

::: layout/base/nsLayoutUtils.cpp:3982
(Diff revision 1)
>  
>    BoxToRectAndText(nsIFrame* aRelativeTo, nsLayoutUtils::RectCallback* aCallback,
> -                   mozilla::dom::DOMStringList* aTextList, uint32_t aFlags)
> +                   Sequence<nsString>* aTextList, uint32_t aFlags)
>      : BoxToRect(aRelativeTo, aCallback, aFlags), mTextList(aTextList) {}
>  
> -  static void AccumulateText(nsIFrame* aFrame, nsAString& aResult) {
> +  static void AccumulateText(nsIFrame* aFrame, nsAString* aResult) {

Passing & would indicate the value is never null...

::: layout/base/nsLayoutUtils.cpp:4014
(Diff revision 1)
>    virtual void AddBox(nsIFrame* aFrame) override {
>      BoxToRect::AddBox(aFrame);
>      if (mTextList) {
> -      nsAutoString textForFrame;
> +      nsString* textForFrame = mTextList->AppendElement(fallible);
> +      if (textForFrame) {
> -      AccumulateText(aFrame, textForFrame);
> +        AccumulateText(aFrame, textForFrame);

...so perhaps pass *textForFrame here?
Attachment #8843001 - Flags: review?(bugs) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1f560050c31
Part 1: Change ClientRectsAndTexts usage of DOMStringList to Sequence<DOMString>. r=smaug
https://hg.mozilla.org/mozilla-central/rev/d1f560050c31
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.