835800 - Convert more DOM attribute reflectors to DOMString

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 834877 for reasoning.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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());
  }

Comment 2

[Boris Zbarsky [:bzbarsky]]

Actually, I spun off localName to bug 925495.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So I looked into doing this by replacing the jsop_name call in the getgname case with something like this:

        MConstant *insGlobal = constant(ObjectValue(*obj));
        current->push(insGlobal);
        return jsop_getprop(name);

but I think that's not enough on its own; at the very least BaselineInspector needs to be taught about the possibility of maybeShapesForPropertyOp being called in cases when the IC is for a name, not a getprop...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Er, comment 3 has nothing to do with this bug.

Comment 5

[(no longer active)]

Does this require manually going through all WebIDL files and look for DOMString attributes and change the underlying C++ implementation for each one?

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Dhi Aurrahman]

Hi Boris,

Could you point out some relevant tests? Thank you.

Comment 8

[Boris Zbarsky [: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.

Comment 9

[Dhi Aurrahman]

(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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  You want dom/html/test/*reflection.html and dom/html/test/forms/*reflection.html.

Comment 11

[Dhi Aurrahman]

Attachment: First attempt

Hi Boris,

This is my first attempt on creating the patch, based on my interpretation on comment 8.

Comment 12

[Boris Zbarsky [: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!

Comment 13

[Dhi Aurrahman]

Attachment: 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!

Comment 14

[Boris Zbarsky [: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.

Comment 15

[Dhi Aurrahman]

Attachment: Follow up after comment 14

Update the patch based on comment 14.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8563815 [details] [diff] [review]
Follow up after comment 14

r=me

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d549ebdfa54

Comment 18

[Boris Zbarsky [:bzbarsky]]

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?

Comment 19

[Dhi Aurrahman]

Hi Boris,

I'll take a look. Thanks!

Comment 20

[Dhi Aurrahman]

Attachment: 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?

Comment 21

[Boris Zbarsky [: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);

Comment 22

[Dhi Aurrahman]

Attachment: Follow up after comment 21

Follow up after comment 21.

Try push at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e87d08c62771

Comment 23

[Dhi Aurrahman]

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).

Comment 24

[Boris Zbarsky [: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.

Comment 25

[Boris Zbarsky [: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!

Comment 26

[Dhi Aurrahman]

Attachment: Follow up after comment 25

Try push at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0565b3e7c271

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8566699 [details] [diff] [review]
Follow up after comment 25

r=me.  Thank you for sticking with this!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c1fc54afdf5

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0c1fc54afdf5