Media sidebar should work for original sources as well

RESOLVED FIXED in Firefox 32

Status

()

Firefox
Developer Tools: Style Editor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: harth, Assigned: harth)

Tracking

unspecified
Firefox 32
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
We should display the @media sidebar for original sources in the style editor. We can pull the rules out of the generated source and show the line numbers from the original file.
(Assignee)

Updated

4 years ago
Assignee: nobody → fayearthur
(Assignee)

Comment 1

4 years ago
Created attachment 8432277 [details] [diff] [review]
Show media sidebar on original sources

Shows @media sidebar on original sources with line number in original source.
Attachment #8432277 - Flags: review?(pbrosset)
(Assignee)

Comment 2

4 years ago
Created attachment 8435069 [details] [diff] [review]
Show @media sidebar for original sources

This one fixes a couple oranges too that cropped up in my try run.

Try: https://tbpl.mozilla.org/?tree=Try&rev=5f5a93181029
Attachment #8432277 - Attachment is obsolete: true
Attachment #8432277 - Flags: review?(pbrosset)
Attachment #8435069 - Flags: review?(pbrosset)
(Assignee)

Comment 3

4 years ago
Comment on attachment 8435069 [details] [diff] [review]
Show @media sidebar for original sources

This is not too much of a change to make it work for original sources. Other changes are to fix oranges cropping up because of the sequence of editors being added and selected.
Attachment #8435069 - Flags: review?(pbrosset) → review?(bgrinstead)
Status: NEW → ASSIGNED
Comment on attachment 8435069 [details] [diff] [review]
Show @media sidebar for original sources

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

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +735,3 @@
>  
>        for (let rule of rules) {
> +        let loc = rule;

It took me a while to figure out what 'loc' means here (I guess location).  So basically it is an object where we only care about three things {href aka 'source', line, column}.

What confused me a bit about this loop was that loc was assigned to the full rule object, then later reassigned if in a source and then loc.href was set separately. 

IMO this would be a bit clearer and would match the interface of the varible to what is returned by getOriginalLocation():

let location = {
  line: rule.line,
  column: rule.column,
  source: editor.styleSheet.href
}
if (.isOriginalSource) {
  location = .getOriginalLocation(...);
}

...

That's just a suggestion for what would read easier for me, feel free to use it or not.  I would also change the signature of _jumpToLocation to expect this type of object instead of a rule-like object since it no longer.

::: browser/devtools/styleeditor/test/browser_styleeditor_media_sidebar_sourcemaps.js
@@ +8,5 @@
> +const MAP_PREF = "devtools.styleeditor.source-maps-enabled";
> +
> +const LABELS = ["screen and (max-width: 320px)",
> +                "screen and (min-width: 1200px)"];
> +const LINE_NOS = [4, 4];

Why is this expecting line 4 for each media query instead of 5 and 8?  Maybe I'm just misunderstanding source mapping. Screenshot here: https://www.dropbox.com/s/2cb9vusn20p3k2q/source-map-media.png
Attachment #8435069 - Flags: review?(bgrinstead)
(Assignee)

Comment 5

4 years ago
(In reply to Brian Grinstead [:bgrins] from comment #4)
> Why is this expecting line 4 for each media query instead of 5 and 8?  Maybe
> I'm just misunderstanding source mapping. Screenshot here:
> https://www.dropbox.com/s/2cb9vusn20p3k2q/source-map-media.png

It's a shame, but we have to use the location of the first rule within the media query as it's location, because we can't get the line number for a @media rule, see bug 591303.
(Assignee)

Comment 6

4 years ago
Created attachment 8435310 [details] [diff] [review]
Show @media sidebar for original sources v2

Thanks for the review. Updated the confusing line. Not much we can do about the line number for now.
Attachment #8435069 - Attachment is obsolete: true
Attachment #8435310 - Flags: review?(bgrinstead)
Comment on attachment 8435310 [details] [diff] [review]
Show @media sidebar for original sources v2

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

Looks good

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +739,5 @@
> +        let location = {
> +          line: line,
> +          column: column,
> +          source: editor.styleSheet.href,
> +          styleSheet: parentStyleSheet

Do you need to include parentStyleSheet here?  I'm not positive, but it seems like the call to selectStyleSheet further down would have the same result whether it was called with source or styleSheet.
Attachment #8435310 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 8

4 years ago
(In reply to Brian Grinstead [:bgrins] from comment #7)
> Do you need to include parentStyleSheet here?  I'm not positive, but it
> seems like the call to selectStyleSheet further down would have the same
> result whether it was called with source or styleSheet.

Yeah, href is null for inline sheets, so if we know the exact sheet we can select the right one in that case, and default to using the href if not.
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/8ebe2f070df0
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8ebe2f070df0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.