7 years ago
10 months ago


(Reporter: rcampbell, Unassigned)


Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(2 attachments, 3 obsolete attachments)



7 years ago
We should provide a way to display pages rendered using different media types if available.

Not clear where would be the best place to put this control: The Style Editor, The Inspector, or a stand-alone option?

For an example of how Chrome's implemented it, see:
We want a "option" menu in the toolbox toolbar (on the left of the tabs). Maybe there.
This might need its own option meni, as it will have a menu list from which users will choose. May be integrate this with Responsive Mode, as both are related ?
We should not worry about the UI for now (we can expose this feature as a gcli-command to start).
Posted patch Part 2 - Add GCLI command (obsolete) — Splinter Review
Holding off on r? until I've addressed any comments from part 1
Comment on attachment 775852 [details] [diff] [review]
Part 1 - Add API for chrome callers to emulate CSS media types

I'm not crazy about this feature, since media types are something we should be discouraging authors from using in the first place (and we should also ensure anything important covered by them is covered by media queries, if that's not the case already), and also since these media types are likely to be associated with other characteristics that this doesn't emulate.  Though I suppose giving authors the ability to test what happens with different media types helps them catch things that are unnecessarily type-specific.

As far as the patch goes -- please don't use |short| in C++ -- use int16_t or similar.  That said, uint8_t seems more sensible.  Or, actually... just make the API take a string argument.  Given that the caller has a string, the recipient wants a string, and strings make better API, it seems like you want the API to be string-based.  (I'd probably skip on the input validation, and just make mMediaEmulated by an nsCOMPtr<nsIAtom> or nsRefPtr<nsIAtom>.)

In nsPresContext.h, please put "bool mIsEmulatingMedia" next to the long list of 1-bit unsigned members, and make it a 1-bit unsigned like them.

Other than that, I guess this is fine, but I'd like to look at the revised patch.
Attachment #775852 - Flags: review?(dbaron) → review-
David, thanks for that comment: I'd missed the obvious, strings make far more sense.
Attachment #775852 - Attachment is obsolete: true
Attachment #776463 - Flags: review?(dbaron)
Posted patch Part 2 - Add GCLI command (obsolete) — Splinter Review
Attachment #775853 - Attachment is obsolete: true
Comment on attachment 776463 [details] [diff] [review]
Part 1 - Add API for chrome callers to emulate CSS media types

Drop the changes to content/base/src/nsGkAtomList.h; they're no longer 

I might prefer renaming resetMedium -> stopEmulatingMedium just to
make it clearer that what it resets is the emulation.

And I realize you probably also want to do the
EnumerateExternalResources bit that nsDocumentViewer::SetTextZoom (and
two other functions) do.  Otherwise you won't touch SVG resource 
documents, which you probably should for this feature.  (And, in fact,
it might not be a bad idea to write a version of CallChildren that does
external resources too, though as a separate patch, including fixing
the three existing users.)

Also, the test doesn't need SimpleTest.waitForExplicitFinish() and
SimpleTest.finish(), as far as I can see.  

I think you should also switch the test to use 
getComputedStyle(document.body, "background-color") rather than using
screenshots; it's much more efficient, and you can write more precise
tests (without the O(N^2) business).

r=dbaron with those changes
Attachment #776463 - Flags: review?(dbaron) → review+
Updated with the renaming suggested in comment 10
Attachment #776465 - Attachment is obsolete: true
Attachment #777158 - Flags: review?(jwalker)
Comment on attachment 777158 [details] [diff] [review]
Part 2 - Add GCLI command

Review of attachment 777158 [details] [diff] [review]:

I like it - I think this could be really useful.

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +2207,5 @@
> +    description: gcli.lookup("mediaResetDesc"),
> +    manual: gcli.lookup("mediaEmulateManual"),
> +    exec: function(args, context) {
> +      let markupDocumentViewer = context.environment.chromeDocument.defaultView
> +                                        .gBrowser.markupDocumentViewer;

For what it's worth context.environment.chromeDocument.defaultView == context.environment.chromeWindow

::: browser/locales/en-US/chrome/browser/devtools/
@@ +1306,5 @@
> +
> +# LOCALIZATION NOTE (mediaResetManual) A fuller description of the
> +# 'media reset' command, displayed when the user asks for help on what it
> +# does.
> +mediaResetManual=Stop emulating a CSS media type

Our use of comments in l10n files isn't great, and I've a patch that's about to land that makes things much less repetitive. So can I suggest:

# LOCALIZATION NOTE (mediaDesc, mediaEmulateDesc, mediaEmulateManual,
# mediaEmulateType, mediaResetDesc, mediaResetManual) These strings describe
# the 'media' commands and all available parameters.
mediaDesc=CSS media type emulation
mediaEmulateDesc=Emulate a specified CSS media type
mediaEmulateManual=View the document as if rendered on a device supporting the given media type, with the relevant CSS rules applied.
mediaEmulateType=The media type to emulate
mediaResetDesc=Stop emulating a CSS media type
mediaResetManual=Stop emulating a CSS media type
Attachment #777158 - Flags: review?(jwalker) → review+
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #9)
> And, in fact, it might not be a bad idea to write a version of CallChildren that does
> external resources too

Filed bug 895328.

There was an errant constant from an earlier iteration of part 1 still present in part 2, which I fixed on checkin.

Given comments 1-3, marking this leave-open for now in case DevTools want to iterate on exposing this further on the UI - if not please clear the whiteboard annotation.
Whiteboard: [leave open]
Nice! As a matter of fact, I implemented something extremely similar, even
code-wise, in BlueGriffon. Please add a new API to nsIHTMLEditor so this
can be controlled from chrome JS above the editor w/o having to call
execCommand. Thanks!
Let's call this one done, since we did land an API here in Firefox 25.

I've filed bug 1202711 for exposing a UI to control it in responsive design.
Last Resolved: 4 years ago
Resolution: --- → FIXED
Summary: Emulate CSS Media Types → API to emulate CSS Media Types
Whiteboard: [leave open]


10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.