In a Web Extensions world, there should be no need for all of the XPIDL interfaces for CSS style sheets and rules that we have.

While looking at this, I encountered the interface shimming we do here: https://searchfox.org/mozilla-central/rev/0bc3300a3abc4b12f14bd0ddf77ce8aa8125043c/dom/base/nsDOMClassInfo.cpp#1681 A few of the interfaces I was looking at removing are in there. Boris, what sort of Telemetry usage do you think we should get down to before we can remove this shimming? It looks like it's been hovering around a bit over 2%: https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=bucket-0!bucket-1&cumulative=0&end_date=2018-01-01&keys=!__none__!__none__&max_channel_version=nightly%252F59&measure=COMPONENTS_SHIM_ACCESSED_BY_CONTENT&min_channel_version=nightly%252F56&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-12-02&trim=1&use_submission_date=0 I guess that's still too high. Luckily it looks like the shimming doesn't need the XPIDL interfaces to actually exist, since we're hard coding the JS properties to set to the corresponding Web IDL interface objects.

> Boris, what sort of Telemetry usage do you think we should get down to before we can remove this shimming? You mean remove the Components shimming in general? Hard to say; would have to figure out what people are using Components for and what would break. If we got it to 0.1% or lower we could consider removal without understanding uses... But the telemetry measures Components usage in general. It's possible that the nsIDOMCSS* bits aren't used at all; we have no way to tell from this telemetry. The interfaces were added to this list simply because they had constants, not because there was any obvious indication that they were used. That said, we certainly have code in-tree that does Ci.nsIDOMCSSRule. But I'm not sure that runs against the shim, and of course we can change this code.

Jörg, same as bug 1427419 applies here too. There are a few nsIDOMCSS* interfaces my patches will remove, which are still used by comm-central/mozilla/extensions/inspector/.

Yep, thanks, same NI forward to the SM folks.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2) > But the telemetry measures Components usage in general. It's possible that > the nsIDOMCSS* bits aren't used at all; we have no way to tell from this > telemetry. The interfaces were added to this list simply because they had > constants, not because there was any obvious indication that they were used. OK. Well, for now I'll leave the shimming as is. > That said, we certainly have code in-tree that does Ci.nsIDOMCSSRule. But > I'm not sure that runs against the shim, and of course we can change this > code. Yeah, the patches here should fix all of those up.

r?jryans for the devtools/ changes, r?bz for the dom/webidl/ and xpcom/ changes, and r?xidorn for the rest. Also, Markus, can you tell me if there is some way I can test the changes I've made in part 24? I've just made them blindly. I assume on macOS we have some UI that uses this but I'm not sure. Thanks!

Comment on attachment 8940135 [details] Bug 1427512 - Part 24: Stop using XPCOM interfaces to access CSS property values in nsMenuItemIconX. https://reviewboard.mozilla.org/r/210452/#review216136 Looks good, with the review comment applied. (There was a mistake in the computation of "left".) It looks like we don't use -moz-image-region in menus anywhere in our current Firefox frontend CSS code. IIRC this was primarily used by "complete themes" which we no longer support. But it's possible that Thunderbird or Seamonkey still use it. In order to test your changes, you can try adding a -moz-image-region property here: https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/browser/themes/osx/browser.css#192 And then make sure that the icon next to the "Bookmarks Toolbar" item in the the Bookmarks menu reacts to the image region values in the right ways. You can compare it to the non-native menu that you get when you open chrome://browser/content/browser.xul in a new tab and click on the Bookmarks menu in there. ::: widget/cocoa/nsMenuItemIconX.mm:193 (Diff revision 1) > - // Return NS_ERROR_FAILURE if the image region is invalid so the image > + // Return NS_ERROR_FAILURE if the image region is invalid so the image > - // is not drawn, and behavior is similar to XUL menus. > + // is not drawn, and behavior is similar to XUL menus. > - int32_t bottom = GetDOMRectSide(imageRegionRect, &nsIDOMRect::GetBottom); > - int32_t right = GetDOMRectSide(imageRegionRect, &nsIDOMRect::GetRight); > - int32_t top = GetDOMRectSide(imageRegionRect, &nsIDOMRect::GetTop); > - int32_t left = GetDOMRectSide(imageRegionRect, &nsIDOMRect::GetLeft); > + int32_t bottom = r.YMost(); > + int32_t right = r.XMost(); > + int32_t top = r.Y(); > + int32_t left = r.YMost(); > > - if (top < 0 || left < 0 || bottom <= top || right <= left) > + if (top < 0 || left < 0 || bottom <= top || right <= left) > - return NS_ERROR_FAILURE; > + return NS_ERROR_FAILURE; This check can be instead: if (r.X() < 0 || r.Y() < 0 || r.IsEmpty())

Comment on attachment 8940114 [details] Bug 1427512 - Part 3: Remove nsIDOMCSSMediaRule. https://reviewboard.mozilla.org/r/210410/#review216168 Thanks, /devtools changes look good!

Comment on attachment 8940120 [details] Bug 1427512 - Part 9: Remove nsIDOMCSSImportRule. https://reviewboard.mozilla.org/r/210422/#review216172 Thanks, /devtools changes look good! ::: devtools/server/actors/styles.js:1317 (Diff revision 1) > let document; > if (this.rawNode) { > document = this.rawNode.ownerDocument; > } else { > let parentStyleSheet = this._parentSheet; > - while (parentStyleSheet.ownerRule && > + while (parentStyleSheet.ownerRule) { Hmm, I think we still want to test the rule type here...? Can you add `parentStyleSheet.ownerRule.class == "CSSImportRule"`?

Comment on attachment 8940120 [details] Bug 1427512 - Part 9: Remove nsIDOMCSSImportRule. https://reviewboard.mozilla.org/r/210422/#review216172 > Hmm, I think we still want to test the rule type here...? Can you add `parentStyleSheet.ownerRule.class == "CSSImportRule"`? The ownerRule of a sheet is always an import rule.

Comment on attachment 8940127 [details] Bug 1427512 - Part 16: Remove nsIDOMCSSRule. https://reviewboard.mozilla.org/r/210436/#review216182 Thanks, /devtools changes look good! ::: devtools/server/actors/styles.js:950 (Diff revision 1) > this.pageStyle = pageStyle; > this.rawStyle = item.style; > this._parentSheet = null; > this._onStyleApplied = this._onStyleApplied.bind(this); > > - if (item instanceof (Ci.nsIDOMCSSRule)) { > + if (item instanceof CSSRule) { Does this kind of `instanceof` check work across different JS compartments? (I assume DevTools tests would fail if it doesn't...) The `CSSRule` here is from a sandbox, while the items being tested are from the document, so we should ensure that kind of `instanceof` does the right thing.

Comment on attachment 8940127 [details] Bug 1427512 - Part 16: Remove nsIDOMCSSRule. https://reviewboard.mozilla.org/r/210436/#review216182 > Does this kind of `instanceof` check work across different JS compartments? (I assume DevTools tests would fail if it doesn't...) > > The `CSSRule` here is from a sandbox, while the items being tested are from the document, so we should ensure that kind of `instanceof` does the right thing. The check works across different compartments now, but we plan to make it stop working because we've failed to convince other browsers to follow suit on it working. We should ideally have some other solution here... Unfortunately a class check doesn't work for this. Maybe we could keep the instanceof thing working in the system-principal case or something if we really had to, at the cost of a bunch of complexity...

Comment on attachment 8940130 [details] Bug 1427512 - Part 19: Remove nsIDOMCSSRuleList. https://reviewboard.mozilla.org/r/210442/#review216204 Thanks, /devtools changes look good!

Comment on attachment 8940126 [details] Bug 1427512 - Part 15: Remove nsIDOMCSSStyleRule. https://reviewboard.mozilla.org/r/210434/#review216206 Thanks, /devtools changes look good!

Comment on attachment 8940129 [details] Bug 1427512 - Part 18: Remove nsIDOMCSSStyleSheet. https://reviewboard.mozilla.org/r/210440/#review216208 Thanks, /devtools changes look good!

Comment on attachment 8940131 [details] Bug 1427512 - Part 20: Remove nsIDOMMediaList. https://reviewboard.mozilla.org/r/210444/#review216210 Thanks, /devtools changes look good!

Comment on attachment 8940132 [details] Bug 1427512 - Part 21: Remove nsIDOMStyleSheetList. https://reviewboard.mozilla.org/r/210446/#review216212 Thanks, /devtools changes look good!

Comment on attachment 8940140 [details] Bug 1427512 - Part 30: Stop testing whether Ci shims should be removed. https://reviewboard.mozilla.org/r/210462/#review216214 Thanks, /devtools changes look good!

Comment on attachment 8940127 [details] Bug 1427512 - Part 16: Remove nsIDOMCSSRule. https://reviewboard.mozilla.org/r/210436/#review216248 r=me with the nits and modulo the instanceof thing. ::: layout/style/ServoPageRule.h:31 (Diff revision 1) > class ServoPageRuleDeclaration final : public nsDOMCSSDeclaration > { > public: > NS_DECL_ISUPPORTS_INHERITED > > - NS_IMETHOD GetParentRule(nsIDOMCSSRule** aParent) final; > + css::Rule* GetParentRule() final; override final? ::: layout/style/ServoStyleRule.h:34 (Diff revision 1) > class ServoStyleRuleDeclaration final : public nsDOMCSSDeclaration > { > public: > NS_DECL_ISUPPORTS_INHERITED > > - NS_IMETHOD GetParentRule(nsIDOMCSSRule** aParent) final; > + css::Rule* GetParentRule() override; override final? ::: layout/style/StyleRule.cpp:1066 (Diff revision 1) > friend class mozilla::DefaultDelete<DOMCSSDeclarationImpl>; > > public: > explicit DOMCSSDeclarationImpl(css::StyleRule *aRule); > > - NS_IMETHOD GetParentRule(nsIDOMCSSRule **aParent) override; > + css::Rule* GetParentRule() override { return mRule; } override final? ::: layout/style/nsCSSRules.h:214 (Diff revision 1) > class nsCSSKeyframeStyleDeclaration final : public nsDOMCSSDeclaration > { > public: > explicit nsCSSKeyframeStyleDeclaration(nsCSSKeyframeRule *aRule); > > - NS_IMETHOD GetParentRule(nsIDOMCSSRule **aParent) override; > + mozilla::css::Rule* GetParentRule() override; override final? ::: layout/style/nsCSSRules.h:333 (Diff revision 1) > class nsCSSPageStyleDeclaration final : public nsDOMCSSDeclaration > { > public: > explicit nsCSSPageStyleDeclaration(nsCSSPageRule *aRule); > > - NS_IMETHOD GetParentRule(nsIDOMCSSRule **aParent) override; > + mozilla::css::Rule* GetParentRule() override; override final? ::: layout/style/nsDOMCSSAttrDeclaration.h:39 (Diff revision 1) > virtual mozilla::DeclarationBlock* GetCSSDeclaration(Operation aOperation) override; > virtual void GetCSSParsingEnvironment(CSSParsingEnvironment& aCSSParseEnv, > nsIPrincipal* aSubjectPrincipal) override; > nsDOMCSSDeclaration::ServoCSSParsingEnvironment > GetServoCSSParsingEnvironment(nsIPrincipal* aSubjectPrincipal) const final; > - NS_IMETHOD GetParentRule(nsIDOMCSSRule **aParent) override; > + mozilla::css::Rule* GetParentRule() override; override final

Comment on attachment 8940129 [details] Bug 1427512 - Part 18: Remove nsIDOMCSSStyleSheet. https://reviewboard.mozilla.org/r/210440/#review216252

Comment on attachment 8940131 [details] Bug 1427512 - Part 20: Remove nsIDOMMediaList. https://reviewboard.mozilla.org/r/210444/#review216272

Comment on attachment 8940132 [details] Bug 1427512 - Part 21: Remove nsIDOMStyleSheetList. https://reviewboard.mozilla.org/r/210446/#review216284 ::: dom/base/StyleSheetList.cpp:20 (Diff revision 1) > > NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(StyleSheetList) > > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(StyleSheetList) > NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > - NS_INTERFACE_MAP_ENTRY(nsIDOMStyleSheetList) > + NS_INTERFACE_MAP_ENTRY(nsIDocumentObserver) Why? This class doesn't actually implement nsIDocumentObserver in any meaningful way.

Comment on attachment 8940132 [details] Bug 1427512 - Part 21: Remove nsIDOMStyleSheetList. https://reviewboard.mozilla.org/r/210446/#review216292 ::: dom/base/StyleSheetList.cpp:20 (Diff revision 1) > > NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(StyleSheetList) > > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(StyleSheetList) > NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > - NS_INTERFACE_MAP_ENTRY(nsIDOMStyleSheetList) > + NS_INTERFACE_MAP_ENTRY(nsIDocumentObserver) It does, it uses `nsStubDocumentObserver` to get notified when the owner node is going away.

Comment on attachment 8940132 [details] Bug 1427512 - Part 21: Remove nsIDOMStyleSheetList. https://reviewboard.mozilla.org/r/210446/#review216296 ::: dom/base/StyleSheetList.cpp:20 (Diff revision 1) > > NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(StyleSheetList) > > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(StyleSheetList) > NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > - NS_INTERFACE_MAP_ENTRY(nsIDOMStyleSheetList) > + NS_INTERFACE_MAP_ENTRY(nsIDocumentObserver) Oh right, but that's not document observer, nvm.

Comment on attachment 8940133 [details] Bug 1427512 - Part 22: Remove nsIDOMStyleSheet. https://reviewboard.mozilla.org/r/210448/#review216298 r=me modulo the error handling in GetHref. ::: layout/style/StyleSheet.cpp:335 (Diff revision 1) > -NS_IMETHODIMP > +void > StyleSheet::GetHref(nsAString& aHref) > { > if (nsIURI* sheetURI = SheetInfo().mOriginalSheetURI) { > nsAutoCString str; > - nsresult rv = sheetURI->GetSpec(str); > + sheetURI->GetSpec(str); Why is silently swallowing the error here ok?

Comment on attachment 8940134 [details] Bug 1427512 - Part 23: Remove nsIDOMCounter. https://reviewboard.mozilla.org/r/210450/#review216300

Comment on attachment 8940136 [details] Bug 1427512 - Part 25: Remove nsIDOMRect. https://reviewboard.mozilla.org/r/210454/#review216302 ::: layout/style/nsROCSSPrimitiveValue.cpp:174 (Diff revision 1) > } > case CSS_RECT : > { > NS_ASSERTION(mValue.mRect, "mValue.mRect should never be null"); > NS_NAMED_LITERAL_STRING(comma, ", "); > nsCOMPtr<nsIDOMCSSPrimitiveValue> sideCSSValue; sideCSSValue looks unused now. Kill it?

Comment on attachment 8940137 [details] Bug 1427512 - Part 26: Remove nsIDOMCSSValueList. https://reviewboard.mozilla.org/r/210456/#review216306

Comment on attachment 8940138 [details] Bug 1427512 - Part 27: Remove nsIDOMCSSPrimitiveValue. https://reviewboard.mozilla.org/r/210458/#review216308 ::: editor/libeditor/HTMLAnonymousNodeEditor.cpp:475 (Diff revision 1) > - aBorderLeft = GetCSSFloatValue(cssDecl, NS_LITERAL_STRING("border-left-width")); > - aBorderTop = GetCSSFloatValue(cssDecl, NS_LITERAL_STRING("border-top-width")); > - aMarginLeft = GetCSSFloatValue(cssDecl, NS_LITERAL_STRING("margin-left")); > - aMarginTop = GetCSSFloatValue(cssDecl, NS_LITERAL_STRING("margin-top")); > + > + const nsStyleBorder* border = sc->StyleBorder(); > + const nsStyleMargin* margin = sc->StyleMargin(); > + const nsStylePosition* position = sc->StylePosition(); > + > + aBorderLeft = nsPresContext::AppUnitsToIntCSSPixels(border->GetComputedBorder().left); This doesn't necessarily match the old behavior. The old behavior returned _used_ borders when there was a frame. Is that change ok? ::: editor/libeditor/HTMLAnonymousNodeEditor.cpp:477 (Diff revision 1) > - aMarginLeft = GetCSSFloatValue(cssDecl, NS_LITERAL_STRING("margin-left")); > - aMarginTop = GetCSSFloatValue(cssDecl, NS_LITERAL_STRING("margin-top")); > + const nsStyleMargin* margin = sc->StyleMargin(); > + const nsStylePosition* position = sc->StylePosition(); > + > + aBorderLeft = nsPresContext::AppUnitsToIntCSSPixels(border->GetComputedBorder().left); > + aBorderTop = nsPresContext::AppUnitsToIntCSSPixels(border->GetComputedBorder().top); > + aMarginLeft = GetCSSFloatValue(margin->mMargin.GetLeft()); Again, not equivalent. The old code returned used margins when there is a frame, which may well not match computed margins. ::: editor/libeditor/HTMLAnonymousNodeEditor.cpp:480 (Diff revision 1) > + aBorderLeft = nsPresContext::AppUnitsToIntCSSPixels(border->GetComputedBorder().left); > + aBorderTop = nsPresContext::AppUnitsToIntCSSPixels(border->GetComputedBorder().top); > + aMarginLeft = GetCSSFloatValue(margin->mMargin.GetLeft()); > + aMarginTop = GetCSSFloatValue(margin->mMargin.GetTop()); > > - aX = GetCSSFloatValue(cssDecl, NS_LITERAL_STRING("left")) + > + aX = GetCSSFloatValue(position->mOffset.GetLeft()) + Likewise here. ::: editor/libeditor/HTMLAnonymousNodeEditor.cpp:484 (Diff revision 1) > > - aX = GetCSSFloatValue(cssDecl, NS_LITERAL_STRING("left")) + > + aX = GetCSSFloatValue(position->mOffset.GetLeft()) + > aMarginLeft + aBorderLeft; > - aY = GetCSSFloatValue(cssDecl, NS_LITERAL_STRING("top")) + > + aY = GetCSSFloatValue(position->mOffset.GetTop()) + > aMarginTop + aBorderTop; > - aW = GetCSSFloatValue(cssDecl, NS_LITERAL_STRING("width")); > + aW = GetCSSFloatValue(position->mWidth); And here; used values vs computed. ::: layout/style/nsROCSSPrimitiveValue.cpp:563 (Diff revision 1) > - mType = CSS_IDENT; > + mType = CSSPrimitiveValueBinding::CSS_IDENT; > } > > // FIXME: CSS_STRING should imply a string with "" and a need for escaping. > void > +nsROCSSPrimitiveValue::SetString(const nsACString& aString) Why is a separate method preferable to an optional-and-defaulted argument?

Comment on attachment 8940139 [details] Bug 1427512 - Part 28: Remove nsIDOMCSSValue. https://reviewboard.mozilla.org/r/210460/#review216322

Comment on attachment 8940140 [details] Bug 1427512 - Part 30: Stop testing whether Ci shims should be removed. https://reviewboard.mozilla.org/r/210462/#review216324 I did not review the acessible/ or windowutils bits in detail, because I figure those need to change anyway. ::: accessible/windows/msaa/nsWinUtils.cpp:50 (Diff revision 1) > - nsCOMPtr<nsPIDOMWindowInner> window = elm->OwnerDoc()->GetInnerWindow(); > - if (!window) > return nullptr; > + } > > - ErrorResult dummy; > + return NS_NewComputedDOMStyle(elm->AsElement(), nsString(), EmptyString(), please. ::: accessible/windows/msaa/nsWinUtils.cpp:51 (Diff revision 1) > - if (!window) > return nullptr; > + } > > - ErrorResult dummy; > - nsCOMPtr<nsICSSDeclaration> cssDecl; > + return NS_NewComputedDOMStyle(elm->AsElement(), nsString(), > + elm->OwnerDoc()->GetShell()); This is not OK if GetShell() returns null; will crash... Can you just make this return `already_AddRefed<nsICSSDeclaration>` and more or less keep the old code? ::: devtools/server/actors/object.js:1805 (Diff revision 1) > return true; > }, > > function CSSStyleDeclaration({obj, hooks}, grip, rawObj) { > if (isWorker || !rawObj || > - !(rawObj instanceof Ci.nsIDOMCSSStyleDeclaration)) { > + obj.class != "CSSStyleDeclaration" && Please parenthesize the && bit, so it's clear what the logic is. ::: dom/base/nsDOMWindowUtils.cpp:2180 (Diff revision 1) > - ENSURE_SUCCESS(rv, rv.StealNSResult()); > - } > > - static_cast<nsComputedDOMStyle*>(decl.get())->SetExposeVisitedStyle(true); > - nsresult rv = decl->GetPropertyValue(aPropertyName, aResult); > - static_cast<nsComputedDOMStyle*>(decl.get())->SetExposeVisitedStyle(false); > + RefPtr<nsComputedDOMStyle> cs = > + NS_NewComputedDOMStyle(element, aPseudoElement, > + element->OwnerDoc()->GetShell()); Not ok if GetShell() is null. ::: layout/style/nsICSSDeclaration.h:87 (Diff revision 1) > NS_IMETHOD SetProperty(const nsAString& aPropertyName, > const nsAString& aValue, > const nsAString& aPriority, > - nsIPrincipal* aSubjectPrincipal = nullptr) override = 0; > - NS_IMETHOD GetLength(uint32_t* aLength) override = 0; > - NS_IMETHOD Item(uint32_t aIndex, nsAString& aReturn) override > + nsIPrincipal* aSubjectPrincipal = nullptr) = 0; > + NS_IMETHOD GetLength(uint32_t* aLength) = 0; > + NS_IMETHOD Item(uint32_t aIndex, nsAString& aReturn) Does this need to stay virtual? Followup to check which of these things can be devirtualized, please.

Regarding the instanceof checks, yes I had in my mind the fact that cross-global instanceof for Web IDL defined things will stop working at some point in the future. I was considering whether to use Object.prototype.toString.call() as an approximation, but then I'd need to explicitly check for all CSSRule subclasses, which wouldn't be very future proof. I could walk up the prototype chain manually... Or, as Anne suggests in https://github.com/heycam/webidl/pull/356#issuecomment-299097101, we could go ahead and add some chrome-only thing to use.

Comment on attachment 8940127 [details] Bug 1427512 - Part 16: Remove nsIDOMCSSRule. https://reviewboard.mozilla.org/r/210436/#review216248 > override final? Official style is to just use final. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC

(In reply to Cameron McCormack (:heycam) from comment #62) > Or, as Anne suggests in > https://github.com/heycam/webidl/pull/356#issuecomment-299097101, we could > go ahead and add some chrome-only thing to use. I filed bug 1428531 to look at doing that.

Comment on attachment 8940132 [details] Bug 1427512 - Part 21: Remove nsIDOMStyleSheetList. https://reviewboard.mozilla.org/r/210446/#review216284 > Why? This class doesn't actually implement nsIDocumentObserver in any meaningful way. Then we should really be inheriting from nsStubMutationObserver instead of nsStubDocumentObserver...

Comment on attachment 8940133 [details] Bug 1427512 - Part 22: Remove nsIDOMStyleSheet. https://reviewboard.mozilla.org/r/210448/#review216298 > Why is silently swallowing the error here ok? It's probably not (though was the old behavior). I'll make it propagate.

Comment on attachment 8940138 [details] Bug 1427512 - Part 27: Remove nsIDOMCSSPrimitiveValue. https://reviewboard.mozilla.org/r/210458/#review216308 > This doesn't necessarily match the old behavior. The old behavior returned _used_ borders when there was a frame. Is that change ok? That's a good point. I'm not sure that change is OK, so I'm going to go back to the old code but use concrete CSSValue / nsROCSSPrimitiveValue types instead of nsIDOMCSSValue / nsIDOMCSSPrimitiveValue.

Comment on attachment 8940138 [details] Bug 1427512 - Part 27: Remove nsIDOMCSSPrimitiveValue. https://reviewboard.mozilla.org/r/210458/#review216308 > Why is a separate method preferable to an optional-and-defaulted argument? Because I was working under the misapprehension that it would be troublesome to include CSSPrimitiveValueBinding.h in this header here, to use CSSPrimitiveValueBinding::CSS_STRING. But there's no such problem.

Comment on attachment 8940140 [details] Bug 1427512 - Part 30: Stop testing whether Ci shims should be removed. https://reviewboard.mozilla.org/r/210462/#review216324 > Does this need to stay virtual? Followup to check which of these things can be devirtualized, please. Filed bug 1428610. But you are right that Item() can be devirtualized and I'll do that here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f6f89e1b4299f3b96c7df2284eec60ab7fdb2c9 Emilio, do you know what's going wrong with bindgen on Windows in that try push?

(In reply to Cameron McCormack (:heycam) from comment #70) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=1f6f89e1b4299f3b96c7df2284eec60ab7fdb2c9 > > Emilio, do you know what's going wrong with bindgen on Windows in that try > push? Isn't that because you change this Windows-specific header file to reference a file doesn't exist in https://hg.mozilla.org/try/diff/96df36a50002/accessible/windows/msaa/nsWinUtils.h ?

Erk, thanks. Pushing again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfe88892f94b1a1f8bbbe4a93b02f55060a2b3d6

With that fixed, I still get the bindgen error: 13:19:36 INFO - error[E0243]: wrong number of type arguments: expected 1, found 0 13:19:36 INFO - --> z:/build/build/src/obj-firefox/toolkit/library\i686-pc-windows-msvc\release\build\style-a1e600532d2d6f07\out/gecko/structs.rs:1753:1195 13:19:36 INFO - | 13:19:36 INFO - 1753 | # [ repr ( C ) ] # [ derive ( Debug ) ] pub struct nsStyleContext { pub mPseudoTag : root :: RefPtr < root :: nsAtom > , pub mBits : u64 , } # [ test ] fn bindgen_test_layout_nsStyleContext ( ) { assert_eq ! ( :: std :: mem :: size_of :: < nsStyleContext > ( ) , 16usize , concat ! ( "Size of: " , stringify ! ( nsStyleContext ) ) ) ; assert_eq ! ( :: std :: mem :: align_of :: < nsStyleContext > ( ) , 8usize , concat ! ( "Alignment of " , stringify ! ( nsStyleContext ) ) ) ; assert_eq ! ( unsafe { & ( * ( 0 as * const nsStyleContext ) ) . mPseudoTag as * const _ as usize } , 0usize , concat ! ( "Alignment of field: " , stringify ! ( nsStyleContext ) , "::" , stringify ! ( mPseudoTag ) ) ) ; assert_eq ! ( unsafe { & ( * ( 0 as * const nsStyleContext ) ) . mBits as * const _ as usize } , 8usize , concat ! ( "Alignment of field: " , stringify ! ( nsStyleContext ) , "::" , stringify ! ( mBits ) ) ) ; } # [ repr ( C ) ] # [ derive ( Debug ) ] pub struct nsCSSCounterStyleRule { pub _base : root :: mozilla :: css :: Rule , pub mName : root :: RefPtr < root :: nsAtom > , pub mValues : [ root :: nsCSSValue ; 10usize ] , pub mGeneration : u32 , } pub type nsCSSCounterStyleRule_Getter = :: std :: option :: Option < > ; extern "C" { 13:19:36 INFO - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 1 type argument It's generating: pub type nsCSSCounterStyleRule_Getter = ::std::option::Option<>; and that type comes from: typedef decltype(&nsCSSCounterStyleRule::GetSymbols) Getter; The part 6 patch does change that method's declaration from NS_IMETHOD GetSymbols(nsAString & aSymbols) override; to void GetSymbols(nsAString& aSymbols); so I guess that confused it somehow.

(In reply to Cameron McCormack (:heycam) from comment #102) > With that fixed, I still get the bindgen error: > > 13:19:36 INFO - error[E0243]: wrong number of type arguments: expected > 1, found 0 > 13:19:36 INFO - --> > z:/build/build/src/obj-firefox/toolkit/library\i686-pc-windows- > msvc\release\build\style-a1e600532d2d6f07\out/gecko/structs.rs:1753:1195 > 13:19:36 INFO - | > 13:19:36 INFO - 1753 | # [ repr ( C ) ] # [ derive ( Debug ) ] pub > struct nsStyleContext { pub mPseudoTag : root :: RefPtr < root :: nsAtom > , > pub mBits : u64 , } # [ test ] fn bindgen_test_layout_nsStyleContext ( ) { > assert_eq ! ( :: std :: mem :: size_of :: < nsStyleContext > ( ) , 16usize , > concat ! ( "Size of: " , stringify ! ( nsStyleContext ) ) ) ; assert_eq ! ( > :: std :: mem :: align_of :: < nsStyleContext > ( ) , 8usize , concat ! ( > "Alignment of " , stringify ! ( nsStyleContext ) ) ) ; assert_eq ! ( unsafe > { & ( * ( 0 as * const nsStyleContext ) ) . mPseudoTag as * const _ as usize > } , 0usize , concat ! ( "Alignment of field: " , stringify ! ( > nsStyleContext ) , "::" , stringify ! ( mPseudoTag ) ) ) ; assert_eq ! ( > unsafe { & ( * ( 0 as * const nsStyleContext ) ) . mBits as * const _ as > usize } , 8usize , concat ! ( "Alignment of field: " , stringify ! ( > nsStyleContext ) , "::" , stringify ! ( mBits ) ) ) ; } # [ repr ( C ) ] # [ > derive ( Debug ) ] pub struct nsCSSCounterStyleRule { pub _base : root :: > mozilla :: css :: Rule , pub mName : root :: RefPtr < root :: nsAtom > , pub > mValues : [ root :: nsCSSValue ; 10usize ] , pub mGeneration : u32 , } pub > type nsCSSCounterStyleRule_Getter = :: std :: option :: Option < > ; extern > "C" { > 13:19:36 INFO - | > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 1 type argument > > > It's generating: > > pub type nsCSSCounterStyleRule_Getter = ::std::option::Option<>; > > and that type comes from: > > typedef decltype(&nsCSSCounterStyleRule::GetSymbols) Getter; > > The part 6 patch does change that method's declaration from > > NS_IMETHOD GetSymbols(nsAString & aSymbols) override; > > to > > void GetSymbols(nsAString& aSymbols); > > so I guess that confused it somehow. In windows some methods are "thiscall", which stable rustc doesn't support IIRC, and bindgen bails out to not generate it, maybe it's related to that... I don't have a windows machine, but it should be reproducible on linux with the windows -target. I can try to get it reduced if you don't win me at that :)

Cam, I'll get to this tomorrow, but if you want you can try to add NS_FASTCALL to that function and see if it changes.

It's due to thiscall, I filed https://github.com/rust-lang-nursery/rust-bindgen/issues/1219. There's a workaround there (annotating it with stdcall works as I suggested in comment 104).

Comment on attachment 8940112 [details] Bug 1427512 - Part 1: Remove nsIDOMCSSMozDocumentRule. https://reviewboard.mozilla.org/r/210406/#review216474

Comment on attachment 8940114 [details] Bug 1427512 - Part 3: Remove nsIDOMCSSMediaRule. https://reviewboard.mozilla.org/r/210410/#review216478

Comment on attachment 8940113 [details] Bug 1427512 - Part 2: Remove nsIDOMCSSSupportsRule. https://reviewboard.mozilla.org/r/210408/#review216476

Comment on attachment 8940115 [details] Bug 1427512 - Part 4: Remove nsIDOMCSSConditionRule. https://reviewboard.mozilla.org/r/210412/#review216480 r=me with nits addressed. ::: layout/style/ServoDocumentRule.h:34 (Diff revision 2) > - // nsIDOMCSSConditionRule interface > - NS_DECL_NSIDOMCSSCONDITIONRULE > + void GetConditionText(nsAString& aConditionText) override; > + void SetConditionText(const nsAString& aConditionText, > + ErrorResult& aRv) override; Probably move these under WebIDL interface? Also it seems to me it is probably better to mark `final` instead of `override` here? ::: layout/style/ServoDocumentRule.h:40 (Diff revision 2) > - NS_DECL_NSIDOMCSSCONDITIONRULE > + void SetConditionText(const nsAString& aConditionText, > + ErrorResult& aRv) override; > > // WebIDL interface > void GetCssTextImpl(nsAString& aCssText) const override; > using CSSMozDocumentRule::SetConditionText; I suppose this `using` is no longer necessary. I think this was needed because we have XPCOM and WebIDL overload of the same method, and they are implemented at different inheritance level. But now that XPCOM part has gone, we shouldn't need it anymore. ::: layout/style/ServoMediaRule.h:38 (Diff revision 2) > - // nsIDOMCSSConditionRule interface > - NS_DECL_NSIDOMCSSCONDITIONRULE > + void GetConditionText(nsAString& aConditionText) override; > + void SetConditionText(const nsAString& aConditionText, > + ErrorResult& aRv) override; Same here, move to WebIDL interface. ::: layout/style/ServoMediaRule.h:44 (Diff revision 2) > - NS_DECL_NSIDOMCSSCONDITIONRULE > + void SetConditionText(const nsAString& aConditionText, > + ErrorResult& aRv) override; > > // WebIDL interface > void GetCssTextImpl(nsAString& aCssText) const override; > using CSSMediaRule::SetConditionText; Same here, seems no longer needed. ::: layout/style/ServoSupportsRule.h:34 (Diff revision 2) > - // nsIDOMCSSConditionRule interface > - NS_DECL_NSIDOMCSSCONDITIONRULE > + void GetConditionText(nsAString& aConditionText) override; > + void SetConditionText(const nsAString& aConditionText, > + ErrorResult& aRv) override; Ditto. ::: layout/style/ServoSupportsRule.h:40 (Diff revision 2) > - NS_DECL_NSIDOMCSSCONDITIONRULE > + void SetConditionText(const nsAString& aConditionText, > + ErrorResult& aRv) override; > > // WebIDL interface > void GetCssTextImpl(nsAString& aCssText) const override; > using CSSSupportsRule::SetConditionText; Ditto. ::: layout/style/nsCSSRules.h:76 (Diff revision 2) > + void GetConditionText(nsAString& aConditionText) override; > + void SetConditionText(const nsAString& aConditionText, > + ErrorResult& aRv) override; Ditto. ::: layout/style/nsCSSRules.h:85 (Diff revision 2) > // @media rule methods > nsresult SetMedia(nsMediaList* aMedia); > > // WebIDL interface > void GetCssTextImpl(nsAString& aCssText) const override; > using CSSMediaRule::SetConditionText; Ditto. ::: layout/style/nsCSSRules.h:120 (Diff revision 2) > + void GetConditionText(nsAString& aConditionText) override; > + void SetConditionText(const nsAString& aConditionText, > + ErrorResult& aRv) override; Ditto. ::: layout/style/nsCSSRules.h:143 (Diff revision 2) > > void SetURLs(URL *aURLs) { mURLs = aURLs; } > > // WebIDL interface > void GetCssTextImpl(nsAString& aCssText) const override; > using dom::CSSMozDocumentRule::SetConditionText; Ditto. ::: layout/style/nsCSSRules.h:426 (Diff revision 2) > - // nsIDOMCSSConditionRule interface > - NS_DECL_NSIDOMCSSCONDITIONRULE > + void GetConditionText(nsAString& aConditionText) override; > + void SetConditionText(const nsAString& aConditionText, > + ErrorResult& aRv) override; Ditto. ::: layout/style/nsCSSRules.h:432 (Diff revision 2) > - NS_DECL_NSIDOMCSSCONDITIONRULE > + void SetConditionText(const nsAString& aConditionText, > + ErrorResult& aRv) override; > > // WebIDL interface > void GetCssTextImpl(nsAString& aCssText) const override; > using dom::CSSSupportsRule::SetConditionText; Ditto.

Comment on attachment 8940116 [details] Bug 1427512 - Part 5: Remove nsIDOMCSSGroupingRule. https://reviewboard.mozilla.org/r/210414/#review216482 r=me with nits addressed. ::: layout/style/CSSMediaRule.cpp:15 (Diff revision 2) > #include "mozilla/dom/MediaList.h" > > namespace mozilla { > namespace dom { > > -NS_IMPL_ADDREF_INHERITED(CSSMediaRule, css::ConditionRule) > +NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0(CSSMediaRule, css::ConditionRule) Since GroupRule is a cycle collection class which provides the CC participant, I suppose it is not necessary to mention CYCLE_COLLECTION here again, `NS_IMPL_ISUPPORTS_INHERITED0` should be enough. The only difference is that `NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION` adds an additional branch for cycle collection which would be done anyway in GroupRule. It may be a straightforward transformation in this file, but the latter files don't mention CYCLE_COLLECTION in their INTERFACE_MAP, which made me wonder. (Actually I suspect we no longer need these boilerplate of nsISupports anymore after we remove all direct inheritance from XPIDL interface on a class. Probably we still want the addref and release impl for the refcount log, though.) ::: layout/style/CSSMozDocumentRule.cpp:15 (Diff revision 2) > namespace mozilla { > namespace dom { > > using namespace mozilla::css; > > -NS_IMPL_ADDREF_INHERITED(CSSMozDocumentRule, css::ConditionRule) > +NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0(CSSMozDocumentRule, css::ConditionRule) Ditto. ::: layout/style/CSSSupportsRule.cpp:16 (Diff revision 2) > using namespace mozilla::css; > > namespace mozilla { > namespace dom { > > -NS_IMPL_ADDREF_INHERITED(mozilla::dom::CSSSupportsRule, css::ConditionRule) > +NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0(CSSSupportsRule, css::ConditionRule) Ditto.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #103) > In windows some methods are "thiscall", which stable rustc doesn't support > IIRC, and bindgen bails out to not generate it, maybe it's related to that... > > I don't have a windows machine, but it should be reproducible on linux with > the windows -target. Wasn't this a problem we have worked around long ago for win32 in bug 1365254? Why does it happen again with such a straightforward refactor :/

I think the simple solution is to just blacklist nsCSSCounterStyleRule::Getter in toml. We don't need those functions in Rust side anyway.

Comment on attachment 8940117 [details] Bug 1427512 - Part 6: Remove nsIDOMCSSCounterStyleRule. https://reviewboard.mozilla.org/r/210416/#review216484 ::: layout/style/nsCSSCounterStyleRule.cpp:49 (Diff revision 2) > // QueryInterface implementation for nsCSSCounterStyleRule > // If this ever gets its own cycle-collection bits, reevaluate our IsCCLeaf > // implementation. > -NS_INTERFACE_MAP_BEGIN(nsCSSCounterStyleRule) > +NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0(nsCSSCounterStyleRule, css::Rule) Like part 5, given it doesn't provide its own cycle collection participant, it is probably unnecessary to mention CYCLE_COLLECTION here.

Comment on attachment 8940118 [details] Bug 1427512 - Part 7: Remove nsIDOMCSSFontFaceRule. https://reviewboard.mozilla.org/r/210418/#review216486

Comment on attachment 8940119 [details] Bug 1427512 - Part 8: Remove nsIDOMCSSFontFeatureValuesRule. https://reviewboard.mozilla.org/r/210420/#review216488 ::: layout/style/CSSFontFeatureValuesRule.cpp:13 (Diff revision 2) > // QueryInterface implementation for CSSFontFeatureValuesRule > // If this ever gets its own cycle-collection bits, reevaluate our IsCCLeaf > // implementation. > -NS_INTERFACE_MAP_BEGIN(CSSFontFeatureValuesRule) > +NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0(CSSFontFeatureValuesRule, css::Rule) No own cycle collection bit, no CYCLE_COLLECTION need here. Actually, since this is an abstract class, I guess we really don't need anything here. The AddRef/Release log is probably better living in leaf classes. But in long term, maybe we'll merge the two classes, so probably it's fine...

Comment on attachment 8940120 [details] Bug 1427512 - Part 9: Remove nsIDOMCSSImportRule. https://reviewboard.mozilla.org/r/210422/#review216490

Comment on attachment 8940121 [details] Bug 1427512 - Part 10: Remove nsIDOMCSSKeyframesRule. https://reviewboard.mozilla.org/r/210424/#review216492 ::: layout/style/nsCSSRules.h (Diff revision 2) > } > private: > nsCSSKeyframesRule(const nsCSSKeyframesRule& aCopy); > ~nsCSSKeyframesRule(); > public: > - NS_DECL_ISUPPORTS_INHERITED You probably should leave the nsISupports bits as-is for now at least for the AddRef/Release log... although maybe it doesn't matter since the class is going away in foreseeable future...

Comment on attachment 8940122 [details] Bug 1427512 - Part 11: Remove nsIDOMCSSKeyframeRule. https://reviewboard.mozilla.org/r/210426/#review216494

Comment on attachment 8940123 [details] Bug 1427512 - Part 12: Remove nsIDOMCSSPageRule. https://reviewboard.mozilla.org/r/210428/#review216496

Comment on attachment 8940124 [details] Bug 1427512 - Part 13: Remove nsIDOMCSSUnknownRule. https://reviewboard.mozilla.org/r/210430/#review216498

Comment on attachment 8940125 [details] Bug 1427512 - Part 14: Remove mention of non-existent nsIDOMCSSStyleRuleCollection. https://reviewboard.mozilla.org/r/210432/#review216500

Comment on attachment 8940126 [details] Bug 1427512 - Part 15: Remove nsIDOMCSSStyleRule. https://reviewboard.mozilla.org/r/210434/#review216502

Comment on attachment 8940139 [details] Bug 1427512 - Part 28: Remove nsIDOMCSSValue. https://reviewboard.mozilla.org/r/210460/#review216506 Thanks, /devtools changes look good!

Comment on attachment 8940127 [details] Bug 1427512 - Part 16: Remove nsIDOMCSSRule. https://reviewboard.mozilla.org/r/210436/#review216504 ::: js/xpconnect/src/Sandbox.cpp:973 (Diff revision 2) > + if (CSSRule && !dom::CSSRuleBinding::GetConstructorObject(cx)) > + return false; I guess this is fine... but I really don't think I'm qualified to review js code, so you probably want bz to cover the changes in js/xpconnect (I don't see you mentioned this part in comment 36, so chances are that bz might have overlooked this part). ::: layout/style/CSSImportRule.h:11 (Diff revision 2) > > #ifndef mozilla_dom_CSSImportRule_h > #define mozilla_dom_CSSImportRule_h > > #include "mozilla/css/Rule.h" > +#include "mozilla/dom/CSSRuleBinding.h" What about just reference this file in Rule.h so that you don't need to reference it over and over again in each rule header? ::: layout/style/CSSImportRule.h:26 (Diff revision 2) > > public: > bool IsCCLeaf() const final; > > int32_t GetType() const final { return css::Rule::IMPORT_RULE; } > using Rule::GetType; You can now remove this. ::: layout/style/CSSKeyframeRule.h:24 (Diff revision 2) > using css::Rule::Rule; > virtual ~CSSKeyframeRule() {} > > public: > int32_t GetType() const final { return Rule::KEYFRAME_RULE; } > using Rule::GetType; And this. ::: layout/style/CSSKeyframesRule.h:25 (Diff revision 2) > using css::GroupRule::GroupRule; > virtual ~CSSKeyframesRule() {} > > public: > int32_t GetType() const final { return Rule::KEYFRAMES_RULE; } > using Rule::GetType; And this. ::: layout/style/CSSMediaRule.h:28 (Diff revision 2) > NS_DECL_ISUPPORTS_INHERITED > > int32_t GetType() const override { return css::Rule::MEDIA_RULE; } > > // XPCOM interface > using Rule::GetType; And this. ::: layout/style/CSSMozDocumentRule.h:27 (Diff revision 2) > > public: > NS_DECL_ISUPPORTS_INHERITED > > int32_t GetType() const final override { return css::Rule::DOCUMENT_RULE; } > using Rule::GetType; And this. ::: layout/style/CSSNamespaceRule.h:31 (Diff revision 2) > return Rule::IsCCLeaf(); > } > int32_t GetType() const final { > return Rule::NAMESPACE_RULE; > } > using Rule::GetType; And this. ::: layout/style/CSSPageRule.h:29 (Diff revision 2) > > public: > virtual bool IsCCLeaf() const override = 0; > > int32_t GetType() const final { return Rule::PAGE_RULE; } > using Rule::GetType; And this. ::: layout/style/CSSSupportsRule.h:26 (Diff revision 2) > > public: > NS_DECL_ISUPPORTS_INHERITED > > int32_t GetType() const override { return css::Rule::SUPPORTS_RULE; } > using Rule::GetType; And this. ::: layout/style/ServoKeyframeRule.cpp:32 (Diff revision 2) > > NS_DECL_CYCLE_COLLECTING_ISUPPORTS > NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_AMBIGUOUS( > ServoKeyframeDeclaration, nsICSSDeclaration) > > - NS_IMETHOD GetParentRule(nsIDOMCSSRule** aParent) final > + css::Rule* GetParentRule() override { return mRule; } Why not keep using `final` here? ::: layout/style/ServoStyleRule.h:34 (Diff revision 2) > class ServoStyleRuleDeclaration final : public nsDOMCSSDeclaration > { > public: > NS_DECL_ISUPPORTS_INHERITED > > - NS_IMETHOD GetParentRule(nsIDOMCSSRule** aParent) final; > + css::Rule* GetParentRule() override; Why not keep using `final` here? ::: layout/style/StyleRule.cpp:1066 (Diff revision 2) > friend class mozilla::DefaultDelete<DOMCSSDeclarationImpl>; > > public: > explicit DOMCSSDeclarationImpl(css::StyleRule *aRule); > > - NS_IMETHOD GetParentRule(nsIDOMCSSRule **aParent) override; > + css::Rule* GetParentRule() override { return mRule; } You can probably change this to `final` rather than `override`.

Comment on attachment 8940128 [details] Bug 1427512 - Part 17: Remove nsICSSStyleRuleDOMWrapper. https://reviewboard.mozilla.org/r/210438/#review216552

Comment on attachment 8940129 [details] Bug 1427512 - Part 18: Remove nsIDOMCSSStyleSheet. https://reviewboard.mozilla.org/r/210440/#review216554

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #111) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #103) > > In windows some methods are "thiscall", which stable rustc doesn't support > > IIRC, and bindgen bails out to not generate it, maybe it's related to that... > > > > I don't have a windows machine, but it should be reproducible on linux with > > the windows -target. > > Wasn't this a problem we have worked around long ago for win32 in bug > 1365254? Why does it happen again with such a straightforward refactor :/ Well, yes, we worked around the bug relying on us not doing code generation for those functions. This patch changes the second bit...

Comment on attachment 8940127 [details] Bug 1427512 - Part 16: Remove nsIDOMCSSRule. https://reviewboard.mozilla.org/r/210436/#review216248 > Official style is to just use final. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC Hmm. I guess that sort of works, as long as you make sure to _not_ use "virtual" when you use "final". That seems really fragile to me (and the compiler messages you get are a bit confusing when you're not overriding a parent class function, but I guess we ended up with that being the official guide...

Comment on attachment 8940132 [details] Bug 1427512 - Part 21: Remove nsIDOMStyleSheetList. https://reviewboard.mozilla.org/r/210446/#review216284 > Then we should really be inheriting from nsStubMutationObserver instead of nsStubDocumentObserver... Yes, we should.

Comment on attachment 8940133 [details] Bug 1427512 - Part 22: Remove nsIDOMStyleSheet. https://reviewboard.mozilla.org/r/210448/#review216298 > It's probably not (though was the old behavior). I'll make it propagate. The old behavior did not swallow the error. It returned it (see the NS_ENSURE_SUCCESS(rv, rv) bit). Now the binding caller then swallowed it, which is arguably a bug or incomplete fix that was introduced in bug 1297300 (before that GetSpec failure was in fact ignored; that bug added the early return on GetSpec failure but didn't fix bindings accordingly).

> I was considering whether to use Object.prototype.toString.call() ChromeUtils.getClassName is faster and clearer. But as you note (and this was pointed out in the platform thread too), it's a problem when testing for a non-leaf interface... Let's follow up on this part in bug 1428531.

> (I don't see you mentioned this part in comment 36, so chances are that bz might have overlooked this part) I didn't actually notice comment 36, so I just reviewed everything. I definitely reviewed the xpconnect changes carefully, because I figured no one else was reviewing them. ;)

Comment on attachment 8940130 [details] Bug 1427512 - Part 19: Remove nsIDOMCSSRuleList. https://reviewboard.mozilla.org/r/210442/#review216920

Comment on attachment 8940131 [details] Bug 1427512 - Part 20: Remove nsIDOMMediaList. https://reviewboard.mozilla.org/r/210444/#review216922

Comment on attachment 8940132 [details] Bug 1427512 - Part 21: Remove nsIDOMStyleSheetList. https://reviewboard.mozilla.org/r/210446/#review216928

Comment on attachment 8940138 [details] Bug 1427512 - Part 27: Remove nsIDOMCSSPrimitiveValue. https://reviewboard.mozilla.org/r/210458/#review216932 r=me ::: editor/libeditor/HTMLAnonymousNodeEditor.cpp:79 (Diff revision 2) > - rv = val->GetFloatValue(nsIDOMCSSPrimitiveValue::CSS_PX, &f); > - NS_ENSURE_SUCCESS(rv, 0); > + f = val->GetFloatValue(CSSPrimitiveValueBinding::CSS_PX, rv); > + if (rv.Failed()) { > + return 0; > + } > break; > - case nsIDOMCSSPrimitiveValue::CSS_IDENT: { > + case CSSPrimitiveValueBinding::CSS_IDENT: { I'm 99% sure this CSS_IDENT case can't happen for the specific properties that get queried through here. In particular, computed style never returns "thin"/"medium"/"thick" for those border values; they get mapped to px before that code is ever reached. Please file a followup bug to investigate and remove this case if that's true.

Comment on attachment 8940133 [details] Bug 1427512 - Part 22: Remove nsIDOMStyleSheet. https://reviewboard.mozilla.org/r/210448/#review216934

Comment on attachment 8940140 [details] Bug 1427512 - Part 30: Stop testing whether Ci shims should be removed. https://reviewboard.mozilla.org/r/210462/#review216936 Boy, mozreview is confused. r=me on the "new" part 30, but my review- was on the "old" part 30, which was a totally different patch. I don't see an updated version of that patch, offhand...

Comment on attachment 8940134 [details] Bug 1427512 - Part 23: Remove nsIDOMCounter. https://reviewboard.mozilla.org/r/210450/#review216938 ::: dom/webidl/CSSPrimitiveValue.webidl:49 (Diff revision 2) > void setStringValue(unsigned short stringType, > DOMString stringValue); > [Throws] > DOMString getStringValue(); > [Throws] > - Counter getCounterValue(); > + void getCounterValue(); // always throws Can we simply remove it? It seems to me `CSSPrimitiveValue` is not part of any standard nowadays (it was only part of obsolete DOM 2, but not in the living DOM spec), and we are the only browser having it. Also apparently we don't have internal use for it. So I suppose we can simply remove this method as well as all related code.

Comment on attachment 8940136 [details] Bug 1427512 - Part 25: Remove nsIDOMRect. https://reviewboard.mozilla.org/r/210454/#review216940

Comment on attachment 8940137 [details] Bug 1427512 - Part 26: Remove nsIDOMCSSValueList. https://reviewboard.mozilla.org/r/210456/#review216942

Just so you're aware, mozreview is showing r+ from me on a patch I _explicitly_ marked r- in my previous review round (what is now labeled as "part 29", which mozreview seems to think got changed to that from the "part 28" that I _did_ mark r+ on). I haven't carefully sorted through the state of the various patches and which ones I've OKed and which ones I haven't, but the state currently marked in both mozreview and bugzilla does NOT match reality afaict. It looks like what's not "part 30" got mutated from the r- old "part 29"? Anyway, I still need to re-review the thing I marked r- on there, whatever that is. Unfortunately, I no longer trust mozreview for either tracking that or giving me useful interdiffs, given what the output I'm seeing is. Note that there is no longer a "part 28" in the mozreview patch queue, in case that matters....

It's possible that comment 142 is because mozreview commit ids got modified in the patches when they shouldn't have been? Certainly https://reviewboard.mozilla.org/r/210460/diff/1-2/ shows the commit id changing to "JHpZnyfN7Wi" for the "Remove nsIDOMCSSDeclaration" patch, but https://reviewboard.mozilla.org/r/210462/diff/1-2/ says that patch used to have id "6DasjoMa6C8"...

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #143) > It's possible that comment 142 is because mozreview commit ids got modified > in the patches when they shouldn't have been? Certainly > https://reviewboard.mozilla.org/r/210460/diff/1-2/ shows the commit id > changing to "JHpZnyfN7Wi" for the "Remove nsIDOMCSSDeclaration" patch, but > https://reviewboard.mozilla.org/r/210462/diff/1-2/ says that patch used to > have id "6DasjoMa6C8"... That might be the fault of the git extension heycam might be using. It seems to me that the "MozReview-Commit-ID" field changes among update to patches, which means MozReview is not able to match the previous patch with new patch in any sense. (In hg, there is obsolescence marker may be used for this, but there is no correspondence for git, and I'm not even sure whether that works nowadays given the Commit-ID thing.) I guess heycam should probably check whether he has the recommended setting for the vcs.

Comment on attachment 8940138 [details] Bug 1427512 - Part 27: Remove nsIDOMCSSPrimitiveValue. https://reviewboard.mozilla.org/r/210458/#review216950

So I think what happened is the old part 28 (see <https://reviewboard.mozilla.org/r/210460/diff/1/> and look at the commit message, not the title) got merged into part 27 (compare that diff to <https://reviewboard.mozilla.org/r/210458/diff/1-2/>). Combined with the nonconstant mozreview commit ids, that made mozreview think what used to be part 29 is now part 28, etc, _and carry over review flags to totally unrelated changesets_... Cameron, it might be possible to get useful interdiffs out of mozreview here if you either unmerge the old parts 27/28 or change the mozreview id on the old part 29 to match what it used to be, so mozreview will stick it in the right place in the list.

Comment on attachment 8940139 [details] Bug 1427512 - Part 28: Remove nsIDOMCSSValue. https://reviewboard.mozilla.org/r/210460/#review216962

The merging of parts 27 and 28 was unintentional. I will de-merge, and edit the MozReview-Commit-IDs for the last few patches to match those used in the revision 1 patches. I have no idea why the part 27 and part 29 commit IDs changed. And why MozReview thinks it can match up the new part 30 patch with the old (removed) part 28 patch, despite having different commit messages and commits IDs...

Comment on attachment 8940134 [details] Bug 1427512 - Part 23: Remove nsIDOMCounter. https://reviewboard.mozilla.org/r/210450/#review216938 > Can we simply remove it? It seems to me `CSSPrimitiveValue` is not part of any standard nowadays (it was only part of obsolete DOM 2, but not in the living DOM spec), and we are the only browser having it. Also apparently we don't have internal use for it. > > So I suppose we can simply remove this method as well as all related code. It probably is fine to remove that method; it's pretty unlikely anyone is relying on its presence. But note that WebKit also does implement it. I think I'd rather remove all of CSSPrimitiveValue etc. when it's safe to do so.

Comment on attachment 8940138 [details] Bug 1427512 - Part 27: Remove nsIDOMCSSPrimitiveValue. https://reviewboard.mozilla.org/r/210458/#review216932 > I'm 99% sure this CSS_IDENT case can't happen for the specific properties that get queried through here. In particular, computed style never returns "thin"/"medium"/"thick" for those border values; they get mapped to px before that code is ever reached. Please file a followup bug to investigate and remove this case if that's true. You're right. nsComputedDOMStyle::GetBorderWidthFor only ever creates a length-typed CSSValue. Filed bug 1428974.

I created this by pulling https://reviewboard-hg.mozilla.org/gecko/rev/9bbfdc4c73a5 and https://reviewboard-hg.mozilla.org/gecko/rev/96df36a50002e9f15e92addd8312cb4160bcd10d into my tree and then doing: $ hg up -C -r 9bbfdc4c73a5^ $ hg export 9bbfdc4c73a5 | hg qimport -n test - $ hg qpush $ hg qdiff | patch -p1 -R $ hg export 96df36a50002e9f15e92addd8312cb4160bcd10d | patch -p1 $ hg diff

Comment on attachment 8940139 [details] Bug 1427512 - Part 28: Remove nsIDOMCSSValue. https://reviewboard.mozilla.org/r/210460/#review217316 r=me for the "Part 29: Remove nsIDOMCSSDeclaration" bit based on this diff and the interdiff at https://bug1427512.bmoattachments.org/attachment.cgi?id=8941243