Closed Bug 1435666 Opened 2 years ago Closed 2 years ago

remove some SVG- and XPath-related XPCOM interfaces

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(4 files)

No description provided.
:frg, please note.  Like bug 1427512, this will at least need the package manifest updates to be ported across, and looking at comm-central there are some uses of constants on nsIDOMXPathResult that will need to be updated to either XPathResult.BLAH or using hard coded constant values.
Flags: needinfo?(frgrahl)
Or how about for calendar calendar/base/modules/calXMLUtils.jsm
Instead of
  const XPR = Components.interfaces.nsIDOMXPathResult;
we do:
const XPR = {
{
  // XPathResultType
  ANY_TYPE: 0,
  NUMBER_TYPE: 1,
  STRING_TYPE: 2,
  BOOLEAN_TYPE: 3,
  UNORDERED_NODE_ITERATOR_TYPE: 4,
  ORDERED_NODE_ITERATOR_TYPE: 5,
  UNORDERED_NODE_SNAPSHOT_TYPE: 6,
  ORDERED_NODE_SNAPSHOT_TYPE: 7,
  ANY_UNORDERED_NODE_TYPE: 8,
  FIRST_ORDERED_NODE_TYPE: 9
};
Thanks for the headup.
Flags: needinfo?(frgrahl)
Comment on attachment 8948315 [details]
Bug 1435666 - Part 1: Remove nsIDOMSVGElement.

https://reviewboard.mozilla.org/r/217792/#review223694

This is awesome.  r=me

::: dom/svg/SVGTests.cpp:33
(Diff revision 1)
>  }
>  
>  already_AddRefed<DOMSVGStringList>
>  SVGTests::RequiredFeatures()
>  {
> -  nsCOMPtr<nsIDOMSVGElement> elem = do_QueryInterface(this);
> +  nsCOMPtr<nsIDOMElement> elem = do_QueryInterface(this);

How about we QI to Element, then assert IsSVGElement() before doing the cast?  Would make me a little happier about the casting situation here.

Same elsewhere in this file.

Alternatly, we could just have a pure virtual AsSVGElement() on SVGTests and implement it in the two subclasses (returning `this`), and skip the QI bits.
Attachment #8948315 - Flags: review?(bzbarsky) → review+
Comment on attachment 8948315 [details]
Bug 1435666 - Part 1: Remove nsIDOMSVGElement.

https://reviewboard.mozilla.org/r/217792/#review223704

Actually, one more thing.  You need to fix mobile/android/installer/package-manifest.in too to remove dom_svg.xpt
Comment on attachment 8948316 [details]
Bug 1435666 - Part 2: Remove nsIDOMTimeEvent.

https://reviewboard.mozilla.org/r/217794/#review223706

Again, need to update mobile/android/installer/package-manifest.in.

::: dom/smil/nsSMILTimeValueSpec.cpp:375
(Diff revision 1)
>  }
>  
>  bool
>  nsSMILTimeValueSpec::CheckRepeatEventDetail(nsIDOMEvent *aEvent)
>  {
> -  nsCOMPtr<nsIDOMTimeEvent> timeEvent = do_QueryInterface(aEvent);
> +  TimeEvent* timeEvent = Event::FromSupports(aEvent)->AsTimeEvent();

I'd prefer you use aEvent->InternalDOMEvent() here, not Event::FromSupports.
Attachment #8948316 - Flags: review?(bzbarsky) → review+
Comment on attachment 8948317 [details]
Bug 1435666 - Part 3: Remove nsIDOMXPathResult.

https://reviewboard.mozilla.org/r/217796/#review223716

::: devtools/client/webconsole/webconsole.js:986
(Diff revision 1)
>      // "filtered-by-type" class, which turns on or off the display.
>  
>      let attribute = !WORKERTYPES_PREFKEYS.includes(prefKey)
>                        ? "filter" : "workerType";
>  
> -    let xpath = ".//*[contains(@class, 'message') and " +
> +    let selector = "[" + attribute + "='" + prefKey + "'].message";

I guess we hope the attribute names and pref keys don't have any weird chars in them... but we had to hope that before too.

::: dom/base/test/unit/test_range.js:7
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  const C_i = Components.interfaces;
>  
> -const UNORDERED_TYPE = C_i.nsIDOMXPathResult.ANY_UNORDERED_NODE_TYPE;
> +const UNORDERED_TYPE = 8; // XPathResult.ANY_UNORDERED_NODE_TYPE

The other option is to expose XPathResult to importGlobalProperties.  followup is fine for that.
Attachment #8948317 - Flags: review?(bzbarsky) → review+
Comment on attachment 8948317 [details]
Bug 1435666 - Part 3: Remove nsIDOMXPathResult.

https://reviewboard.mozilla.org/r/217796/#review223720

::: dom/base/test/unit/test_range.js
(Diff revision 1)
>    return do_parse_document(aPath, "application/xml").then(processParsedDocument);
>  }
>  
>  function processParsedDocument(doc) {
>    Assert.ok(doc.documentElement.localName != "parsererror");
> -  Assert.ok(doc instanceof C_i.nsIDOMXPathEvaluator);

This should be in the next changeset, right?
Comment on attachment 8948318 [details]
Bug 1435666 - Part 4: Remove nsIDOMXPathEvaluator.

https://reviewboard.mozilla.org/r/217798/#review223722

Again, update mobile/android/installer/package-manifest.in
Attachment #8948318 - Flags: review?(bzbarsky) → review+
Priority: -- → P2
Comment on attachment 8948317 [details]
Bug 1435666 - Part 3: Remove nsIDOMXPathResult.

https://reviewboard.mozilla.org/r/217796/#review223716

> I guess we hope the attribute names and pref keys don't have any weird chars in them... but we had to hope that before too.

Yeah.  In theory, the set of weird chars we'd behave unexpectedly on is different between XPath expressions and Selectors too, but hopefully it doesn't matter.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12)
> The other option is to expose XPathResult to importGlobalProperties. 
> followup is fine for that.

Filed bug 1436254.
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s efc5abe5c3956428b56f7958774bbb1c7f3519e1 -d fe90c7d53a7c: rebasing 446133:efc5abe5c395 "Bug 1435666 - Part 1: Remove nsIDOMSVGElement. r=bz"
rebasing 446134:3421cf6041de "Bug 1435666 - Part 2: Remove nsIDOMTimeEvent. r=bz"
rebasing 446135:420f16d6faf1 "Bug 1435666 - Part 3: Remove nsIDOMXPathResult. r=bz"
merging browser/components/sessionstore/test/content-forms.js
merging mobile/android/tests/browser/chrome/test_session_form_data.html
merging testing/marionette/doc/api/element.js.html
merging testing/marionette/element.js
merging toolkit/modules/sessionstore/FormData.jsm
rebasing 446136:fafee28d9e50 "Bug 1435666 - Part 4: Remove nsIDOMXPathEvaluator. r=bz" (tip)
merging dom/base/nsDocument.cpp
merging dom/base/nsIDocument.h
merging layout/build/nsLayoutModule.cpp
warning: conflicts while merging layout/build/nsLayoutModule.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Keywords: checkin-needed
Pushed by cam@mcc.id.au:
https://hg.mozilla.org/integration/autoland/rev/5e7d8c3b3f91
Part 1: Remove nsIDOMSVGElement. r=bz
https://hg.mozilla.org/integration/autoland/rev/8c1af668dd35
Part 2: Remove nsIDOMTimeEvent. r=bz
https://hg.mozilla.org/integration/autoland/rev/5a15c3d2e334
Part 3: Remove nsIDOMXPathResult. r=bz
https://hg.mozilla.org/integration/autoland/rev/be94b038da50
Part 4: Remove nsIDOMXPathEvaluator. r=bz
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/48b59ff51dca
Port bug 1435666 to TB/IB/SM: remove dom_svg.xpt, dom_smil.xpt, dom_xpath.xpt from package manifests. rs=bustage-fix
You need to log in before you can comment on or make changes to this bug.