1066101 - Responsive Design View uses incorrect dates in screenshot filename

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect)

Description

[Chris Adams]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140825202822

Steps to reproduce:

Open the responsive design view
Click on the screenshot button


Actual results:

The filename used a date 6 days in the past: 

"Screen Shot 2014-09-05 at 10.50.46"


Expected results:

It should have used the current date, as the new full-page screenshot does:

"Screen Shot 2014-09-11 at 10.50.46"

Comment 1

[Chris Adams]

JavaScript Date() API horribleness strikes again:

http://dxr.mozilla.org/mozilla-central/source/browser/devtools/responsivedesign/responsivedesign.jsm#775

      let day = ("0" + (date.getDay() + 1)).substr(-2, 2);

getDay() actually returns the day of the week. That should be:

      let day = ("0" + (date.getDate())).substr(-2, 2);

Comment 2

[Chris Adams]

Attachment: Use getDate() instead of getDay() in generated filename

Comment 3

[:Gijs (he/him)]

Awesome, thanks for the patch! Let me find someone who can review this for you... :-)

Comment 4

[:Gijs (he/him)]

Comment on attachment 8487975 [details] [diff] [review]
Use getDate() instead of getDay() in generated filename

Review of attachment 8487975 [details] [diff] [review]:
-----------------------------------------------------------------

Mike, you reviewed the original code here, so... :-)

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8487975 [details] [diff] [review]
Use getDate() instead of getDay() in generated filename

Review of attachment 8487975 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

The change looks good to me.  However, could you attach an updated one with the right commit message format[1]?

Flag me for review? again on the new patch, and then I'll push it to Try to run tests.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 6

[Chris Adams]

Attachment: fix-screenshot-filename.patch

Reformatted to follow https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions more closely

Comment 7

[Chris Adams]

I just uploaded a version with what appears to be the right format. Please let me know if there's something I missed.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8490084 [details] [diff] [review]
fix-screenshot-filename.patch

Review of attachment 8490084 [details] [diff] [review]:
-----------------------------------------------------------------

One thing to fix.  Be sure to set "review?" to my email address next time to be sure I am notified of your new patch version.

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +769,5 @@
>      let filename = aFileName;
>      if (!filename) {
>        let date = new Date();
>        let month = ("0" + (date.getMonth() + 1)).substr(-2, 2);
> +      let day = ("0" + (date.getDate()).substr(-2, 2);

I didn't notice late time around... but you now have unbalanced parentheses.  Changing "(date" to "date" seems like the right fix.

Comment 9

[Chris Adams]

Attachment: fix-screenshot-filename.patch

Comment 10

[Chris Adams]

(In reply to J. Ryan Stinnett [:jryans] from comment #8)
> I didn't notice late time around... but you now have unbalanced parentheses.
> Changing "(date" to "date" seems like the right fix.

Sorry about that, I'd corrected that locally but made the mistake of using a local commit rather than updating the patch queue. I just uploaded a corrected version.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8490247 [details] [diff] [review]
fix-screenshot-filename.patch

Review of attachment 8490247 [details] [diff] [review]:
-----------------------------------------------------------------

No worries, looks good now!

Even though this seems like a pretty safe change, I'll push this to our test suite now to make sure.

If everything looks green there, you can then set this bug's keyword field to "checkin-needed" and someone will land your patch for you.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=013fe95e2de6

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Tests look green, marking for checkin.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/2170a92dad71

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2170a92dad71

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Thanks again for the fix!  Take a look at our other mentored bugs[1] if you'd like to work on some more.

[1]: https://wiki.mozilla.org/DevTools/GetInvolved#Mentored_and_Good_First_Bugs