Support browser screen dimensions parameters

[alshakero]

Note: You may prefer to review #31 instead. It includes this PR

Depends on #17, related to p3topS-Ia-p2.

This PR adds the support for screenWidth and screenHeight params. This allows for screenshotting long/short/wide or whatever dimensions the user needs. Both new params are optional hence backward compatible.

The first commit is a failing test on purpose. The follow-up commit is the fix to the failing test (the implementation). I used TDD to make the requirement easier to understand and easier to see that it's backward compatible.

To test

The closest I could get to simulating the deployed version is starting the node server. There is another layer of abstraction in PHP but I couldn't get that running.

1. Git clone this and run npm install.
2. Create these dirs /opt/mshots/public_html/thumbnails/. By running sudo mkdir -p /opt/mshots/public_html/thumbnails/.
3. Then make them writable (sudo chmod 777 /opt/mshots/public_html/thumbnails/). Don't worry, you can delete these after you're done.
4. Run the Node daemon script by doing (node lib/mshots.js -p 7000 -n 2)
5. Access http://localhost:7000/queue?url=https://wordpress.com/&f=/opt/mshots/public_html/thumbnails/875/8751a27a0b7ea911b1fd202d34602123df995951/7be4fb73894606a68035c8fc9c79cce4.jpg
6. Wait for a ~5 seconds then view the image by running open /opt/mshots/public_html/thumbnails/875/8751a27a0b7ea911b1fd202d34602123df995951/7be4fb73894606a68035c8fc9c79cce4.jpg.

You should see a default-sized image: 1280x960px.

Now, you can try whatever combination you see fit of these params (screen_width, screen_height, vpw, vph) :

http://localhost:7000/queue?url=https://wordpress.com/&f=/opt/mshots/public_html/thumbnails/875/8751a27a0b7ea911b1fd202d34602123df995951/7be4fb73894606a68035c8fc9c79cce4.jpg&screen_width=1920&screen_height=5000&vpw=1920&vph=1080

After every test, to see the image, you can run

open /opt/mshots/public_html/thumbnails/875/8751a27a0b7ea911b1fd202d34602123df995951/7be4fb73894606a68035c8fc9c79cce4.jpg

Once you're done. Feel free to delete the folders:

sudo rm -rf /opt/mshots/public_html/thumbnails/

[deBhal]

Looks great except for one concern with the caching.

This service is used in core and Akismet, so we need to make sure not to trigger a recache of everything.

Totally not a blocker, but I'd lean towards something like "clip height" or "image_height" for the name of the new parameter. There doesn't seem to be a really good name for it, though, and screen_height will work for my needs, so I'm happy if you're happy.

[deBhal]

We need to make sure we only update this url if these values are non-defualt so as to avoid re-caching all the old images.

[alshakero]

This service is used in core and Akismet, so we need to make sure not to trigger a recache of everything.

Great point! Addressed.

There doesn't seem to be a really good name for it, though, and screen_height will work for my needs

I agree that screen_height is not great. But I think it's OK, I'm crossing my fingers that users will see that it's viewport vs screen. Image might be more accurate since there is no screen, but I think it will convey less information about the function of the parameter. Screen might be technically wrong, but it conveys more info IMHO.

My only concern now is how to test this before merging, I couldn't run the whole thing (PHP and Node) locally. Do you have any ideas?

[andrewserong]

@alshakero and @deBhal I've made an unsuccessful attempt at running mShots in Docker for a local development environment. Unfortunately I haven't gotten very far, but in case either of you would like to take a look / experiment with it, I've pushed it to a draft PR at: #22 — I'm on rotation next week, so won't have a chance to continue chipping away at it, but feel free to dive in if you have the time or inclination :)

[deBhal]

Nice! I was just thinking that containerising the service might simplify the process a lot (especially testing), but I wasn't sure how hard the containerisation itself would be.

[andrewserong]

Thanks @deBhal! It is a little fiddly (and obviously not quite working just yet)... I'm not sure if this would help, but I was thinking over the weekend that maybe adding a docker-compose file and running multiple containers might making things a bit clearer, too. E.g. have a puppeteer service container that exposes port 7777 and runs mshots_ctl.sh start as its entrypoint/command, and another container that runs the PHP over port 80 (or 8080/8000 or whichever suits). Feel free to change anything you like about that PR, it was really just where I got to with my exploration before wrapping up for the week!

[andrewserong]

Related to this change, I've put together a similar PR to add a quality query param. WIP here: #23

[deBhal]

I was retesting this PR to and ran into a bit of awkwardness - the clip parameter we're using doesn't respect the page height, so you end up with blank whiteness below the bottom of the page. This is only a little worse than what was already happening with a large viewport, it's mostly our tall-viewport use-cases that make it look worse.

Regardless, we can fix it by pulling the clientHeight out of the browser, e.g. for http://localhost:7000/queue?url=https%3A%2F%2Fpublic-api.wordpress.com%2Frest%2Fv1%2Ftemplate%2Fdemo%2Fmayland%2Fvesta%3Ffont_base%3DRaleway%26font_headings%3DCabin%26site_title%3DVesta&screen_height=3200&f=/opt/mshots/public_html/thumbnails/ed2/ed271f0f0255e9d784e345dc2f0d8cc48ca26019/c54a96223dc6e94997eca44b9b4af8fa.jpg:

[deBhal]

The test failures were being caused by the default css body { margin: 8px; }. I've switched the size limitation checks from the body element to the document element to fix that.

A consequence of that was also having to switch from clientHeight to scrollHeight to get the same value. I found this a little unintuitive at first - I didn't realize that the viewport was the same thing as the size of the documentElement, but I guess that's a pretty reasonable place for the "outside" and "inside" of the browser to meet.

[deBhal]

Closing: I merged these changes as part of #31