Closed Bug 1013814 Opened 6 years ago Closed 5 years ago

Create DOMUtils.getRelativeRuleLine(nsIDOMCSSStyleRule *aRule)

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jwalker, Assigned: tromey)

References

Details

Attachments

(1 file, 1 obsolete file)

DOMUtils.getRelativeRuleLine is just like its cousin DOMUtils.getRuleLine except when the style rule is in a <style> element, in which case it returns a line number relative to the top of the <style> element rather than to the containing HTML.

Alternatively maybe DOMUtils.getStyleElementLine could get the line number of the <style> element in the HTML document.
The latter sounds easier.
Duplicate of this bug: 1191024
Needed for as-authored.
Assignee: nobody → ttromey
Blocks: 984880
Status: NEW → ASSIGNED
Comment on attachment 8646996 [details] [diff] [review]
add inIDOMUtils.getRelativeRuleLine

I think I need reviews for both the layout and devtools bits.
I'm not sure if the (minor) dom changes need separate review.
Attachment #8646996 - Flags: review?(pbrosset)
Attachment #8646996 - Flags: review?(cam)
Comment on attachment 8646996 [details] [diff] [review]
add inIDOMUtils.getRelativeRuleLine

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

r=me on the layout/ (and dom/) changes.

::: dom/base/nsIStyleSheetLinkingElement.h
@@ +103,5 @@
> +   *
> +   * @return the line number of this element; or 1 if no line number
> +   *         was set
> +   */
> +  virtual uint32_t GetLineNumber() = 0;

Please rev the uuid for nsIStyleSheetLinkingElement.

::: layout/inspector/tests/test_getRelativeRuleLine.html
@@ +30,5 @@
> +                           .getService(SpecialPowers.Ci.inIDOMUtils);
> +
> +  let tests = [
> +    { sheetNo: 0, ruleNo: 0, lineNo: 1, columnNo: 1 },
> +    { sheetNo: 1, ruleNo: 0, lineNo: 2, columnNo: 15 },

Not to do with this bug, but is it expected that column 15 is reported here (after the "@supports") instead of 5 (before it)?
Attachment #8646996 - Flags: review?(cam) → review+
Comment on attachment 8646996 [details] [diff] [review]
add inIDOMUtils.getRelativeRuleLine

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

Looking good. Thanks Tom for cleaning that part of the devtools code up.
Attachment #8646996 - Flags: review?(pbrosset) → review+
(In reply to Cameron McCormack (:heycam) from comment #6)

> Not to do with this bug, but is it expected that column 15 is reported here
> (after the "@supports") instead of 5 (before it)?

Yeah, it is expected in the sense that the parser gives the rule the location
of the token following the @suppports keyword:

https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp#4135

This seems to be done consistently (I spot checked a couple others) for
@-keyword rules.

Whether this is the most useful location, I am not sure.  It's possible that
the devtools would prefer the location of the @-keyword itself.  I made a note in
my project to-do list to check on this.  I tend to think it isn't an issue; but if
it is I will file a separate bug for it.
Address review comment.
Attachment #8646996 - Attachment is obsolete: true
Attachment #8647697 - Flags: review+
Keywords: checkin-needed
(In reply to Tom Tromey :tromey from comment #8)

> Whether this is the most useful location, I am not sure.  It's possible that
> the devtools would prefer the location of the @-keyword itself.  I made a
> note in
> my project to-do list to check on this.  I tend to think it isn't an issue;
> but if
> it is I will file a separate bug for it.

I looked at this, and I think that at least for the time being, the current
locations are ok.  The rule view exposes ordinary rules and the contents of
@supports and @keyframes; and there's no way in the rule view to edit the
latter two directly (just their "ordinary rule" contents).
https://hg.mozilla.org/mozilla-central/rev/880092cace77
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.