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

The issue of test case skip-waiting-installed.https.html #6548

Closed
xzhan96 opened this issue Jul 14, 2017 · 9 comments
Closed

The issue of test case skip-waiting-installed.https.html #6548

xzhan96 opened this issue Jul 14, 2017 · 9 comments

Comments

@xzhan96
Copy link

xzhan96 commented Jul 14, 2017

There is a discussion about the behavior of skipWaiting() in ServiceWorker issues 1015 , and the final decision should be in pull 1019:

promise resolves right away after setting the skip waiting flag

If I understand right, then there maybe a issue in the test skip-waiting-installed.https.html.

    var saw_message = new Promise(function(resolve) {
        onmessage = function(e) {
            resolve(e.data);
          };
        })
      .then(function(message) {
          assert_equals(
            message, 'PASS',
            'skipWaiting promise should be resolved with undefined');

          assert_equals(registration.active.scriptURL, normalizeURL(url2),
                        'skipWaiting should make worker become active');
        });

When the saw_message was called, registration.active.scriptURL should be still url1 instead of url2, because skipWaiting() will be executed just after new service worker is "installed" and before it become "activating"/"activated".

@xzhan96
Copy link
Author

xzhan96 commented Jul 14, 2017

@wanderview please check if it is an issue.

@wanderview
Copy link
Member

I'm on PTO this week. @asutherland, what do you think?

@xzhan96
Copy link
Author

xzhan96 commented Jul 18, 2017

@asutherland could you confirm if it's an issue?

@asutherland
Copy link
Contributor

asutherland commented Jul 19, 2017

Apologies for the delay; I've improved my email filters for mentions.

Yes, the test was wrong. Your fix on #6564 looks right to me and is consistent with the gecko-local patch I had on bug 1318142 which was the original source of the issue I raised.

@xzhan96
Copy link
Author

xzhan96 commented Jul 19, 2017

@asutherland Thanks for your explanation.
So I will drop my #6564 and wait your patch merged. Another question is that if you patch will sync to Chromium/third_party/WebKit/LayoutTests/external/wpt automatically ? or need to cherry-pick?

@makotoshimazu
Copy link
Contributor

@xzhan96
There is daily (weekly?) sync for layout tests in external/wpt. We don't need to cherry-pick manually.

@xzhan96
Copy link
Author

xzhan96 commented Jul 19, 2017

Ok, so let's wait for asutherland's patch. Can we know when it will be merged?

@asutherland
Copy link
Contributor

@xzhan96 I think it makes sense to keep and merge your #6564 fix now. My efforts on that bug have not yet landed in Gecko and it will be at least a week (possibly longer) before they do.

@xzhan96
Copy link
Author

xzhan96 commented Jul 19, 2017

Ok, thanks a lot. :-)

@xzhan96 xzhan96 closed this as completed Jul 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants