Last Comment Bug 819930 - API to emulate CSS Media Types
: API to emulate CSS Media Types
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks: 1202711
  Show dependency treegraph
 
Reported: 2012-12-10 06:51 PST by Rob Campbell [:rc] (:robcee)
Modified: 2015-09-08 09:31 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Add API for chrome callers to emulate CSS media types (13.12 KB, patch)
2013-07-15 12:18 PDT, Graeme McCutcheon [:graememcc]
dbaron: review-
Details | Diff | Splinter Review
Part 2 - Add GCLI command (8.12 KB, patch)
2013-07-15 12:19 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Splinter Review
Part 1 - Add API for chrome callers to emulate CSS media types (11.71 KB, patch)
2013-07-16 09:27 PDT, Graeme McCutcheon [:graememcc]
dbaron: review+
Details | Diff | Splinter Review
Part 2 - Add GCLI command (7.13 KB, patch)
2013-07-16 09:28 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Splinter Review
Part 2 - Add GCLI command (7.14 KB, patch)
2013-07-17 08:55 PDT, Graeme McCutcheon [:graememcc]
jwalker: review+
Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2012-12-10 06:51:10 PST
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:

https://plus.google.com/u/0/115133653231679625609/posts/MgpioU84JPe
Comment 1 Paul Rouget [:paul] 2012-12-10 07:59:28 PST
We want a "option" menu in the toolbox toolbar (on the left of the tabs). Maybe there.
Comment 2 Girish Sharma [:Optimizer] 2012-12-10 08:03:39 PST
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 ?
Comment 3 Paul Rouget [:paul] 2012-12-10 08:12:41 PST
We should not worry about the UI for now (we can expose this feature as a gcli-command to start).
Comment 4 Graeme McCutcheon [:graememcc] 2013-07-15 12:18:15 PDT
Created attachment 775852 [details] [diff] [review]
Part 1 - Add API for chrome callers to emulate CSS media types
Comment 5 Graeme McCutcheon [:graememcc] 2013-07-15 12:19:38 PDT
Created attachment 775853 [details] [diff] [review]
Part 2 - Add GCLI command

Holding off on r? until I've addressed any comments from part 1
Comment 6 David Baron :dbaron: ⌚️UTC-10 2013-07-15 14:10:44 PDT
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.
Comment 7 Graeme McCutcheon [:graememcc] 2013-07-16 09:27:36 PDT
Created attachment 776463 [details] [diff] [review]
Part 1 - Add API for chrome callers to emulate CSS media types

David, thanks for that comment: I'd missed the obvious, strings make far more sense.
Comment 8 Graeme McCutcheon [:graememcc] 2013-07-16 09:28:09 PDT
Created attachment 776465 [details] [diff] [review]
Part 2 - Add GCLI command
Comment 9 David Baron :dbaron: ⌚️UTC-10 2013-07-16 15:59:59 PDT
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 
needed.

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
Comment 10 Graeme McCutcheon [:graememcc] 2013-07-17 08:55:47 PDT
Created attachment 777158 [details] [diff] [review]
Part 2 - Add GCLI command

Updated with the renaming suggested in comment 10
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-07-17 09:14:18 PDT
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.
Thanks.

::: 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/gclicommands.properties
@@ +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
Comment 12 Graeme McCutcheon [:graememcc] 2013-07-18 02:18:10 PDT
(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.
Comment 13 Graeme McCutcheon [:graememcc] 2013-07-18 02:29:53 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/66f4834afb70
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c42e1391bd2

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.
Comment 15 Daniel Glazman (:glazou) 2013-07-22 04:15:30 PDT
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!
Comment 16 J. Ryan Stinnett [:jryans] (use ni?) 2015-09-08 09:31:06 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.