If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Convert more DOM attribute reflectors to DOMString

RESOLVED FIXED in Firefox 38

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bz, Assigned: Dhi Aurrahman, Mentored)

Tracking

unspecified
mozilla38
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 6 obsolete attachments)

See bug 834877 for reasoning.
Assignee: bzbarsky → nobody
Whiteboard: [mentor=bz][lang=c++]
Note to self.  Can also do this for Node.localName:

  void GetLocalName(mozilla::dom::DOMString& aLocalName)
  {
    const nsString& localName = mNodeInfo->LocalName();
    aLocalName.SetStringBuffer(nsStringBuffer::FromString(localName),
                               localName.Length());
  }
Actually, I spun off localName to bug 925495.
Comment hidden (obsolete)
Er, comment 3 has nothing to do with this bug.
Does this require manually going through all WebIDL files and look for DOMString attributes and change the underlying C++ implementation for each one?
This is specifically about *Element WebIDL files that have DOMString attributes that map to actual GetAttr/SetAttr() calls.  And yes, changing the C++ implementations for those.

You might be able to rename or remove the nsAString version of GetHTMLAttr and at least find a bunch of callers that way for HTML elements.
Mentor: bzbarsky@mit.edu
Whiteboard: [mentor=bz][lang=c++] → [lang=c++]
(Assignee)

Comment 7

3 years ago
Hi Boris,

Could you point out some relevant tests? Thank you.
Flags: needinfo?(bzbarsky)
This isn't really about tests...

What this is about are things that in the HTML spec are defined to "reflect" a content attribute and that in terms of our implementation are done in terms of GetHTMLAttr.  We have overloads of GetHTMLAttr that take DOMString and nsAString.  This bug is about more or less trying to get rid of the latter and make consumers use the former.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 9

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #8)
> This isn't really about tests...
> 
> What this is about are things that in the HTML spec are defined to "reflect"
> a content attribute and that in terms of our implementation are done in
> terms of GetHTMLAttr.  We have overloads of GetHTMLAttr that take DOMString
> and nsAString.  This bug is about more or less trying to get rid of the
> latter and make consumers use the former.

OK. I was just wondering, if I changed some of the implementations I want to make sure it won't break anything.
Oh, I see.  You want dom/html/test/*reflection.html and dom/html/test/forms/*reflection.html.
(Assignee)

Comment 11

3 years ago
Created attachment 8563441 [details] [diff] [review]
First attempt

Hi Boris,

This is my first attempt on creating the patch, based on my interpretation on comment 8.
Attachment #8563441 - Flags: review?(bzbarsky)
Comment on attachment 8563441 [details] [diff] [review]
First attempt

> HTMLTrackElement::CreateTextTrack()

The changes here look wrong to me.

In particular, the changes here mean we get srclang/label into DOMStrings, then pass those DOMStrings to a method that takes const nsAString&.  This will trigger the DOMString conversion operator to nsAString&, which will assert that the DOMString doesn't have any data yet.

I filed bug 1132655 on the fact that this code even compiles.

What you want to do instead is keep the versions of GetSrclang and GetLabel that take an nsString& and undo the change to this function.

>+++ b/dom/html/HTMLTrackElement.h
>+  virtual void GetItemValueText(DOMString& aText) MOZ_OVERRIDE

Can't you just pass aText directly through to GetSrc now instead of having the "value" temporary?

>@@ -3050,17 +3050,17 @@ nsGenericHTMLElement::GetItemValue(nsIVa
>+    DOMString string;
>     GetItemValueText(string);
>     out->SetAsAString(string);

This is the same problem as in HTMLTrackElement::CreateTextTrack.  What you want is probably more like:

  DOMString string;
  GetItemValueText(string);
  nsString xpcomString;
  string.ToString(xpcomString);
  out->SetAsAString(xpcomString);

>+++ b/dom/html/nsGenericHTMLElement.h
>+  void GetItemId(mozilla::dom::DOMString& aItemId)
...
>     GetHTMLURIAttr(nsGkAtoms::itemid, aItemId);

No point to this change; GetHTMLURIAttr takes an nsAString& anyway.  Just undo it, please.

r=me with those comments addressed.  Thank you for picking this up!
Attachment #8563441 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 13

3 years ago
Created attachment 8563740 [details] [diff] [review]
Follow up after comment 12

Hi Boris,

Since I'm not sure about some of the the follow up changes in this patch (after comment 12). I hope you don't mind if I put it as `r?` again. Thanks!
Attachment #8563441 - Attachment is obsolete: true
Attachment #8563740 - Flags: review?(bzbarsky)
Comment on attachment 8563740 [details] [diff] [review]
Follow up after comment 12

>@@ -3029,19 +3029,21 @@ nsGenericHTMLElement::GetItemValue(JSCon
>+  string.ToString(xpcomString);
>+  if (!xpc::NonVoidStringToJsval(aCx, xpcomString, aRetval)) {

No, what you had in your previous patch here was correct.  xpc::NonVoidStringToJsval has an overload taking DOMString.

On the other hand, this bit:

>+    DOMString string;
>     GetItemValueText(string);
>     out->SetAsAString(string);

In the _other_ nsGenericHTMLElement::GetItemValue is still wrong.

r=me with that fixed, though I'd like to see the updated patch.
Attachment #8563740 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 15

3 years ago
Created attachment 8563815 [details] [diff] [review]
Follow up after comment 14

Update the patch based on comment 14.
Attachment #8563740 - Attachment is obsolete: true
Attachment #8563815 - Flags: review?(bzbarsky)
Comment on attachment 8563815 [details] [diff] [review]
Follow up after comment 14

r=me
Attachment #8563815 - Flags: review?(bzbarsky) → review+
Assignee: nobody → diorahman
Created attachment 8564236 [details] [diff] [review]
Merged to tip

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d549ebdfa54
Attachment #8563815 - Attachment is obsolete: true
OK so this fails try.  The main thing I see failing is 

Several things fail:

1)  nsGenericHTMLElement::GetDir, the version taking nsAString.  This used to call the nsString version, but now it calls back into itself, going into infinite recursion.  It should get changed to use a DOMString on the stack.  GetLang and GetTitle will need similar treatment.

2)  HTMLBodyElement::GetText, the version taking nsAString.  This has the same issue.  So do GetALink, GetLink, GetVLink, GetBgColor, GetBackground.

Can you get those fixed, and maybe check the other getters that got converted to see if there are similar issues there, and then I'll push to try again?
Flags: needinfo?(diorahman)
(Assignee)

Comment 19

3 years ago
Hi Boris,

I'll take a look. Thanks!
Flags: needinfo?(diorahman)
(Assignee)

Comment 20

3 years ago
Created attachment 8564541 [details] [diff] [review]
An attempt after comment 18

Oops, I didn't see the `merged to tip`-patch is uploaded. Anyway, this is the patch that I pushed to try server https://treeherder.mozilla.org/#/jobs?repo=try&revision=a36a1402750b (got it failed to this infamous bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1101288)

Still not sure with the additional steps for each getters. Is letting nsTSubstring::Assign() to accept DOMString a viable solution?
Attachment #8564541 - Flags: feedback?(bzbarsky)
Comment on attachment 8564541 [details] [diff] [review]
An attempt after comment 18

>+    nsString string;
>+    dir.ToString(string);
>+    aDir.Assign(string);

How about:

  dir.ToString(aDir);
Attachment #8564541 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 22

3 years ago
Created attachment 8564631 [details] [diff] [review]
Follow up after comment 21

Follow up after comment 21.

Try push at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e87d08c62771
Attachment #8564541 - Attachment is obsolete: true
Attachment #8564631 - Flags: review?(bzbarsky)
(Assignee)

Comment 23

3 years ago
The failed test at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e87d08c62771 i.e. 

`./mach mochitest-browser browser/components/customizableui/test/browser_970511_undo_restore_default.js` (in debug build)

Should not be failed if we add the `requestLongerTimeout(N)`. However, I'm not sure either that patch is even necessary or we should add that requestLongerTimeout inside this patch (or not).
Flags: needinfo?(bzbarsky)
Sorry about the lag here....

That timeout is a known intermittent, so not something you should worry about in this patch.  I retriggered that one test run so we can see whether this patch makes the timeout always happen, but I think that's pretty unlikely.
Flags: needinfo?(bzbarsky)
Comment on attachment 8564631 [details] [diff] [review]
Follow up after comment 21

Don't you also need to fix HTMLDivElement::GetAlign (the version taking nsAString)?  I guess nothing in our tests calls it, but extensions could.

Also, HTMLFrameSetElement::GetCols/GetRows.

Also HTMLTableCellElement::GetHeaders/GetAbbr/GetScope/GetAlign/GetAxis/GetHeight/GetWidth/GetCh/GetChOff/GetVAlign/GetBgColor.

r=me with those fixed, but please do post an updated patch!
Attachment #8564631 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 26

3 years ago
Created attachment 8566699 [details] [diff] [review]
Follow up after comment 25

Try push at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0565b3e7c271
Attachment #8564631 - Attachment is obsolete: true
Attachment #8566699 - Flags: review?(bzbarsky)
Comment on attachment 8566699 [details] [diff] [review]
Follow up after comment 25

r=me.  Thank you for sticking with this!
Attachment #8566699 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
Attachment #8564236 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c1fc54afdf5
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0c1fc54afdf5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.