598105 - Support the HTML5 data-* attributes in the sanitizing fragment sink

Status: RESOLVED FIXED

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

Description

[(no longer active)]

HTML5 allows data-* attributes, and we need to allow them in the sanitizing fragment content sink as well.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 476901 [details] [diff] [review]
Patch (v1)

>+        if (k.Find(NS_LITERAL_STRING("data-")) != 0) {

  if (!StringBeginsWith(k, NS_LITERAL_STRING("data-"))) }

seems more idiomatic.

Also, instead of nested ifs, would it make sense to make use of "&&" here?  At this point we have three-deep nesting for what is fundamentally just an and on several boolean expressions....

r=me with those nits.

Comment 3

[(no longer active)]

Attachment: Patch (v1.1)

With comments addressed.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> +    if ((!sAllowedAttributes && !sAllowedAttributes->GetEntry(keyAtom)) &&

That first && should be ||.

Comment 5

[(no longer active)]

Attachment: Patch (v1.2)

(In reply to comment #4)
> > +    if ((!sAllowedAttributes && !sAllowedAttributes->GetEntry(keyAtom)) &&
> 
> That first && should be ||.

Of course!

Comment 6

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/fa3912a92409

Comment 7

[LegNeato]

Comment on attachment 476912 [details] [diff] [review]
Patch (v1.2)

a=LegNeato for 1.9.2.11 and 1.9.1.14

Comment 8

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e69182117a09
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2a4d91dc89fc

Comment 9

[LegNeato]

This caused the tree to go red:

/builds/slave/mozilla-1.9.2-linux/build/content/html/document/src/nsHTMLFragmentContentSink.cpp:1111: error: no matching function for call to 'StringBeginsWith(nsCAutoString&, const nsString&)'

reopening.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, on 1.9.2 and 1.9.1 k is probably a UTF-8 string (and hence you need an NS_LITERAL_CSTRING.

I have no idea why this was reopened, though.  This is fixed on trunk.

I assume you backed it out of 1.9.2, right?  So unsetting that flag.  Did you back out of 1.9.1 as well?

Comment 11

[LegNeato]

Sorry, shouldn't have reopened. I have not backed out the patch as I do not have commit privs. Wasn't sure if we generally file new bugs for bustage or just continue on with the existing one.

Comment 12

[(no longer active)]

Bustage fix pushed on both branches.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dbd650f54b39
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c31c35eeb64e

I chose to use |key| instead of |k|, which is the UTF-16 version of the latter.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Christian, for bustage we'd generally use the existing bug, since either a bustage fix is pushed (and then we need to say what the changeset for that was) or the patch is backed out (and then whatever bugzilla bit indicates that the patch is checked in needs to be flipped back to the "not checked in" state).