Closed Bug 888864 Opened 6 years ago Closed 6 years ago

Changing a stylesheet ProcessingInstruction's data no longer applies immediately

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: Gijs, Assigned: almasry.mina)

References

Details

(Keywords: regression, testcase, Whiteboard: [mentor=bz][lang=c++])

Attachments

(2 files, 3 obsolete files)

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.
Attached file Testcase
Keywords: testcase
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.
Whiteboard: [mentor=bz][lang=c++]
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?
Assignee: nobody → almasry.mina
> 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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #775905 - Flags: review?(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
Attachment #775905 - Flags: review?(bzbarsky) → review+
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #775905 - Attachment is obsolete: true
Attachment #775956 - Flags: review?(bzbarsky)
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.
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.
Attachment #775956 - Attachment is patch: true
Attachment #775956 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 775956 [details] [diff] [review]
patch

Yep, this looks exactly right.
Attachment #775956 - Flags: review?(bzbarsky) → review+
> Yep, this looks exactly right.

Alrighty you're da boss. Marking it checkin-needed.
Keywords: checkin-needed
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.
Flags: in-testsuite+
Keywords: checkin-needed
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?
Attachment #775956 - Attachment is obsolete: true
Keywords: push-needed
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.
Attachment #776723 - Attachment is obsolete: true
Keywords: checkin-needed
I was hoping you were going to fix up the commit message too :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fff53368c4e
Keywords: checkin-needed
(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!
I think this has been resolved. The patch is in the tree.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/7fff53368c4e
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.