Closed Bug 1402930 Opened 7 years ago Closed 7 years ago

Changing the DPR value should impact images with srcset attribute

Categories

(DevTools :: Responsive Design Mode, defect, P2)

Product:
DevTools
Component:
Responsive Design Mode
Type:
defect

Tracking

(firefox57 wontfix, firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: zer0, Assigned: jryans)

References

Details

Attachments

(2 files)

Bug 1402930 - Use the PresContext's override dppx value if set;
59 bytes, text/x-review-board-request
bradwerth
: review+
Details
Bug 1402930 - Added Unit Test for Image's srcset scenario;
59 bytes, text/x-review-board-request
bradwerth
: review+
Details 
STR:

1. Open <https://webkit.org/demos/srcset/>
2. Toggle the RDM (Cmd+M / Ctrl+M)
3. Change the DPR from the dropdown

Expected result:

The images displayed in the page is changed as well in accordance with the density specified in the srcset attribute.

Actual result:

Nothing happens.
Assignee: nobody → zer0
Comment on attachment 8911908 [details]
Bug 1402930 - Added Unit Test for Image's srcset scenario;

https://reviewboard.mozilla.org/r/183302/#review188842

Looks good. Commit comments should start with "Bug" and not "Byg".
Attachment #8911908 - Flags: review?(bwerth) → review+
Comment on attachment 8911907 [details]
Bug 1402930 - Use the PresContext's override dppx value if set;

https://reviewboard.mozilla.org/r/183300/#review188838
Attachment #8911907 - Flags: review?(bwerth) → review+
(In reply to Brad Werth [:bradwerth] from comment #3)
> Comment on attachment 8911908 [details]
> Bug 1402930 - Added Unit Test for Image's srcset scenario;
> 
> https://reviewboard.mozilla.org/r/183302/#review188842
> 
> Looks good. Commit comments should start with "Bug" and not "Byg".

Thanks! I didn't notice the typo, ah ah ah, apparently MozReview was smart enough to understand the meaning anyway. :) Thanks for the catch!
Here the try build, just in case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be8523cc3d68b45078036043cb8dbb0d942354f2

Brad, let me know if the try message I set is fine, or it would be better run a different one.
Flags: needinfo?(bwerth)
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #8)
> Brad, let me know if the try message I set is fine, or it would be better
> run a different one.

You can certainly run a smaller set of tests to check your new test. Running as few platforms/tests as possible saves our build server resources. For mochitest changes I make that I believe are not platform-specific (like this one) I run on linux64 debug, and only the core mochitests. The try chooser syntax looks like:

try: -b d -p linux64 -u mochitest-1,mochitest-2,mochitest-3,mochitest-4,mochitest-5,mochitest-6,mochitest-7,mochitest-8,mochitest-9,mochitest-10 -t none

If you need to tweak the test, then that's what I suggest you run in future try runs.
Flags: needinfo?(bwerth)
This appears to be ready to land, according to try.  I'll try telling MozReview to land it.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4edb91f3fbbe
Use the PresContext's override dppx value if set; r=bradwerth
https://hg.mozilla.org/integration/autoland/rev/13a81548a728
Added Unit Test for Image's srcset scenario; r=bradwerth
Backed out for failing modified mochitest dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html on Android:

https://hg.mozilla.org/integration/autoland/rev/e8fca5a1955a6d3ba069643897d05ef4a94704da

Push which ran failing tests: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6a9013a830e9c039a0a938d2402d4174e2606e49&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139580605&repo=autoland

[task 2017-10-25T18:56:40.517Z] 18:56:40     INFO -  250 INFO TEST-PASS | dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html | Image url is properly set for 1.5dppx.
[task 2017-10-25T18:56:40.517Z] 18:56:40     INFO -  251 INFO override window's dppx to 2
[task 2017-10-25T18:56:40.518Z] 18:56:40     INFO -  252 INFO TEST-PASS | dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html | Image is properly set for 2dppx.
[task 2017-10-25T18:56:40.518Z] 18:56:40     INFO -  253 INFO restore window's dppx to default value
[task 2017-10-25T18:56:40.518Z] 18:56:40     INFO -  Buffered messages finished
[task 2017-10-25T18:56:40.519Z] 18:56:40     INFO -  254 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html | Test timed out.
[task 2017-10-25T18:56:40.519Z] 18:56:40     INFO -      reportError@SimpleTest/TestRunner.js:121:7
[task 2017-10-25T18:56:40.520Z] 18:56:40     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
Flags: needinfo?(zer0)
Flags: needinfo?(zer0) → needinfo?(jryans)
I've spent some time on this locally.  The test appears to hit a race where the 1x image sends 2 load events.  Not yet sure of the details, but I'll keep digging.
Flags: needinfo?(jryans)
Assignee: zer0 → jryans
Okay, it's not actually a race in image loading.  The new test block would sometimes get a load because the image was affected by dppx changes from the previous test blocks.  I avoided this by only setting the image srcset at the start of the new test block.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/293a4ab2d987
Use the PresContext's override dppx value if set; r=bradwerth
https://hg.mozilla.org/integration/autoland/rev/7f51f5b71274
Added Unit Test for Image's srcset scenario; r=bradwerth
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: