888864 - Changing a stylesheet ProcessingInstruction's data no longer applies immediately

Status: RESOLVED FIXED

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

Description

[:Gijs (he/him)]

STR:

1. Load XHTML document with ?xml-stylesheet processing instruction.
2. Use JS to change the xml-stylesheet's data

ER:
The document is restyled.
AR:
The document is not restyled until you reappend the PI node.

This is a regression from bug 686449.

I'll try and add a testcase in a bit.

Comment 1

[:Gijs (he/him)]

Attachment: Testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

So we need to either make SetTextInternal virtual or make the WebIDL SetData virtual.  Or add some sort of "data changed" virtual notification...

Making the WebIDL SetData virtual seems simplest.

Comment 3

[Mina Almasry]

I think I can work on this. I  I assume you mean the make SetData here virtual? http://dxr.mozilla.org/mozilla-central/source/content/events/src/nsDOMDataContainerEvent.h#l42

What else needs to be done with that?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> http://dxr.mozilla.org/mozilla-central/source/content/events
> /src/nsDOMDataContainerEvent.h#l42

No, the one at http://hg.mozilla.org/mozilla-central/annotate/18467a85acf6/content/base/src/nsGenericDOMDataNode.h#l172

> What else needs to be done with that?

Implement an override on ProcessingInstruction that calls the superclass and then updates the stylesheet.

Comment 5

[Mina Almasry]

Attachment: patch

Well that wasn't too hard. Here is a patch to fix this, with a test. Two things to mention:

- I had to implement the override on XMLStylesheetProcessingInstruction, not ProcessingInstruction. The reason for that is that XMLStylesheet inherits from nsStyleLinkElement, and therefore can call UpdateStyleSheetInternal, while ProcessingInstruction can't.

- Compiling this gives me some warnings saying "warning: 'nsTextNode::SetData' hides overloaded virtual function [-Woverloaded-virtual]". The interwebs tells me the solution is to go to all these files and put in |using nsGenericDOMDataNode::SetData();| and that unhides the virtual method so that it is usable again. Would you like me to do that? Note that this isn't because SetData became virtual. nsGenericDOMDataNode::SetData gets hidden anyway, but I think the warning is appearing because it's virtual.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 775905 [details] [diff] [review]
patch

>+    nsresult res = nsGenericDOMDataNode::SetData(aData);
>+    if (NS_FAILED(res)) {
>+      rv.Throw(res);
>+      return;

Probably better to do:

  nsGenericDOMDataNode::SetData(aData, rv);
  if (rv.Failed() {
    return;
  }

Also, make sure this is clearly marked virtual.

>- I had to implement the override on XMLStylesheetProcessingInstruction,

Yep.

> Would you like me to do that?

Yes, please.

r=me with that

Comment 7

[Mina Almasry]

Attachment: patch

Here is a patch with the changes. I'm asking for another review because... are you sure that's what you want? It seems very strange to me that I had to add both:

using nsGenericDOMDataNode::SetData;
using ProcessingInstruction::SetData;

to not get any warnings. I think the confusion is happening because now there are 2 sets of virtual functions now: one SetData(nsAString&) and one SetData(nsAString&, ErrorResult&)

Would you like me to look into this further, or is this patch good as is?

Comment 8

[Mina Almasry]

Let me add that the reason I'm confused is that I'm not sure what NS_FORWARD_NSIDOMCHARACTERDATA defines.. if you'd like me to look further into this, please point me to where that macro is defined.

Comment 9

[Josh Matthews [:jdm]]

Usually you can use MXR or DXR to search for it, but since it's autogenerated from the IDL file into your build dir, you'll need to resort to grep/ack. You'll find it in $(objdir)/dist/include/nsIDOMCharacterData.h.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 775956 [details] [diff] [review]
patch

Yep, this looks exactly right.

Comment 11

[Mina Almasry]

> Yep, this looks exactly right.

Alrighty you're da boss. Marking it checkin-needed.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8421bc750585

Keep in mind for future patches that in general, the commit message should be summarizing what the patch is actually doing, not just restating the bug title.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for Werror bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/130d254e9e44

https://tbpl.mozilla.org/php/getParsedLog.php?id=25330135&tree=Mozilla-Inbound

Comment 14

[Mina Almasry]

Attachment: patch that builds... please push to tryserver

Strange... this patch builds for me, even with -Werror turned on. And I tested that by removing and readding some using declarations line.

Can someone be so kind and push this to the try server for me?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=f78e5a49472f

Comment 16

[Mina Almasry]

Attachment: final (I think) patch

Right... so I had an extra using declaration I don't need, and that was making the build on linux fail, but not Mac or Windows.

This patch builds on OS X and Linux, so I'm not bothering with finding someone to push it to the try server for me. Also the last review was +, so I'm not asking for another review. I'm marking it checkin-needed, feel free to reverse if wrong.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

I was hoping you were going to fix up the commit message too :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fff53368c4e

Comment 18

[Mina Almasry]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> I was hoping you were going to fix up the commit message too :(

My mistake. Sincerely sorry and will remember next time. Thanks for the push!

Comment 19

[Mina Almasry]

I think this has been resolved. The patch is in the tree.

Comment 20

[Boris Zbarsky [:bzbarsky]]

It's in inbound.  It hasn't been merged to mozilla-central yet, which is when this bug would get marked fixed by the merge tool.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7fff53368c4e