Skip to content
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

Restrict status reporting for wake lock #125

Conversation

Projects
None yet
4 participants
@andrey-logvinov
Copy link
Collaborator

commented Apr 18, 2018

@andrey-logvinov andrey-logvinov requested a review from marcoscaceres Apr 18, 2018

@andrey-logvinov

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 18, 2018

Added the restriction that the global wake lock status is reported to the document only if the document itself requests the wake lock. This is to prevent spying on wake lock actions by other isolated contexts. Presumably, a document should not be interested in wake lock status unless it has requested the wake lock.

@andrey-logvinov andrey-logvinov requested a review from dontcallmedom Apr 18, 2018

@marcoscaceres

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

Running tidy and rebasing should fix the conflicts.

@marcoscaceres
Copy link
Member

left a comment

We might need to add a state machine.

index.html Outdated
<li data-tests="wakelock-cancel-twice.https.html">If the <a>cancel()</a> method has already been invoked on this
object, abort these steps.
<li data-tests="wakelock-cancel-twice.https.html">If the
<a>cancel()</a> method has already been invoked on this object, abort

This comment has been minimized.

Copy link
@marcoscaceres

marcoscaceres Jul 10, 2018

Member

We probably want a proper state machine here (i.e., internal slot to track this) - and we probably want to throw an InvalidStateError if called twice.

@andrey-logvinov

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2018

@marcoscaceres what version of tidy did you use and with which arguments? I tried building it from htacg/tidy-html5 both from "master" and "next" branches and I didn't get the changes you introduced in 8ebd85e. I used command line "tidy -m -config tidyconfig.txt index.html". Did you perhaps make some changes to tidyconfig.txt too?

@marcoscaceres

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

hey, using: HTML Tidy for Apple macOS version 5.6.0

with: tidy -config tidyconfig.txt -o index.html index.html

No changes to config. I can send a commit on top of yours.

@marcoscaceres

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

Looking at the conflicts, they seem to be legit, and not tidy related. There is only two. The first seems to be fine to take from HEAD. But I was unsure about the second:

<<<<<<< HEAD
      <ol>
        <li data-tests="wakelock-type.https.html">If <a>WakeLock</a> object's
        <a data-lt="WakeLock.type">type</a> attribute is not equal to
        <var>type</var>, abort these steps.
        </li>
        <li data-tests="wakelock-onactivechange.https.html">
          <a>Queue a task</a> which updates the <a>WakeLock</a> object's
          <code>active</code> attribute and <a data-lt="fire an event">fires an
          event</a> named <code>activechange</code> at the <a>WakeLock</a>
          object.
        </li>
      </ol>
=======
>>>>>>> Restrict status reporting for wake lock

Did you remove that text on purpose or is it intended to stay?

@andrey-logvinov

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2018

Now I understand, I already ran tidy before I submitted this PR doing the same changes as you did in your commit so I'm not getting any new changes now. But in your commit there are also changes to the deleted section hence the conflict. Yes, the removal is intentional, now the event is handled in "update reported wake lock status".

@marcoscaceres

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

Ok, cool. You can probably just fix them straight from Github's UI.

@andrey-logvinov andrey-logvinov force-pushed the andrey-logvinov:restrict_status_reporting branch from 838810b to 1e0d86a Jul 10, 2018

@andrey-logvinov

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2018

Just found out that Yandex W3C membership has ended so I can no longer contribute on behalf of Yandex due to IPR issues (that's why the ipr check fails). That's unfortunate, but I will check with Yandex and if this state of things is final I will try to apply as an invited expert so hopefully I will be able to continue working on the spec. Not sure what to do with this PR though, I created it while Yandex was still a member.

@marcoscaceres

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

@dontcallmedom might have suggestions (Mozilla is also not a member of this WG, lols)

@dontcallmedom

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

I think it's fine to ignore the IPR checker here since indeed, the substantive part of the pull request was done while Yandex was signed up in the group.

@marcoscaceres

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

Marked as non substantive for IPR from ash-nazg.

@andrey-logvinov

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2018

@marcoscaceres ok to merge it? I think state machine is out of scope for this PR, it's better to create a separate issue.

@marcoscaceres

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

@marcoscaceres ok to merge it? I think state machine is out of scope for this PR, it's better to create a separate issue.

We can do it in two parts, sure.

@marcoscaceres

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

To be clear, I've not completely reviewed this, but would like the state machine and the appropriate errors in another PR before we merge this one.

@kenchris

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2019

This PR would need to be rewritten given the spec changes.

@kenchris

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2019

Can we close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.