Bug 103752 - Web Inspector: [Overrides] Add ability to override the page CSS media type
Summary: Web Inspector: [Overrides] Add ability to override the page CSS media type
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-30 09:13 PST by Alexander Pavlov (apavlov)
Modified: 2013-03-25 03:28 PDT (History)
12 users (show)

See Also:


Attachments
Patch (16.07 KB, patch)
2012-11-30 09:29 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (19.03 KB, patch)
2012-12-03 01:04 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (19.16 KB, patch)
2012-12-03 01:57 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch for landing (20.15 KB, patch)
2012-12-04 01:59 PST, Alexander Pavlov (apavlov)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2012-11-30 09:13:12 PST
It would be great to have an ability switching media type of the page to print in the DevTools to see what CSS get applied and the result html.

Upstreaming http://code.google.com/p/chromium/issues/detail?id=161496
Comment 1 Alexander Pavlov (apavlov) 2012-11-30 09:29:35 PST
Created attachment 176980 [details]
Patch
Comment 2 Andrey Kosyakov 2012-11-30 15:52:06 PST
Comment on attachment 176980 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176980&action=review

> Source/WebCore/page/FrameView.cpp:1339
> +    if (InspectorInstrumentation::shouldEmulatePrintMedia(m_frame.get()))
> +        overrideType = "print";

Why not make it generic -- i.e. allow to override to arbitrary media type?
Comment 3 Pavel Feldman 2012-11-30 17:36:09 PST
Comment on attachment 176980 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176980&action=review

> Source/WebCore/inspector/Inspector.json:435
> +                    { "name": "enabled", "type": "boolean", "description": "Whether the printed media emulation should be enabled." }

Lets keep it generic in the protocol.

> Source/WebCore/inspector/front-end/SettingsScreen.js:331
> +        const p = document.createElement("p");

This should be a part of the overrides

>> Source/WebCore/page/FrameView.cpp:1339
>> +        overrideType = "print";
> 
> Why not make it generic -- i.e. allow to override to arbitrary media type?

+1
Comment 4 Alexander Pavlov (apavlov) 2012-12-03 01:04:21 PST
Created attachment 177204 [details]
Patch
Comment 5 Build Bot 2012-12-03 01:22:38 PST
Comment on attachment 177204 [details]
Patch

Attachment 177204 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15106388
Comment 6 Alexander Pavlov (apavlov) 2012-12-03 01:57:57 PST
Created attachment 177211 [details]
Patch
Comment 7 Pavel Feldman 2012-12-03 10:58:02 PST
Comment on attachment 177211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177211&action=review

> Source/WebCore/inspector/InspectorPageAgent.cpp:378
> +

accidental change?
Comment 8 Andrey Kosyakov 2012-12-03 11:08:14 PST
Comment on attachment 177211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177211&action=review

> Source/WebCore/inspector/front-end/OverridesView.js:615
> +            if (mediaType === "all") {
> +                // "all" is not a device-specific media type.
> +                continue;
> +            }

Just remove "all" from the array?

> Source/WebCore/inspector/front-end/OverridesView.js:621
> +            if (mediaType === "print")
> +                mediaSelectElement.selectedIndex = i;

this looks a bit fishy... wouldn't it be better to persist the default selection?
Comment 9 Alexander Pavlov (apavlov) 2012-12-04 01:54:46 PST
Comment on attachment 177211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177211&action=review

>> Source/WebCore/inspector/InspectorPageAgent.cpp:378
>> +
> 
> accidental change?

Indeed!

>> Source/WebCore/inspector/front-end/OverridesView.js:615
>> +            }
> 
> Just remove "all" from the array?

I had thought about this at first, but having a "pure-device" set of media types next to the full set thereof sounds bad (in fact, we do need the latter in the CSS tokenizer, so this could be refactored barring one weird value there).

>> Source/WebCore/inspector/front-end/OverridesView.js:621
>> +                mediaSelectElement.selectedIndex = i;
> 
> this looks a bit fishy... wouldn't it be better to persist the default selection?

A nice idea. Implemented.
Comment 10 Alexander Pavlov (apavlov) 2012-12-04 01:59:24 PST
Created attachment 177450 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-12-04 02:18:22 PST
Comment on attachment 177450 [details]
Patch for landing

Rejecting attachment 177450 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ripts/update-webkit line 152.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
From http://git.chromium.org/external/Webkit
   eb9e8cc..96514be  HEAD       -> origin/HEAD
error: Ref refs/remotes/origin/master is at 96514be36664e1430392d9b2fd3200c2aab861f9 but expected eb9e8ccd030f66f658b9fcdabeeb134aaaf82783
 ! eb9e8cc..96514be  master     -> origin/master  (unable to update local ref)
Died at Tools/Scripts/update-webkit line 152.

Full output: http://queues.webkit.org/results/15120776
Comment 12 Alexander Pavlov (apavlov) 2012-12-04 02:26:43 PST
Committed r136493: <http://trac.webkit.org/changeset/136493>
Comment 13 Alexander Pavlov (apavlov) 2013-03-25 03:28:18 PDT
*** Bug 13530 has been marked as a duplicate of this bug. ***