Closed Bug 824592 Opened 7 years ago Closed 4 years ago

Make nsIDOMElementCSSInlineStyle non-scriptable once all HTML elements are on WebIDL bindings

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

We don't have a good bug for me to block this one on.  :(

Once we don't need this interface for Xrays, we can nuke it from classinfo and whatnot.
Depends on: 826738
What would be particularly awesome is if we could just nuke this interface....
I'll take this.
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
I changed the QI's to this interface to QI to nsStyledElement.
https://tbpl.mozilla.org/?tree=Try&rev=9ed0a59ad0d6
Attachment #776892 - Flags: review?(bzbarsky)
Comment on attachment 776892 [details] [diff] [review]
Patch

StyledElement doesn't have its own IID, does it?  So QIing to it is a bit weird.

Also, I believe we can get rid of nsStyledElementNotElementCSSInlineStyle now, since we no longer have to do quickstub unwrapping to nsIDOMElementCSSInlineStyle.
(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 776892 [details] [diff] [review]
> Patch
> 
> StyledElement doesn't have its own IID, does it?  So QIing to it is a bit
> weird.
> 

Right.  I can add an IID, but any idea why this doesn't cause any tests to fail?
Note having its own IID means QI will end up seeing the inherited IID.  As in, QI to nsStyledElement is equivalent to QI to Element.  Furthermore, the implementation of nsStyledElementNotElementCSSInlineStyle::Style works on all Element instances in the sense of creating the declaration.  What won't work is those styles actually applying to the element, because it'll have the wrong GetInlineStyleRule() method.

So to get fails you would need to have an Element that's not an nsStyledElement go through some codepath on which someone modifies the result of calling ->Style() and then checks whether the styles got applied or something...
Comment on attachment 776892 [details] [diff] [review]
Patch

So in any case, we can't remove this API without giving binary addons some replacement...
Attachment #776892 - Flags: review?(bzbarsky) → review-
Well, we nerfed binary addons ... can we do this now?
Flags: needinfo?(bzbarsky)
If we don't think anyone is going to try accessing this thing via the XPCOM interface, probably yes.
Flags: needinfo?(bzbarsky)
Assignee: dzbarsky → bzbarsky
Attachment #8771597 - Flags: review?(peterv)
This is basically David's patch merged to tip.  The merge wasn't quite trivial, so I figure someone else should look at this too, though I've basically reviewed it....
Attachment #8771598 - Flags: review?(peterv)
Comment on attachment 8771596 [details] [diff] [review]
part 1.  Get rid of nsStyledElementNotElementCSSInlineStyle, since quickstubs are long-since gone

Review of attachment 8771596 [details] [diff] [review]:
-----------------------------------------------------------------

Man, I had to look up why we did this, because I didn't remember anymore.
Attachment #8771596 - Flags: review?(peterv) → review+
Attachment #8771597 - Flags: review?(peterv) → review+
Comment on attachment 8771598 [details] [diff] [review]
part 3.  Get rid of nsIDOMElementCSSInlineStyle

Review of attachment 8771598 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/ChangeStyleTransaction.cpp
@@ +142,3 @@
>    NS_ENSURE_TRUE(inlineStyles, NS_ERROR_NULL_POINTER);
>  
> +  nsICSSDeclaration* cssDecl = inlineStyles->Style();

I guess there's no danger of cssDecl becoming dangling?
Attachment #8771598 - Flags: review?(peterv) → review+
> I guess there's no danger of cssDecl becoming dangling?

Oh, good catch.  I don't actually know, what with mutation events.

I'll make both of those strong refs.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65012571578d
part 1.  Get rid of nsStyledElementNotElementCSSInlineStyle, since quickstubs are long-since gone.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/68f5dadf42ac
part 2.  Give nsStyledElement an IID.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe1c4a7fdef
part 3.  Get rid of nsIDOMElementCSSInlineStyle.  r=peterv
https://hg.mozilla.org/mozilla-central/rev/65012571578d
https://hg.mozilla.org/mozilla-central/rev/68f5dadf42ac
https://hg.mozilla.org/mozilla-central/rev/7fe1c4a7fdef
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.