New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

format & fixup Example code #486

Merged
merged 17 commits into from Oct 30, 2017

Conversation

Projects
None yet
5 participants
@Jxck
Copy link
Contributor

Jxck commented Sep 20, 2017

cleanup code example in HTML help us understand features more easily.
so I cleanup some code like below.

  • format indent
  • use basic new feature of ES6^
    • const
    • arrow function
    • async/await
  • remove deprecated scoped css
  • remove js from html
  • simplify but save meaning and semantics

if there are some format rule or feature regulation in spec.
please mention me and I'll fix them.

format & fixup Example code
cleanup code example in HTML help us understand features more easily.
so I cleanup some code like below.

- format indent
- use basic new feature of ES6^
  - const
  - arrow function
  - async/await
- remove deprecated scoped css

if there are some format rule or feature regulation in spec.
please mention me and I'll fix them.
@dontcallmedom

This comment has been minimized.

Copy link
Member

dontcallmedom commented Sep 20, 2017

Marked as non substantive for IPR from ash-nazg.

@aboba aboba requested review from adam-be and jan-ivar Sep 21, 2017

@jan-ivar
Copy link
Contributor

jan-ivar left a comment

I welcome this! Note await is ES2017. That said, all browsers that implement getUserMedia also implement await AFAIK.

@adam-be WDYT?

var supports = navigator.mediaDevices.getSupportedConstraints();
if(!supports["aspectRatio"]) {
const supports = navigator.mediaDevices.getSupportedConstraints();
if (!supports["aspectRatio"]) {

This comment has been minimized.

@jan-ivar

jan-ivar Sep 28, 2017

Contributor

or even simpler:

if (!supports.aspectRatio) {

Applies throughout.

This comment has been minimized.

@Jxck

Jxck Sep 28, 2017

Contributor

also, I using support as additional feature when not exact.
so, How about you think this ?

const constraints = {
  width: 1280,
  height: 720
}
const supports = navigator.mediaDevices.getSupportedConstraints();
if (supports.aspectRatio) {
  constraints.aspectRatio = 1.5
}

This comment has been minimized.

@jan-ivar

jan-ivar Oct 4, 2017

Contributor

That's redundant since unimplemented keys are already ignored.

The sole purpose of getSupportedConstraints() is to support exact really.

This comment has been minimized.

@Jxck

Jxck Oct 5, 2017

Contributor

</pre>
const constraints = {
frameRate: { min: 20 },
width: { min: 640, ideal: 1280 },

This comment has been minimized.

@jan-ivar

jan-ivar Sep 28, 2017

Contributor

Which style guide are we following? Google style {min: 20} or Mozilla style { min: 20 }?

I have no strong opinion (I lean toward the former if anything).

Applies throughout.

This comment has been minimized.

@Jxck

Jxck Sep 28, 2017

Contributor

I'm also working same refuctoring in https://w3c.github.io/webrtc-pc/
and that uses Mozilla Style, so I'm following that.

This comment has been minimized.

@jan-ivar

jan-ivar Oct 4, 2017

Contributor

I'm not sure we should base this decision on which repository you looked at first. ;)

@adam-be Any preference?

This comment has been minimized.

@Jxck

Jxck Oct 5, 2017

Contributor

ok, follow on Google Style.   

height: { min: 480, ideal: 720 },
advanced: [
{ width: 1920, height: 1280 },
{ aspectRatio: 1.3333333333 }

This comment has been minimized.

@jan-ivar

jan-ivar Sep 28, 2017

Contributor

Rather than force people to count decimals, I think we should set a precedent here for using:

{aspectRatio: 4 / 3}

This comment has been minimized.

@Jxck

Jxck Sep 28, 2017

Contributor

ok, same for 1.5 with 3/2

This comment has been minimized.

@Jxck

Jxck Oct 5, 2017

Contributor

  

}]
const constraints = {
width: { min: 640 },
height: { min: 480 },

This comment has been minimized.

@jan-ivar

jan-ivar Sep 28, 2017

Contributor

No mid-sentence space aligning please.

This comment has been minimized.

@Jxck

Jxck Oct 5, 2017

Contributor

  

// Treat like an error.
}
var gotten = navigator.mediaDevices.getUserMedia({
await navigator.mediaDevices.getUserMedia({

This comment has been minimized.

@jan-ivar

jan-ivar Sep 28, 2017

Contributor

Let's include the result for completeness here:

const stream = await navigator.mediaDevices.getUserMedia({

This comment has been minimized.

@Jxck

Jxck Oct 5, 2017

Contributor

  

}).then(gotStream).catch(logError);
startBtn.disabled = true;
}
startBtn.addEventListener('click', async () =&gt; {

This comment has been minimized.

@jan-ivar

jan-ivar Sep 28, 2017

Contributor

I slightly prefer:

startBtn.onclick = async () => {

I have a bias toward brevity, so YMMV. Applies throughout.

This comment has been minimized.

@Jxck

Jxck Sep 28, 2017

Contributor

I agree that name of addEventListener is long enough to make code not brevity.
and spec says about API as onsomething property.

but, I understand that setting handler to onsomething property is old style.

  • can't adding multiple handler
  • there are some new event name which only allows addEventListener (ex, sticky-change but no onsticky-change, DOMContentLoaded too?)

I don't know which is recommended to add Example on Standard Spec,
so I choose better way which I think.

This comment has been minimized.

@jan-ivar

jan-ivar Oct 4, 2017

Contributor

I understand that setting handler to onsomething property is old style.

The mediacapture and WebRTC specs haven't gotten that memo apparently, defining lots of new onsomething properties.

I understand the health benefits, yet at the same time it's not like onclick will ever go away, or can't be used responsibly. But this is why we have editors. @adam-be?

This comment has been minimized.

@Jxck

Jxck Oct 5, 2017

Contributor

ok, use on handler

startBtn.addEventListener('click', async () =&gt; {
try {
startBtn.disabled = true;
const constraint = {

This comment has been minimized.

@jan-ivar

jan-ivar Sep 28, 2017

Contributor

constraints plural, while we're in here.

This comment has been minimized.

@Jxck

Jxck Oct 5, 2017

Contributor

stream.getTracks().forEach((track) =&gt; {
track.onended = () =&gt; {
startBtn.disabled = stream.active;
}

This comment has been minimized.

@jan-ivar

jan-ivar Sep 28, 2017

Contributor

Wow, stream.active hasn't been in the spec for a while! Good we're cleaning this up.

Also, I find it best practice to avoid forEach in async functions. How about:

for (let track of stream.getTracks()) {
  track.onended = () => {
    startBtn.disabled = stream.getTracks().some(t => t.readyState == 'live');
  }
}

This comment has been minimized.

@Jxck

Jxck Sep 28, 2017

Contributor

also follow result for onended/addEL
#486 (comment)

This comment has been minimized.

@jan-ivar

jan-ivar Oct 4, 2017

Contributor

This spec defines onended FWIW.

This comment has been minimized.

@Jxck

Jxck Oct 5, 2017

Contributor

});
try {
const stream = await navigator.mediaDevices.getUserMedia({video: true});
video.srcObject = stream;

This comment has been minimized.

@jan-ivar

jan-ivar Sep 28, 2017

Contributor

Simpler:

video.srcObject = await navigator.mediaDevices.getUserMedia({video: true});

This comment has been minimized.

@Jxck

Jxck Oct 5, 2017

Contributor

const stream = await navigator.mediaDevices.getUserMedia({video: true});
video.srcObject = stream;
video.addEventListener('loadedmetadata', () =&gt; {
canvas.width = video.videoWidth;

This comment has been minimized.

@jan-ivar

jan-ivar Sep 28, 2017

Contributor

We can streamline this now:

await new Promise(resolve => video.onloadedmetadata = resolve);
canvas.width = video.videoWidth;
etc.

This comment has been minimized.

@Jxck

Jxck Sep 28, 2017

Contributor

hum, that code is not equiv with example.
onsomthing describes that calls handler multiple (if calls only once, could change to return promise), but wrapping Promise cares only first time and onsomething handler resolves already resolved promise after that.
if this example intended to 'handle event only once at very first', removeEventLister/clear onsomething immediately but this example doesn't do that currently.

so, we Can streamline this, but it seems not for me that we Need to do.

This comment has been minimized.

@jan-ivar

jan-ivar Oct 4, 2017

Contributor

The purpose here is to read out the width and height of the stream. We can't read that out immediately, so we wait for a single deterministic loadedmetadata event we trigger above in:

video.srcObject = stream;

In light of this, the promise version seems more robust with fewer side-effects than the old code.

This comment has been minimized.

@Jxck

Jxck Oct 5, 2017

Contributor

@adam-be

This comment has been minimized.

Copy link
Member

adam-be commented Oct 3, 2017

@Jxck, are you planning to update the patch based on @jan-ivar's comments?

@Jxck

This comment has been minimized.

Copy link
Contributor

Jxck commented Oct 3, 2017

@adam-be Yes, but I'm waiting for response to comment from @jan-ivar .

@jan-ivar

This comment has been minimized.

Copy link
Contributor

jan-ivar commented Oct 4, 2017

Hi @Jxck sorry for the late response. See replies above.

@Jxck

This comment has been minimized.

Copy link
Contributor

Jxck commented Oct 5, 2017

thanks @jan-ivar, I fixed up PR.

@Jxck Jxck referenced this pull request Oct 5, 2017

Merged

format & fixup Example code #1618

@stefhak

This comment has been minimized.

Copy link
Contributor

stefhak commented Oct 19, 2017

@jan-ivar do you think this is mergeable now?
@adam-be can you please take a look?

@jan-ivar
Copy link
Contributor

jan-ivar left a comment

Beautiful.

for (let track of stream.getTracks()) {
track.onended = () =&gt; {
startBtn.disabled = stream.getTracks().some(t =&gt; t.readyState == 'live');
}

This comment has been minimized.

@jan-ivar

jan-ivar Oct 19, 2017

Contributor

lost a ; here (unless you wanna lose the {} instead)

This comment has been minimized.

@Jxck

Jxck Oct 23, 2017

Contributor

solved


shutter.onclick = () =&gt; {
canvas.getContext('2d').drawImage(video, 0, 0);
};

This comment has been minimized.

@jan-ivar

jan-ivar Oct 19, 2017

Contributor

This would fit on a single line, just sayin'

Jxck added some commits Oct 20, 2017

@adam-be

This comment has been minimized.

Copy link
Member

adam-be commented Oct 24, 2017

Very nice PR!

I made a little node script that extracts all the example snippets from the spec and runs eslint on them. Using Google style as a base, this PR only has a few minor style issues (if we disregard from the double quotes :)). In most cases I like spaces between curly braces and the content and w3c/webrtc-pc#1618 still has a lot of them. Whichever way we go I think we should be consistent between mediacapture-main and webrtc-pc.

@adam-be

This comment has been minimized.

Copy link
Member

adam-be commented Oct 24, 2017

Script output:

Checking ../mediacapture-main/getusermedia.html
Example_1: Passed
Example_2: Passed
Example_3: Passed
Example_4: Passed
Example_5: Passed
Example_6: Passed
Example_7: Passed
Example_8: Passed
Example_9: Passed
=== Example_10 ===

01  temp = {
02    frameRate: {min: 1.0, max: 60.0},
03    facingMode: [ "user", "left" ]
04  };
=== Linter output ===

/home/eadaber/repos/w3c/check-examples/Example_10.js
  3:15  error  There should be no space after '['   array-bracket-spacing
  3:32  error  There should be no space before ']'  array-bracket-spacing

✖ 2 problems (2 errors, 0 warnings)
  2 errors, 0 warnings potentially fixable with the `--fix` option.


Example_10: Linter process exited with code 1

Example_11: Passed
Example_12: Passed
=== Example_13 ===

01  <button id="startBtn">Start</button>
02  <script>
03  const startBtn = document.getElementById('startBtn');
04  
05  startBtn.onclick = async () => {
06    try {
07      startBtn.disabled = true;
08      const constraints = {
09        audio: true,
10        video: true
11      };
12  
13      const stream = await navigator.mediaDevices.getUserMedia(constraints);
14  
15      for (let track of stream.getTracks()) {
16        track.onended = () => {
17          startBtn.disabled = stream.getTracks().some(t => t.readyState == 'live');
18        }
19      };
20    } catch(err) {
21      console.error(err)
22    }
23  };
24  </script>
=== Linter output ===

/home/eadaber/repos/w3c/check-examples/Example_13.html
  17:53  error  Expected parentheses around arrow function argument  arrow-parens
  18:8   error  Missing semicolon                                    semi
  20:5   error  Expected space(s) after "catch"                      keyword-spacing
  21:23  error  Missing semicolon                                    semi

✖ 4 problems (4 errors, 0 warnings)
  4 errors, 0 warnings potentially fixable with the `--fix` option.


Example_13: Linter process exited with code 1

=== Example_14 ===

01  <script>
02  window.onload = async () => {
03    const video = document.getElementById('monitor');
04    const canvas = document.getElementById('photo');
05    const shutter = document.getElementById('shutter');
06  
07    try {
08      video.srcObject = await navigator.mediaDevices.getUserMedia({video: true});
09  
10      await new Promise(resolve => video.onloadedmetadata = resolve);
11      canvas.width = video.videoWidth;
12      canvas.height = video.videoHeight;
13      document.getElementById('splash').hidden = true;
14      document.getElementById('app').hidden = false;
15  
16      shutter.onclick = () => canvas.getContext('2d').drawImage(video, 0, 0);
17    } catch (err) {
18      console.error(err)
19    }
20  };
21  </script>
22  
23  <h1>Snapshot Kiosk</h1>
24  
25  <section id="splash">
26    <p id="errorMessage">Loading...</p>
27  </section>
28  
29  <section id="app" hidden>
30    <video id="monitor" autoplay></video>
31    <button id="shutter">&#x1F4F7;</button>
32    <canvas id="photo"></canvas>
33  </section>
=== Linter output ===

/home/eadaber/repos/w3c/check-examples/Example_14.html
  10:23  error  Expected parentheses around arrow function argument  arrow-parens
  18:23  error  Missing semicolon                                    semi

✖ 2 problems (2 errors, 0 warnings)
  2 errors, 0 warnings potentially fixable with the `--fix` option.


Example_14: Linter process exited with code 1

@adam-be

This comment has been minimized.

Copy link
Member

adam-be commented Oct 24, 2017

I pushed the script to the webrtc-pc repo [1] if you want to try it out. As said above, the rules are based on Googel's js coding style with some exceptions. I think it would be nice to move to single quotes and skip skipping that rule.

[1] w3c/webrtc-pc@0547efa

@Jxck

This comment has been minimized.

Copy link
Contributor

Jxck commented Oct 24, 2017

@adam-be I think it's better to separate Example scripts from html to each JS file for easy maintenance.
and then, run lint-then-merge when running build task, instead of extracts script from html and run lint.

actually, it's already hard for me to handling this PR ... :(

if this accepted, please merge this PR with fixing non-style problem.
and I'll made new other PR for

  • separate exampleNN.js from getusermedia.js
  • adding task to lint then with your scripts (thanks)
  • adding task to merge them into html

how about this proposal ?

@adam-be

This comment has been minimized.

Copy link
Member

adam-be commented Oct 25, 2017

Regardless how we do it, I think it's time to merge this PR and continue to work upstream.

@adam-be
Copy link
Member

adam-be left a comment

This looks good. Depending on the style we decide on there are some nits that we can fix later.

@Jxck

This comment has been minimized.

Copy link
Contributor

Jxck commented Oct 25, 2017

OK, I'll push when I finish current work.
and continue with other PR for separate/lint.

@adam-be

This comment has been minimized.

Copy link
Member

adam-be commented Oct 25, 2017

If you have more review fixes to this PR, push them. We have a editor's meeting on Thursday afternoon (CEST), then we'll talk about how to manage the examples. Until then I think you can wait with the spec/examples split patch.

@Jxck

This comment has been minimized.

Copy link
Contributor

Jxck commented Oct 25, 2017

@adam-be I've done. merge or handle in meeting please.

@adam-be

This comment has been minimized.

Copy link
Member

adam-be commented Oct 25, 2017

Thank you @Jxck

@adam-be

This comment has been minimized.

Copy link
Member

adam-be commented Oct 27, 2017

I'll fix some style issues and merge this PR later today.

@adam-be adam-be merged commit cb3f0fc into w3c:master Oct 30, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ipr PR deemed acceptable as non-substantive by @dontcallmedom.
Details

@Jxck Jxck deleted the Jxck:patch-1 branch Oct 30, 2017

@adam-be

This comment has been minimized.

Copy link
Member

adam-be commented Oct 30, 2017

@Jxck, regarding splitting the examples from the spec; thanks for offering to do this change but we would like to keep the examples in the spec.

@Jxck

This comment has been minimized.

Copy link
Contributor

Jxck commented Oct 30, 2017

Can I ask Why ? (you can see my work was hard because of that, and will it continue ?).

@adam-be

This comment has been minimized.

Copy link
Member

adam-be commented Oct 30, 2017

Mainly added complexity vs. gain. I think you have done a great job and improved the examples a lot, but it's not that common that we need to be editing all examples at the same time. I have done some reformatting and updated the example coding style to fairly match Google's. I hope you will find them easier to work with from now on.

@Jxck

This comment has been minimized.

Copy link
Contributor

Jxck commented Oct 31, 2017

then, please add https://github.com/w3c/webrtc-pc/tree/master/check-examples to mediacapture too and write how to check example to both CONTRIBUTING.md for avoid styling bikeshed.

@adam-be

This comment has been minimized.

Copy link
Member

adam-be commented Oct 31, 2017

I would rather put the check-examples script somewhere else than duplicate it. I'll look into that. Good point putting some instructions in the CONTRIBUTING.md file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment