Closed Bug 1017248 Opened 6 years ago Closed 5 years ago

Media sidebar should work for original sources as well

Categories

(DevTools :: Style Editor, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: harth, Assigned: harth)

Details

Attachments

(1 file, 2 obsolete files)

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: nobody → fayearthur
Shows @media sidebar on original sources with line number in original source.
Attachment #8432277 - Flags: review?(pbrosset)
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)
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)
(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.
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/8ebe2f070df0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.