Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(250)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1058723002: Remove testRunner.injectStyleSheet usage from layout tests. (Closed)

Created:
5 years, 8 months ago by dcheng
Modified:
5 years, 8 months ago
Reviewers:
pfeldman, ojan
CC:
aandrey+blink_chromium.org, apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, devtools-reviews_chromium.org, esprehn, kenneth.r.christiansen, kozyatinskiy+blink_chromium.org, loislo+blink_chromium.org, lushnikov+blink_chromium.org, malch+blink_chromium.org, Mikhail, pfeldman+blink_chromium.org, rune, sergeyv+blink_chromium.org, yurys+blink_chromium.org
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Remove testRunner.injectStyleSheet usage from layout tests. BUG= Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193155

Patch Set 1 #

Total comments: 8

Patch Set 2 : Remove one more file #

Patch Set 3 : Update inspector UA stylesheet test #

Patch Set 4 : Attempt to fix git metadata weirdness #

Patch Set 5 : Another try (intermediate patch) #

Patch Set 6 : And then re-add the changed UA stylesheet tests #

Patch Set 7 : Revert deletions #

Patch Set 8 : Moo #

Patch Set 9 : Update baseline #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+32 lines, -168 lines) Patch
M LayoutTests/css3/device-adapt/viewport-height-extend-to-zoom.html View 1 chunk +0 lines, -9 lines 2 comments Download
M LayoutTests/css3/device-adapt/viewport-initial-height-zero.html View 1 chunk +0 lines, -3 lines 0 comments Download
M LayoutTests/css3/device-adapt/viewport-initial-height-zero-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/css3/device-adapt/viewport-initial-width-zero.html View 1 chunk +0 lines, -3 lines 0 comments Download
M LayoutTests/css3/device-adapt/viewport-initial-width-zero-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/css3/device-adapt/viewport-insert-rule-after.html View 1 chunk +0 lines, -4 lines 0 comments Download
M LayoutTests/css3/device-adapt/viewport-insert-rule-after-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/css3/device-adapt/viewport-insert-rule-before.html View 1 chunk +0 lines, -3 lines 0 comments Download
M LayoutTests/css3/device-adapt/viewport-insert-rule-before-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/css3/device-adapt/viewport-min-width-extend-to-zoom.html View 1 chunk +1 line, -10 lines 0 comments Download
M LayoutTests/css3/device-adapt/viewport-relative-font.html View 1 chunk +0 lines, -1 line 0 comments Download
M LayoutTests/css3/device-adapt/viewport-relative-font-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/css3/device-adapt/viewport-user-agent-style.html View 1 chunk +1 line, -3 lines 0 comments Download
D LayoutTests/css3/device-adapt/viewport-user-style.html View 1 chunk +0 lines, -53 lines 0 comments Download
M LayoutTests/css3/device-adapt/viewport-width-extend-to-zoom.html View 1 chunk +0 lines, -9 lines 0 comments Download
M LayoutTests/inspector/elements/styles/inject-stylesheet.html View 1 2 3 4 5 6 7 3 chunks +10 lines, -10 lines 0 comments Download
M LayoutTests/inspector/elements/styles/inject-stylesheet-expected.txt View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/inspector/elements/styles/resources/inject-stylesheet-iframe-data.html View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/inspector/elements/styles/styles-disable-then-enable-overriden-ua.html View 1 2 5 3 chunks +7 lines, -25 lines 0 comments Download
M LayoutTests/inspector/elements/styles/styles-disable-then-enable-overriden-ua-expected.txt View 1 2 5 3 chunks +6 lines, -15 lines 0 comments Download
M LayoutTests/printing/page-rule-selection.html View 1 chunk +0 lines, -8 lines 0 comments Download
M LayoutTests/printing/page-rule-selection-expected.txt View 1 2 1 chunk +0 lines, -6 lines 0 comments Download

Messages

Total messages: 26 (6 generated)
dcheng
I've CCed authors/reviewers of the @viewport tests as well, in case they have any comments. ...
5 years, 8 months ago (2015-04-02 04:41:18 UTC) #2
pfeldman
https://codereview.chromium.org/1058723002/diff/1/LayoutTests/inspector/elements/styles/inject-stylesheet.html File LayoutTests/inspector/elements/styles/inject-stylesheet.html (left): https://codereview.chromium.org/1058723002/diff/1/LayoutTests/inspector/elements/styles/inject-stylesheet.html#oldcode22 LayoutTests/inspector/elements/styles/inject-stylesheet.html:22: iframe.src = "resources/inject-stylesheet-iframe-data.html"; This resource should also be deleted. ...
5 years, 8 months ago (2015-04-02 05:34:12 UTC) #4
dcheng
https://codereview.chromium.org/1058723002/diff/1/LayoutTests/inspector/elements/styles/inject-stylesheet.html File LayoutTests/inspector/elements/styles/inject-stylesheet.html (left): https://codereview.chromium.org/1058723002/diff/1/LayoutTests/inspector/elements/styles/inject-stylesheet.html#oldcode22 LayoutTests/inspector/elements/styles/inject-stylesheet.html:22: iframe.src = "resources/inject-stylesheet-iframe-data.html"; On 2015/04/02 at 05:34:12, pfeldman wrote: ...
5 years, 8 months ago (2015-04-02 05:39:12 UTC) #5
pfeldman
https://codereview.chromium.org/1058723002/diff/1/LayoutTests/inspector/elements/styles/styles-disable-then-enable-overriden-ua.html File LayoutTests/inspector/elements/styles/styles-disable-then-enable-overriden-ua.html (left): https://codereview.chromium.org/1058723002/diff/1/LayoutTests/inspector/elements/styles/styles-disable-then-enable-overriden-ua.html#oldcode1 LayoutTests/inspector/elements/styles/styles-disable-then-enable-overriden-ua.html:1: <html> > I don't think it's worth keeping around ...
5 years, 8 months ago (2015-04-02 05:46:16 UTC) #6
dcheng
OK, updated the UA stylesheet test. Does it look reasonable to you? It depends on ...
5 years, 8 months ago (2015-04-02 06:14:33 UTC) #7
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1058723002/40001
5 years, 8 months ago (2015-04-02 06:25:25 UTC) #9
pfeldman
devtools lgtm. note that the ua override tests are still marked for deletion (D) not ...
5 years, 8 months ago (2015-04-02 06:34:16 UTC) #10
commit-bot: I haz the power
Try jobs failed on following builders: blink_presubmit on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/30348)
5 years, 8 months ago (2015-04-02 06:34:48 UTC) #12
dcheng
On 2015/04/02 at 06:34:16, pfeldman wrote: > devtools lgtm. note that the ua override tests ...
5 years, 8 months ago (2015-04-02 06:41:22 UTC) #13
pfeldman
Lets not land it and consider exposing new injection way via internals as I am ...
5 years, 8 months ago (2015-04-02 09:40:39 UTC) #14
dcheng
pfeldman: PTAL. I've updated the devtool style injection test to use the proposed method in ...
5 years, 8 months ago (2015-04-02 15:54:29 UTC) #15
ojan
https://codereview.chromium.org/1058723002/diff/150001/LayoutTests/css3/device-adapt/viewport-height-extend-to-zoom.html File LayoutTests/css3/device-adapt/viewport-height-extend-to-zoom.html (left): https://codereview.chromium.org/1058723002/diff/150001/LayoutTests/css3/device-adapt/viewport-height-extend-to-zoom.html#oldcode17 LayoutTests/css3/device-adapt/viewport-height-extend-to-zoom.html:17: testRunner.injectStyleSheet("@viewport { width: -internal-extend-to-zoom 980px; min-zoom: 0.25; max-zoom: 5; ...
5 years, 8 months ago (2015-04-02 18:19:49 UTC) #16
dcheng
https://codereview.chromium.org/1058723002/diff/150001/LayoutTests/css3/device-adapt/viewport-height-extend-to-zoom.html File LayoutTests/css3/device-adapt/viewport-height-extend-to-zoom.html (left): https://codereview.chromium.org/1058723002/diff/150001/LayoutTests/css3/device-adapt/viewport-height-extend-to-zoom.html#oldcode17 LayoutTests/css3/device-adapt/viewport-height-extend-to-zoom.html:17: testRunner.injectStyleSheet("@viewport { width: -internal-extend-to-zoom 980px; min-zoom: 0.25; max-zoom: 5; ...
5 years, 8 months ago (2015-04-02 18:22:43 UTC) #17
dcheng
ping!
5 years, 8 months ago (2015-04-03 18:20:56 UTC) #18
ojan
lgtm, although, it seems better to me to just delete these tests if I'm understanding ...
5 years, 8 months ago (2015-04-03 23:22:59 UTC) #19
dcheng
On 2015/04/03 at 23:22:59, ojan wrote: > lgtm, although, it seems better to me to ...
5 years, 8 months ago (2015-04-04 00:00:53 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1058723002/150001
5 years, 8 months ago (2015-04-04 00:01:24 UTC) #23
commit-bot: I haz the power
Committed patchset #9 (id:150001) as https://src.chromium.org/viewvc/blink?view=rev&revision=193155
5 years, 8 months ago (2015-04-04 02:28:44 UTC) #24
kenneth.christiansen
On 2015/04/04 00:00:53, dcheng wrote: > On 2015/04/03 at 23:22:59, ojan wrote: > > lgtm, ...
5 years, 8 months ago (2015-04-07 11:43:32 UTC) #25
kenneth.christiansen
5 years, 8 months ago (2015-04-07 11:43:35 UTC) #26
Message was sent while issue was closed. 
On 2015/04/04 00:00:53, dcheng wrote:
> On 2015/04/03 at 23:22:59, ojan wrote:
> > lgtm, although, it seems better to me to just delete these tests if I'm
> understanding the feature correctly.
> > 
> > As per the change description in that patch, "This new feature is enabled
only
> for UA and user style sheets." We no longer support user style sheets. I don't
> see it used in our UA style sheet. Can we just remove the feature and it's
tests
> entirely?
> 
> Maybe? http://dev.w3.org/csswg/css-device-adapt/#extend-to-zoom indicates it's
a
> UA internal value, but the test results didn't change at all when I removed
the
> style injection block. I'm not familiar enough with the feature to tell if
it's
> broken, working as intended, or something else, so I'll follow up in a
separate
> bug.

It was basically a way to change the default of @viewport and viewport meta
tags. Whether it is needed by default, I am not sure. I should CC rune and bokan
to this bug

Powered by Google App Engine
This is Rietveld 408576698