1017248 - Media sidebar should work for original sources as well

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Description

[Heather Arthur [:harth]]

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.

Comment 1

[Heather Arthur [:harth]]

Attachment: Show media sidebar on original sources

Shows @media sidebar on original sources with line number in original source.

Comment 2

[Heather Arthur [:harth]]

Attachment: 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

Comment 3

[Heather Arthur [:harth]]

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.

Comment 4

[Brian Grinstead [:bgrins]]

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

Comment 5

[Heather Arthur [:harth]]

(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.

Comment 6

[Heather Arthur [:harth]]

Attachment: 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.

Comment 7

[Brian Grinstead [:bgrins]]

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.

Comment 8

[Heather Arthur [:harth]]

(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.

Comment 9

[Heather Arthur [:harth]]

https://hg.mozilla.org/integration/fx-team/rev/8ebe2f070df0

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ebe2f070df0