Closed Bug 1427419 Opened 6 years ago Closed 6 years ago

convert inIDOMUtils to Web IDL

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

(Keywords: dev-doc-needed)

Attachments

(27 files, 1 obsolete file)

59 bytes, text/x-review-board-request
bzbarsky
: review+
tromey
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Let's convert inIDOMUtils to a Web IDL interface so that we don't have to deal with DOM-related XPIDL interfaces.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12ababb881f09d4969345a68e604ae7d9a96dca3

The layout/inspector/tests/chrome/test_parseStyleSheetObservers.html failure in the TV job seems to be because that test doesn't pass when it's run by itself (verified by pushing a try run with some white space only change to that file).
Assignee: nobody → cam
Status: NEW → ASSIGNED
Boris, let me know if folding most of these patches down would be easier to review.  Probably someone else could have reviewed these, but since nearly all of them also need a DOM peer's review, and each patch isn't too complicated, seemed easiest just to give them to you.
Also, please let me know if there's a way to avoid the GlobalObject& argument in all these static methods, since none of them need to use it.
(In reply to Cameron McCormack (:heycam) (away until 3 Jan) from comment #30)
> Created attachment 8939224 [details]
> Bug 1427419 - Part 28: Remove inIDOMUtils.
> 
> And rename inDOMUtils.cpp to InspectorUtils.cpp.

Hmm, my git-cinnabar checkout doesn't seem to have recorded this as a rename plus changes, unfortunately. :(
Blocks: 1427512
Comment on attachment 8939197 [details]
Bug 1427419 - Part 1: Add an InspectorUtils chrome-only IDL namespace.

https://reviewboard.mozilla.org/r/209602/#review215464

Thanks for the patch.  The devtools parts look good to me.  I did not really examine the other parts.
Attachment #8939197 - Flags: review?(ttromey) → review+
> test_parseStyleSheetObservers.html failure in the TV job seems to be because that test doesn't pass
> when it's run by itself

Could you file a followup bug on that?  Seems nasty.
Flags: needinfo?(cam)
Comment on attachment 8939197 [details]
Bug 1427419 - Part 1: Add an InspectorUtils chrome-only IDL namespace.

https://reviewboard.mozilla.org/r/209602/#review215498
Attachment #8939197 - Flags: review?(bzbarsky) → review+
> Also, please let me know if there's a way to avoid the GlobalObject& argument in all these static methods

Not at the moment.
Comment on attachment 8939198 [details]
Bug 1427419 - Part 2: Move nsIDOMUtils.getAllStyleSheets to InspectorUtils.

https://reviewboard.mozilla.org/r/209604/#review215500
Attachment #8939198 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939199 [details]
Bug 1427419 - Part 3: Move inIDOMUtils.getCSSStyleRules to InspectorUtils.

https://reviewboard.mozilla.org/r/209606/#review215504

::: dom/webidl/InspectorUtils.webidl:13
(Diff revision 1)
>   * A collection of utility methods for use by devtools.
>   */
>  [ChromeOnly]
>  namespace InspectorUtils {
>    sequence<StyleSheet> getAllStyleSheets(Document document);
> +  sequence<CSSRule> getCSSStyleRules(Element element,

If you make that last arg `[TreatNullAs=EmptyString]`, I am guessing you can remove the `|| ""` bits you had to add in a few cases.  Might be nicer that way.

::: layout/inspector/inDOMUtils.cpp:254
(Diff revision 1)
> -          rules->AppendElement(owningRule);
> +          aResult.AppendElement(owningRule);
>          }
>        }
>      }
>    } else {
> -    nsIDocument* doc = element->GetOwnerDocument();
> +    nsIDocument* doc = aElement.GetOwnerDocument();

aElement.OwnerDoc() is probably clearer, now that we have an Element here.

::: layout/inspector/tests/test_getCSSStyleRules.html:19
(Diff revision 1)
>   */
>  
> -let iframe = document.getElementById("test");
> +const InspectorUtils = SpecialPowers.InspectorUtils;
> -let domUtils = SpecialPowers.Cc["@mozilla.org/inspector/dom-utils;1"]
> -                            .getService(SpecialPowers.Ci.inIDOMUtils);
>  const nsIDOMCSSStyleRule = SpecialPowers.Ci["nsIDOMCSSStyleRule"];

This isn't needed anymore, right?
Attachment #8939199 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939200 [details]
Bug 1427419 - Part 4: Move rule line number methods from inIDOMUtils to InspectorUtils.

https://reviewboard.mozilla.org/r/209608/#review215916
Attachment #8939200 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939201 [details]
Bug 1427419 - Part 5: Move nsIDOMUtils.getCSSLexer to InspectorUtils.

https://reviewboard.mozilla.org/r/209610/#review215918

r=me with the function signature bits fixed.

::: dom/webidl/InspectorUtils.webidl:20
(Diff revision 1)
>    sequence<CSSRule> getCSSStyleRules(Element element,
>                                       optional DOMString pseudo = "");
>    unsigned long getRuleLine(CSSRule rule);
>    unsigned long getRuleColumn(CSSRule rule);
>    unsigned long getRelativeRuleLine(CSSRule rule);
> +  [Throws] any getCSSLexer(DOMString text);  // returns a CSSLexer object

Why not just have this return a CSSLexer, then, instead of `any`?  We couldn't do that in xpidl, because it knows nothing about CSSLexer, but webidl does!

::: layout/inspector/inDOMUtils.cpp:374
(Diff revision 1)
> -                        JS::MutableHandleValue aResult)
>  {
> -  MOZ_ASSERT(JS::CurrentGlobalOrNull(aCx));
> +  JS::Rooted<JSObject*> scope(aGlobal.Context(), aGlobal.Get());
> -  JS::Rooted<JSObject*> scope(aCx, JS::CurrentGlobalOrNull(aCx));
>    nsAutoPtr<CSSLexer> lexer(new CSSLexer(aText));
> -  if (!WrapNewBindingNonWrapperCachedObject(aCx, scope, lexer, aResult)) {
> +  if (!WrapNewBindingNonWrapperCachedObject(aGlobal.Context(), scope, lexer,

And this bit would then live in the binding-generated code.  So you'd just have this method body look like this:

    return new CSSLexer(aText);
    
and can remove the [Throws] annotation in the webidl too.
Attachment #8939201 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939202 [details]
Bug 1427419 - Part 6: Move selector methods from inIDOMUtils to InspectorUtils.

https://reviewboard.mozilla.org/r/209612/#review215922

::: layout/inspector/InspectorUtils.h:76
(Diff revision 1)
>  
> +  // Utilities for working with selectors.  We don't have a JS OM representation
> +  // of a single selector or a selector list yet, but given a rule we can index
> +  // into the selector list.
> +  //
> +  // This is a somewhat backwards API; once we move StyleRule to WebIDL we

StyleRule is already WebIDL.  This should probably reference a bug filed to add chromeonly APIs to it.  Failing that, at least remove the "once we move" bits here?

::: layout/inspector/inDOMUtils.cpp:358
(Diff revision 1)
> -} // namespace mozilla
> -
> -NS_IMETHODIMP
> +InspectorUtils::GetSelectorCount(GlobalObject& aGlobal,
> +                                 css::Rule& aRule,
> +                                 ErrorResult& aRv)
> -inDOMUtils::GetSelectorCount(nsIDOMCSSStyleRule* aRule, uint32_t *aCount)
>  {
> -  ErrorResult rv;
> +  if (aRule.GetType() != css::Rule::STYLE_RULE) {

Why are we doing this here, not at the IDL level?

I would think the arg in the webidl should be a CSSStyleRule, not a CSSRule.  Then the bindings will enforce that we have a StyleRule here.

I believe that will allow you to remove the [Throws] annotation too.  

This applies to all the methods you're moving over in this patch.

::: layout/inspector/inDOMUtils.cpp:363
(Diff revision 1)
> -  RefPtr<BindingStyleRule> rule = GetRuleFromDOMRule(aRule, rv);
> -  if (rv.Failed()) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return 0;
> -    return rv.StealNSResult();
>    }
>  
> -  *aCount = rule->GetSelectorCount();
> +  auto& rule = static_cast<BindingStyleRule&>(aRule);

And the type bindings use for CSSStyleRule is already BindingStyleRule, so you won't need this cast.

::: layout/inspector/inIDOMUtils.idl
(Diff revision 1)
> -  // For all three functions below, aSelectorIndex is 0-based
> -  AString getSelectorText(in nsIDOMCSSStyleRule aRule,
> -                          in unsigned long aSelectorIndex);
> -  unsigned long long getSpecificity(in nsIDOMCSSStyleRule aRule,
> -                                    in unsigned long aSelectorIndex);
> -  // Note: This does not handle scoped selectors correctly, because it has no

This comment about scoped selectors got dropped.  Why is that?
Attachment #8939202 - Flags: review?(bzbarsky) → review-
Comment on attachment 8939203 [details]
Bug 1427419 - Part 7: Move inIDOMUtils.isInheritedProperty to InspectorUtils.

https://reviewboard.mozilla.org/r/209614/#review215958
Attachment #8939203 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939204 [details]
Bug 1427419 - Part 8: Move inIDOMUtils.getCSSPropertyNames to InspectorUtils.

https://reviewboard.mozilla.org/r/209616/#review215964

::: layout/inspector/inDOMUtils.cpp:466
(Diff revision 1)
>      }
>    }
>  
> -  if (!(aFlags & EXCLUDE_SHORTHANDS)) {
> -    for ( ; prop < eCSSProperty_COUNT; ++prop) {
> +  for ( ; prop < eCSSProperty_COUNT; ++prop) {
> -      // Some shorthands are also aliases
> +    if (aOptions.mIncludeAliases ||

This comment is worth keeping, because it's not obvious that a thing can be both a shorthand and an alias.
Attachment #8939204 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939205 [details]
Bug 1427419 - Part 9: Move inIDOMUtils.getCSSValuesForProperty to InspectorUtils.

https://reviewboard.mozilla.org/r/209618/#review215970
Attachment #8939205 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939206 [details]
Bug 1427419 - Part 10: Remove unused nsIDOMUtils.colorNameToRGB.

https://reviewboard.mozilla.org/r/209620/#review215972
Attachment #8939206 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939207 [details]
Bug 1427419 - Part 11: Move inIDOMUtils.rgbToColorName to InspectorUtils.

https://reviewboard.mozilla.org/r/209622/#review215974
Attachment #8939207 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939208 [details]
Bug 1427419 - Part 12: Move inIDOMUtils.colorToRGBA to InspectorUtils.

https://reviewboard.mozilla.org/r/209624/#review215976

::: devtools/client/shared/test/unit/test_cssColorDatabase.js:19
(Diff revision 1)
>  const InspectorUtils = require("InspectorUtils");
>  
>  function isValid(colorName) {
>    ok(colorUtils.isValidCSSColor(colorName),
>       colorName + " is valid in database");
> -  ok(DOMUtils.isValidCSSColor(colorName),
> +  ok(InspectorUtils.isValidCSSColor(colorName),

This change (to isValidCSSColor) should be in the next changeset, not here.
Attachment #8939208 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939209 [details]
Bug 1427419 - Part 13: Move inIDOMUtils.isValidCSSColor to InspectorUtils.

https://reviewboard.mozilla.org/r/209626/#review215978
Attachment #8939209 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939210 [details]
Bug 1427419 - Part 14: Move inIDOMUtils.getSubpropertiesForCSSProperty to InspectorUtils.

https://reviewboard.mozilla.org/r/209628/#review215982

::: layout/inspector/inDOMUtils.cpp:610
(Diff revision 1)
> -    return NS_OK;
>    }
>  
>    if (!nsCSSProps::IsShorthand(propertyID)) {
> -    *aValues = static_cast<char16_t**>(moz_xmalloc(sizeof(char16_t*)));
> -    (*aValues)[0] = ToNewUnicode(nsCSSProps::GetStringValue(propertyID));
> +    NS_ConvertASCIItoUTF16 name(nsCSSProps::GetStringValue(propertyID));
> +    aResult.AppendElement(name);

This ends up doing two copies (because all the strings are short), but that's probably fine here...  If we wanted to avoid that, we could AppendElement() with no arg and then CopyASCIIToUTF16 into the new thing or so.
Attachment #8939210 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939211 [details]
Bug 1427419 - Part 15: Move inIDOMUtils.cssPropertyIsShorthand to InspectorUtils.

https://reviewboard.mozilla.org/r/209630/#review215984
Attachment #8939211 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939212 [details]
Bug 1427419 - Part 16: Move inIDOMUtils.cssPropertySupportsType to InspectorUtils.

https://reviewboard.mozilla.org/r/209632/#review215986
Attachment #8939212 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939213 [details]
Bug 1427419 - Part 17: Move inIDOMUtils.isIgnorableWhitespace to InspectorUtils.

https://reviewboard.mozilla.org/r/209634/#review215988

::: layout/inspector/inDOMView.cpp:1242
(Diff revision 1)
> +	// empty inter-tag text node without frame, e.g., in between <table>\n<tr>
> +	return true;

Fix indent, please.
Attachment #8939213 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939214 [details]
Bug 1427419 - Part 18: Move inIDOMUtils.getParentForNode to InspectorUtils.

https://reviewboard.mozilla.org/r/209636/#review215996

::: layout/inspector/inDOMView.cpp:783
(Diff revision 1)
>  
> +static already_AddRefed<nsIDOMNode>
> +GetParentForNode(nsINode* aChild, bool aShowAnonymous)
> +{
> +  MOZ_ASSERT(aChild);
> +  nsINode* parent = InspectorUtils::GetParentForNode(*aChild, aShowAnonymous);

Don't you need to include InspectorUtils.h for this to work?

::: layout/inspector/inDeepTreeWalker.cpp:149
(Diff revision 1)
> +  nsCOMPtr<nsINode> currentNode = do_QueryInterface(mCurrentNode);
> +  nsCOMPtr<nsINode> root = do_QueryInterface(mRoot);
> +
>    nsCOMPtr<nsIDOMNode> parent;
> -  MOZ_ASSERT(mDOMUtils, "mDOMUtils should have been initiated already in Init");
> -  mDOMUtils->GetParentForNode(mCurrentNode, mShowAnonymousContent,
> +  nsINode* parentNode =
> +    InspectorUtils::GetParentForNode(*currentNode, mShowAnonymousContent);

Need to include InspectorUtils.h
Attachment #8939214 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939215 [details]
Bug 1427419 - Part 19: Move inIDOMUtils.getChildrenForNode to InspectorUtils.

https://reviewboard.mozilla.org/r/209638/#review215998

::: dom/webidl/InspectorUtils.webidl:54
(Diff revision 1)
>    const unsigned long TYPE_IMAGE_RECT = 9;
>    const unsigned long TYPE_NUMBER = 10;
>    [Throws] boolean cssPropertySupportsType(DOMString property, unsigned long type);
>  
>    Node? getParentForNode(Node node, boolean showingAnonymousContent);
> +  sequence<Node> getChildrenForNode(Node node, boolean showingAnonymousContent);

Is there a reason to not return NodeList here?  It would avoid copying and the semantic changes being introduces (live vs non-live).

::: layout/inspector/inDOMUtils.cpp:150
(Diff revision 1)
> -                               nsIDOMNodeList** aChildren)
> +                                   nsTArray<RefPtr<nsINode>>& aResult)
>  {
> -  NS_ENSURE_ARG_POINTER(aNode);
> -  NS_PRECONDITION(aChildren, "Must have an out parameter");
> -
>    nsCOMPtr<nsIDOMNodeList> kids;

You could make this `nsCOMPtr<nsINodeList>`.

::: layout/inspector/inDOMUtils.cpp:164
(Diff revision 1)
>    if (!kids) {
> -    aNode->GetChildNodes(getter_AddRefs(kids));
> +    kids = content->ChildNodes();
>    }
>  
> -  kids.forget(aChildren);
> -  return NS_OK;
> +  uint32_t length = 0;
> +  kids->GetLength(&length);

If kids were nsINodeList, you could do:

    uint32_t length = kids->Length();

::: layout/inspector/inDOMUtils.cpp:166
(Diff revision 1)
> +    nsCOMPtr<nsIDOMNode> domNode;
> +    kids->Item(i, getter_AddRefs(domNode));
>  
> -namespace mozilla {
> -namespace dom {
> +    nsCOMPtr<nsINode> node = do_QueryInterface(domNode);
> +    MOZ_ASSERT(node);
> +
> +    aResult.AppendElement(node);

Please do:

    aResult.AppendElement(kids->Item(i));
    
or so.  That's assuming we want to keep returning a sequence.

::: layout/inspector/tests/test_bug609549.xhtml:45
(Diff revision 1)
>       "withoutAnons should be the same length as childNodes");
>    is(withoutAnons[0], $("p"), "didn't get paragraph - without anons");
>    is(withoutAnons[1], $("sandwiched"),
>       "didn't get sandwiched span - without anons");
>  
> -  var withAnons = domUtils.getChildrenForNode($("bound"), true);
> +  var withAnons = InspectorUtils.getChildrenForNode($("bound"), true);

Why don't you need to SpecialPowers.unwrap here?
Attachment #8939215 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939216 [details]
Bug 1427419 - Part 20: Move inIDOMUtils.getBindingURLs to InspectorUtils.

https://reviewboard.mozilla.org/r/209640/#review216002

Please double-check that the hits in extensions/inspector shown by https://searchfox.org/comm-central/search?q=getBindingURLs&case=false&regexp=false&path= are not a problem?  Actually, similar for some of the other unused things being removed here.
Attachment #8939216 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939217 [details]
Bug 1427419 - Part 26: Move inIDOMUtils.scrollElementIntoView to InspectorUtils.

https://reviewboard.mozilla.org/r/209642/#review216006
Attachment #8939217 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939218 [details]
Bug 1427419 - Part 21: Move content state methods from inIDOMUtils to InspectorUtils.

https://reviewboard.mozilla.org/r/209644/#review216008

::: layout/inspector/InspectorUtils.h:186
(Diff revision 1)
>                                   bool aShowingAnonymousContent,
>                                   nsTArray<RefPtr<nsINode>>& aResult);
>  
> +  /**
> +   * Setting and removing content state on an element. Both these functions
> +   * calling EventStateManager::SetContentState internally, the difference is

s/calling/call/

::: layout/inspector/InspectorUtils.h:186
(Diff revision 1)
>                                   bool aShowingAnonymousContent,
>                                   nsTArray<RefPtr<nsINode>>& aResult);
>  
> +  /**
> +   * Setting and removing content state on an element. Both these functions
> +   * calling EventStateManager::SetContentState internally, the difference is

s/,/;/

::: layout/inspector/inDOMUtils.cpp:911
(Diff revision 1)
>  {
> -  NS_ENSURE_ARG_POINTER(aElement);
> -
>    RefPtr<EventStateManager> esm =
>      inLayoutUtils::GetEventStateManagerFor(aElement);
> -  NS_ENSURE_TRUE(esm, NS_ERROR_INVALID_ARG);
> +  MOZ_ASSERT(esm);

You can't assert that.  There is no ESM if no shell (which we should fix, but haven't yet).

::: layout/inspector/inDOMUtils.cpp:924
(Diff revision 1)
>  {
> -  NS_ENSURE_ARG_POINTER(aElement);
> -
>    RefPtr<EventStateManager> esm =
>      inLayoutUtils::GetEventStateManagerFor(aElement);
> -  NS_ENSURE_TRUE(esm, NS_ERROR_INVALID_ARG);
> +  MOZ_ASSERT(esm);

Again, can't assert that.

::: layout/inspector/tests/test_bug462789.html
(Diff revision 1)
>      is(res.tagName, "BODY", "getParentForNode(text, true)");
>    }
>    catch(e) { ok(false, "got an unexpected exception:" + e); }
>  
> -  try {
> -    utils.setContentState(docElement, false);

Why is this test being removed?
Attachment #8939218 - Flags: review?(bzbarsky) → review-
Comment on attachment 8939219 [details]
Bug 1427419 - Part 22: Move inIDOMUtils.getUsedFontFaces to InspectorUtils.

https://reviewboard.mozilla.org/r/209646/#review216012

::: dom/base/nsRange.cpp:3486
(Diff revision 1)
>    doc->FlushPendingNotifications(FlushType::Frames);
>  
>    // Recheck whether we're still in the document
>    NS_ENSURE_TRUE(mStart.Container()->IsInUncomposedDoc(), NS_ERROR_UNEXPECTED);
>  
> -  RefPtr<nsFontFaceList> fontFaceList = new nsFontFaceList();
> +  nsDataHashtable<nsPtrHashKey<gfxFontEntry>, InspectorFontFace*> fontFaces;

The memory ownership model could sure use documenting.  I would feel a lot happier about this code not leaking if the hashtable owned the InspectorFontFace instances explicitly until we moved them out into aResult...  Any way to set that up?

::: dom/webidl/InspectorUtils.webidl:96
(Diff revision 1)
> +                                              // due to aliases, generics, localized names, etc)
> +
> +  // meaningful only when the font is a user font defined using @font-face
> +  readonly attribute CSSFontFaceRule? rule; // null if no associated @font-face rule
> +  readonly attribute long srcIndex; // index in the rule's src list, -1 if no @font-face rule
> +  readonly attribute DOMString URI; // null if not a downloaded font, i.e. local

Shouldn't the return type be `DOMString?` in that case?  Of if it can't actually be null (per the impl), should the docs here be fixed?

::: dom/webidl/InspectorUtils.webidl:97
(Diff revision 1)
> +
> +  // meaningful only when the font is a user font defined using @font-face
> +  readonly attribute CSSFontFaceRule? rule; // null if no associated @font-face rule
> +  readonly attribute long srcIndex; // index in the rule's src list, -1 if no @font-face rule
> +  readonly attribute DOMString URI; // null if not a downloaded font, i.e. local
> +  readonly attribute DOMString localName; // null if not a src:local(...) rule

Again, return type should be `DOMString?` or the docs should be fixed.

::: layout/base/nsLayoutUtils.h:2277
(Diff revision 1)
>     * Adds all font faces used in the frame tree starting from aFrame
>     * to the list aFontFaceList.
>     */
> -  static nsresult GetFontFacesForFrames(nsIFrame* aFrame,
> -                                        nsFontFaceList* aFontFaceList);
> +  static nsresult GetFontFacesForFrames(
> +      nsIFrame* aFrame,
> +      nsDataHashtable<nsPtrHashKey<gfxFontEntry>,

Given how many places we have to type this typename, seems like it could use a typedef....

::: layout/inspector/InspectorFontFace.h:1
(Diff revision 1)
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

This should be a move-with-edits from layout/inspector/nsFontFace.h

::: layout/inspector/InspectorFontFace.h:42
(Diff revision 1)
> +  ~InspectorFontFace()
> +  {
> +    MOZ_COUNT_DTOR(InspectorFontFace);
> +  }
> +
> +  gfxFontEntry* GetFontEntry() const { return mFontEntry.get(); }

No need for .get() here.

::: layout/inspector/InspectorFontFace.cpp:1
(Diff revision 1)
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

This should be a move-with-edits from layout/inspector/nsFontFace.cpp instead of being a delete-and-create.  Please make sure that gets fixed.
Attachment #8939219 - Flags: review?(bzbarsky) → review-
Comment on attachment 8939220 [details]
Bug 1427419 - Part 23: Move inIDOMUtils.getCSSPseudoElementNames to InspectorUtils.

https://reviewboard.mozilla.org/r/209648/#review216014

::: commit-message-ec0af:1
(Diff revision 1)
> +Bug 1427419 - Part 24: Move inIDOMUtils.getCSSPseudoElements to InspectorUtils. r?bz

getCSSPseudoElementNames
Attachment #8939220 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939221 [details]
Bug 1427419 - Part 24: Move pseudo-class lock methods from inIDOMUtils to InspectorUtils.

https://reviewboard.mozilla.org/r/209650/#review216018

::: browser/modules/PluginContent.jsm:524
(Diff revision 1)
>        plugin.removeAttribute("href");
>        let overlay = this.getPluginUI(plugin, "main");
>        this.setVisibility(plugin, overlay, OVERLAY_DISPLAY.FULL);
> -      let inIDOMUtils = Cc["@mozilla.org/inspector/dom-utils;1"]
> -                          .getService(Ci.inIDOMUtils);
>        // Add psuedo class so our styling will take effect

While you're here, s/psuedo/pseudo/
Attachment #8939221 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939222 [details]
Bug 1427419 - Part 25: Move inIDOMUtils.parseStyleSheet to InspectorUtils.

https://reviewboard.mozilla.org/r/209652/#review216020
Attachment #8939222 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939224 [details]
Bug 1427419 - Part 28: Remove inIDOMUtils.

https://reviewboard.mozilla.org/r/209656/#review216024

Please post a diff that actually does the move, because that will be a _lot_ simpler to review....
Attachment #8939224 - Flags: review?(bzbarsky) → review-
Comment on attachment 8939223 [details]
Bug 1427419 - Part 27: Remove inIDOMUtils.

https://reviewboard.mozilla.org/r/209654/#review216022
Attachment #8939223 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #35)
> Could you file a followup bug on that?  Seems nasty.

Filed bug 1428240.
Flags: needinfo?(cam)
Comment on attachment 8939210 [details]
Bug 1427419 - Part 14: Move inIDOMUtils.getSubpropertiesForCSSProperty to InspectorUtils.

https://reviewboard.mozilla.org/r/209628/#review215982

> This ends up doing two copies (because all the strings are short), but that's probably fine here...  If we wanted to avoid that, we could AppendElement() with no arg and then CopyASCIIToUTF16 into the new thing or so.

Oh, I never realized that the NS_Convert* classes were auto strings.
Comment on attachment 8939215 [details]
Bug 1427419 - Part 19: Move inIDOMUtils.getChildrenForNode to InspectorUtils.

https://reviewboard.mozilla.org/r/209638/#review215998

> Is there a reason to not return NodeList here?  It would avoid copying and the semantic changes being introduces (live vs non-live).

Good point about live versus non-live.  I'll go back to the node list.
Comment on attachment 8939215 [details]
Bug 1427419 - Part 19: Move inIDOMUtils.getChildrenForNode to InspectorUtils.

https://reviewboard.mozilla.org/r/209638/#review215998

> Why don't you need to SpecialPowers.unwrap here?

Just because we don't need to do any object identity testing on the nodes in withAnons (unlike withoutAnons above).
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #56)
> Comment on attachment 8939216 [details]
> Bug 1427419 - Part 20: Remove unused inIDOMUtils.getBindingURLs.
> 
> https://reviewboard.mozilla.org/r/209640/#review216002
> 
> Please double-check that the hits in extensions/inspector shown by
> https://searchfox.org/comm-central/
> search?q=getBindingURLs&case=false&regexp=false&path= are not a problem? 
> Actually, similar for some of the other unused things being removed here.

colorNameToRgb is unused in comm-central, but it does look like the rest of the APIs I'm removing are used in the old DOM inspector extension that's still in the tree there.

Jörg, can you tell me if the inspector extension in the tree at comm-central/extensions/inspector/ is still being used?  If so, I might have to migrate these few inIDOMUtils methods that I was planning on removing to InspectorUtils.
Flags: needinfo?(jorgk)
Comment on attachment 8939218 [details]
Bug 1427419 - Part 21: Move content state methods from inIDOMUtils to InspectorUtils.

https://reviewboard.mozilla.org/r/209644/#review216008

> Why is this test being removed?

Mistakenly thought I was removing a getContentState test; will restore.
Comment on attachment 8939218 [details]
Bug 1427419 - Part 21: Move content state methods from inIDOMUtils to InspectorUtils.

https://reviewboard.mozilla.org/r/209644/#review216008

> Mistakenly thought I was removing a getContentState test; will restore.

After restoring, I remember the real reason I removed it. :-)  It's testing that passing false for the second argument throws an exception.  Switching to Web IDL bindings no longer throws an exception since the false gets converted to unsigned long long 0.  I don't think it's worth keeping the test by passing in some other value for the second argument; we've probably got enough Web IDL binding tests for argument conversion.
Comment on attachment 8939219 [details]
Bug 1427419 - Part 22: Move inIDOMUtils.getUsedFontFaces to InspectorUtils.

https://reviewboard.mozilla.org/r/209646/#review216012

> The memory ownership model could sure use documenting.  I would feel a lot happier about this code not leaking if the hashtable owned the InspectorFontFace instances explicitly until we moved them out into aResult...  Any way to set that up?

Using nsClassHashtable did the trick.
Comment on attachment 8939202 [details]
Bug 1427419 - Part 6: Move selector methods from inIDOMUtils to InspectorUtils.

https://reviewboard.mozilla.org/r/209612/#review215922

> This comment about scoped selectors got dropped.  Why is that?

Removed it because I thought it was talking about <style scoped>.  But it could also be talking about :scope, so I've restored it.
Comment on attachment 8939214 [details]
Bug 1427419 - Part 18: Move inIDOMUtils.getParentForNode to InspectorUtils.

https://reviewboard.mozilla.org/r/209636/#review215996

> Don't you need to include InspectorUtils.h for this to work?

Unified build saved me, but yes I'll add that.
(In reply to Cameron McCormack (:heycam) from comment #69)
> Jörg, can you tell me if the inspector extension in the tree at
> comm-central/extensions/inspector/ is still being used?  If so, I might have
> to migrate these few inIDOMUtils methods that I was planning on removing to
> InspectorUtils.
Yes, it's being used and maintained by the SeaMonkey people. I'll NI FRG.
Flags: needinfo?(jorgk) → needinfo?(frgrahl)
Comment on attachment 8939219 [details]
Bug 1427419 - Part 22: Move inIDOMUtils.getUsedFontFaces to InspectorUtils.

https://reviewboard.mozilla.org/r/209646/#review216156

::: dom/base/nsRange.cpp:3488
(Diff revisions 1 - 2)
>    // Recheck whether we're still in the document
>    NS_ENSURE_TRUE(mStart.Container()->IsInUncomposedDoc(), NS_ERROR_UNEXPECTED);
>  
> -  nsDataHashtable<nsPtrHashKey<gfxFontEntry>, InspectorFontFace*> fontFaces;
> +  // A table to map gfxFontEntry objects to InspectorFontFace objects.
> +  // (We hold on to the InspectorFontFaces strongly due to the nsAutoPtrs
> +  // in the nsClassHashtable, until we move them out at the end into aResult

Take out the first "at the end"?

::: layout/inspector/InspectorFontFace.h:1
(Diff revision 2)
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

This still needs to be a move-with-edits from layout/inspector/nsFontFace.h
Attachment #8939219 - Flags: review?(bzbarsky) → review-
Comment on attachment 8939202 [details]
Bug 1427419 - Part 6: Move selector methods from inIDOMUtils to InspectorUtils.

https://reviewboard.mozilla.org/r/209612/#review216160

::: layout/inspector/inDOMUtils.cpp:357
(Diff revision 2)
> -NS_IMETHODIMP
> -inDOMUtils::GetSelectorText(nsIDOMCSSStyleRule* aRule,
> +/* static */ void
> +InspectorUtils::GetSelectorText(GlobalObject& aGlobal,
> +                                BindingStyleRule& aRule,
> -                            uint32_t aSelectorIndex,
> +                                uint32_t aSelectorIndex,
> -                            nsAString& aText)
> +                                nsString& aText)
>  {

This is losing the exception for out-of-range aSelectorIndex, no?  Should propagate it out.  Same for GetSpecificity and SelectorMatchesElement.
Attachment #8939202 - Flags: review?(bzbarsky) → review-
Comment on attachment 8939218 [details]
Bug 1427419 - Part 21: Move content state methods from inIDOMUtils to InspectorUtils.

https://reviewboard.mozilla.org/r/209644/#review216162

r=me
Attachment #8939218 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939224 [details]
Bug 1427419 - Part 28: Remove inIDOMUtils.

https://reviewboard.mozilla.org/r/209656/#review216164

r=me.  Much clearer now that there are no actual changes to the main .cpp file.  ;)
Attachment #8939224 - Flags: review?(bzbarsky) → review+
Blocks: 1415940
Comment on attachment 8939219 [details]
Bug 1427419 - Part 22: Move inIDOMUtils.getUsedFontFaces to InspectorUtils.

https://reviewboard.mozilla.org/r/209646/#review216156

> This still needs to be a move-with-edits from layout/inspector/nsFontFace.h

Ugh, sorry, I only noticed that nsFontFace.cpp was sucessfully recognized as a move.
Attachment #8939224 - Attachment is obsolete: true
mozreview isn't cooperating, but bz could you please take another look at parts 17, 20, and 26, where I've moved the APIs across to InspectorUtils instead of dropping them?
Flags: needinfo?(bzbarsky)
Comment on attachment 8939219 [details]
Bug 1427419 - Part 22: Move inIDOMUtils.getUsedFontFaces to InspectorUtils.

https://reviewboard.mozilla.org/r/209646/#review216954
Attachment #8939219 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939202 [details]
Bug 1427419 - Part 6: Move selector methods from inIDOMUtils to InspectorUtils.

https://reviewboard.mozilla.org/r/209612/#review216958
Attachment #8939202 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939216 [details]
Bug 1427419 - Part 20: Move inIDOMUtils.getBindingURLs to InspectorUtils.

https://reviewboard.mozilla.org/r/209640/#review216964

r=me on the "move, not remove" version
Comment on attachment 8939217 [details]
Bug 1427419 - Part 26: Move inIDOMUtils.scrollElementIntoView to InspectorUtils.

https://reviewboard.mozilla.org/r/209642/#review216966

r=me on "Move inIDOMUtils.scrollElementIntoView to InspectorUtils", though that's not what mozreview thinks this patch used to be....
Thanks for all the reviews, Boris.
Flags: needinfo?(bzbarsky)
Depends on: 1429345
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/503f9595fa36
Part 1: Add an InspectorUtils chrome-only IDL namespace. r=bz,tromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/483b5d2d9d1a
Part 2: Move nsIDOMUtils.getAllStyleSheets to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/70dfa99ad56b
Part 3: Move inIDOMUtils.getCSSStyleRules to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/1245c7305478
Part 4: Move rule line number methods from inIDOMUtils to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/c890abebe25b
Part 5: Move nsIDOMUtils.getCSSLexer to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd91519d1634
Part 6: Move selector methods from inIDOMUtils to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/dae8bd22a766
Part 7: Move inIDOMUtils.isInheritedProperty to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/19ea76feea12
Part 8: Move inIDOMUtils.getCSSPropertyNames to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fab386062a1
Part 9: Move inIDOMUtils.getCSSValuesForProperty to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7386d96d8b2
Part 10: Remove unused nsIDOMUtils.colorNameToRGB. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/044e1b16df76
Part 11: Move inIDOMUtils.rgbToColorName to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/87e619f7e97c
Part 12: Move inIDOMUtils.colorToRGBA to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/aec7e1a3fc90
Part 13: Move inIDOMUtils.isValidCSSColor to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/d11af777fea3
Part 14: Move inIDOMUtils.getSubpropertiesForCSSProperty to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/79e60f844497
Part 15: Move inIDOMUtils.cssPropertyIsShorthand to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca4d7ba6c2d4
Part 16: Move inIDOMUtils.cssPropertySupportsType to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/5da05746efa7
Part 17: Move inIDOMUtils.isIgnorableWhitespace to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/247b8271daf3
Part 18: Move inIDOMUtils.getParentForNode to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/af37e734e0fd
Part 19: Move inIDOMUtils.getChildrenForNode to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/94ef86972ba1
Part 20: Move inIDOMUtils.getBindingURLs to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5968ffde72c
Part 21: Move content state methods from inIDOMUtils to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/b295b67d3a8b
Part 22: Move inIDOMUtils.getUsedFontFaces to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/441bc7969a24
Part 23: Move inIDOMUtils.getCSSPseudoElementNames to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b20cd4c392c
Part 24: Move pseudo-class lock methods from inIDOMUtils to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/3196880bdf5d
Part 25: Move inIDOMUtils.parseStyleSheet to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fd3dcaae286
Part 26: Move inIDOMUtils.scrollElementIntoView to InspectorUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/4446129c5aba
Part 27: Remove inIDOMUtils. r=bz
https://hg.mozilla.org/mozilla-central/rev/503f9595fa36
https://hg.mozilla.org/mozilla-central/rev/483b5d2d9d1a
https://hg.mozilla.org/mozilla-central/rev/70dfa99ad56b
https://hg.mozilla.org/mozilla-central/rev/1245c7305478
https://hg.mozilla.org/mozilla-central/rev/c890abebe25b
https://hg.mozilla.org/mozilla-central/rev/cd91519d1634
https://hg.mozilla.org/mozilla-central/rev/dae8bd22a766
https://hg.mozilla.org/mozilla-central/rev/19ea76feea12
https://hg.mozilla.org/mozilla-central/rev/3fab386062a1
https://hg.mozilla.org/mozilla-central/rev/b7386d96d8b2
https://hg.mozilla.org/mozilla-central/rev/044e1b16df76
https://hg.mozilla.org/mozilla-central/rev/87e619f7e97c
https://hg.mozilla.org/mozilla-central/rev/aec7e1a3fc90
https://hg.mozilla.org/mozilla-central/rev/d11af777fea3
https://hg.mozilla.org/mozilla-central/rev/79e60f844497
https://hg.mozilla.org/mozilla-central/rev/ca4d7ba6c2d4
https://hg.mozilla.org/mozilla-central/rev/5da05746efa7
https://hg.mozilla.org/mozilla-central/rev/247b8271daf3
https://hg.mozilla.org/mozilla-central/rev/af37e734e0fd
https://hg.mozilla.org/mozilla-central/rev/94ef86972ba1
https://hg.mozilla.org/mozilla-central/rev/a5968ffde72c
https://hg.mozilla.org/mozilla-central/rev/b295b67d3a8b
https://hg.mozilla.org/mozilla-central/rev/441bc7969a24
https://hg.mozilla.org/mozilla-central/rev/1b20cd4c392c
https://hg.mozilla.org/mozilla-central/rev/3196880bdf5d
https://hg.mozilla.org/mozilla-central/rev/9fd3dcaae286
https://hg.mozilla.org/mozilla-central/rev/4446129c5aba
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: needinfo?(frgrahl)
The developer documentation for this could use an update to reflect the new name of the interface. (I happened across https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/inIDOMUtils recently; not sure if there are other references to it that should also be updated.)
Keywords: dev-doc-needed
I know very little about this side of things, and we don't really support the internals docs as such any more...but I'll make time for it if I can.

Is this basically just a simple interface name change, from inIDOMUtils to InspectorUtils, or is there more to it?
Flags: needinfo?(jfkthame)
> Is this basically just a simple interface name change, from inIDOMUtils to InspectorUtils, or is there more to it?

It's more than just that, IIUC; methods are updated to return WebIDL types, which may not be exactly the same as the old XPCOM types that were being used. And we no longer use Cc["..."].getService(...) to access this (it's SpecialPowers.InspectorUtils now), so example code is out of date in that respect, too.

Cameron, can you give any guidance here?
Flags: needinfo?(jfkthame) → needinfo?(cam)
I feel like having documentation in MDN for inIDOMUtils was useful at a time when it was possible to use by extensions.  But now it really is just an internal thing for our own devtools to use, it's enough for developers to rely on whatever documentation exists in code for these things.  So I would suggest removing the inIDOMUtils content on MDN.

WDYT Chris?
Flags: needinfo?(cam) → needinfo?(cmills)
If you think that is OK, I am happy to remove it. Is there a particular URL I should point to, or shall I just say something like "document deleted, as in-code documentation is more current"?
Flags: needinfo?(cmills) → needinfo?(cam)
I think it would be fine to say that nsIDOMUtils was converted to Web IDL, and is now called InspectorUtils, and to see documentation in that class's header file.
Flags: needinfo?(cam)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: