1407858 - Add first-class literal support to DOMString

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Nicholas Nethercote [inactive]]

This is a follow-up to bug 1407117, which adds code that assigns static atom literals to DOMStrings via nsString.

It would be better to allow the literals to be assigned directly, without consructing/destructing an nsString.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Give DOMString setter APIs clearer names

The renaming here is like this:

  SetStringBuffer -> SetKnownLiveStringBuffer
  SetEphemeralStringBuffer -> SetStringBuffer
  SetOwnedString -> SetKnownLiveString
  SetOwnedAtom -> SetKnownLiveAtom

This should make it clearer what the lifetime expectations are on the caller side.

MozReview-Commit-ID: ERHbB3r6paN

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make DOMString's data model clearer and update various documentation

MozReview-Commit-ID: AaTeI1e7Qnk

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add a literal string state to DOMString

MozReview-Commit-ID: EsgAAcfDFKt

Comment 4

[Boris Zbarsky [:bzbarsky]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c55bfe679aaef4ff73901543e578e280b33fe928&selectedJob=153090080

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 merged to tip

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8938596 [details] [diff] [review]
part 2.  Make DOMString's data model clearer and update various documentation

>  * The proper way to extract a value is to check IsNull().  If not null, then
>- * check HasStringBuffer().  If that's true, check for a zero length, and if the
>- * length is nonzero call StringBuffer().  If the length is zero this is the
>- * empty string.  If HasStringBuffer() returns false, call AsAString() and get
>- * the value from that.
>+ * check IsEmpty().  If neither of those is true, check HasStringBuffer().  If
>+ * that's true, call StringBuffer().  If HasStringBuffer() returns false, call
>+ * AsAString() and get the value from that.
This all doesn't seem to quite capture the case that AsString() can be called when IsEmpty() returns true.

>+
>+  enum class State : uint8_t {
Nit, { should be on its own line

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8938597 [details] [diff] [review]
part 3.  Add a literal string state to DOMString

>+  // Get the literal string.  This can only be called if HasLiteral()
>+  // returned true.  If that's true, it will never return null.
>+  const char16_t* Literal() const
>+  {
>+    MOZ_ASSERT(HasLiteral(),
>+               "Don't ask for the literal if we don't have it");
>+    MOZ_ASSERT(mStringBuffer,
>+               "We better have a literal if we claim to");
mStringBuffer? Shouldn't you use mLiteral here.
Both point to the same value, but would make reading easier.


>-  // The nsStringBuffer in the OwnedStringBuffer/UnownedStringBuffer cases.
>-  nsStringBuffer* MOZ_UNSAFE_REF("The ways in which this can be safe are "
>+  union {
I think { should be on its own line.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> This all doesn't seem to quite capture the case that AsString() can be called when IsEmpty() returns true.

Well, for _extracting_ a value that's not really relevant, since if IsEmpty() is true then the value is empty.

> Nit, { should be on its own line

Done.

> mStringBuffer? Shouldn't you use mLiteral here.

Yes, good catch.

> I think { should be on its own line.

Done.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a229bf234505
part 1.  Give DOMString setter APIs clearer names.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/282695389a27
part 2.  Make DOMString's data model clearer and update various documentation.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/45fe585acac1
part 3.  Add a literal string state to DOMString.  r=smaug

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/a229bf234505
https://hg.mozilla.org/mozilla-central/rev/282695389a27
https://hg.mozilla.org/mozilla-central/rev/45fe585acac1

Comment 11

[Gerry Chang [:gchang]]

Too late for FF58. Mark 58 won't fix.