Closed Bug 1333912 Opened 7 years ago Closed 7 years ago

Add onvisibilitychange attribute on Document

Categories

(Core :: DOM: Core & HTML, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: kevchan85, Mentored)

References

Details

(Keywords: dev-doc-complete, good-first-bug)

Attachments

(2 files, 3 obsolete files)

See https://w3c.github.io/page-visibility/#extensions-to-the-document-interface

You'll certainly need to modify testing/web-platform/meta/page-visibility/idlharness.html.ini if you implement this, but there might be other tests too.
Priority: -- → P3
Hello Boris, 

I have modified a couple tests for this site but this is the first time I am trying to implement something.
So Am I to create a new file somewhere to implement this? 

This is what I think the below code is doing. load the window and expect it to fail because it is hidden. Is this right? 

[idlharness.html]
  type: testharness
  [Document must be primary interface of window.document]
    expected: FAIL

  [Stringification of window.document]
    expected: FAIL
Flags: needinfo?(bzbarsky)
> So Am I to create a new file somewhere to implement this? 

No, you shouldn't need that.  Just add the attribute to the existing "partial interface Document" defined by visibility API in dom/webidl/Document.webidl and add the event to dom/events/EventNameList.h as a DOCUMENT_ONLY_EVENT.  You should be able to more or less copy how readystatechange works.

> This is what I think the below code is doing. load the window and expect it to fail because it is hidden.

No, it's just checking whether the property exists.  The relevant bits that you will likely need to remove are these:


  [Document interface: attribute onvisibilitychange]
    expected: FAIL

  [Document interface: window.document must inherit property "onvisibilitychange" with the proper type (2)]
    expected: FAIL
Flags: needinfo?(bzbarsky)
Attached patch added proposed suggestions. (obsolete) — Splinter Review
Here is a patch. I am not too sure if this is correct... or what you requested. I don't know how to test this yet...

Would I use mach test here too?
Forgot to add mentor
Flags: needinfo?(bzbarsky)
Comment on attachment 8835090 [details] [diff] [review]
added proposed suggestions.

This is generally the right idea.  But you need "visibilitychange", not "onvisibilitychange", for the first macro arg in EventNameList.h.  And you do need to update the spec links for the page visibility spec in Document.webidl.

As far as testing goes, have you already compiled with this patch?  I expect not, given the naming problem in EventNameList.h....

Once you have verified that the patch compiles, I can push it to try (see https://wiki.mozilla.org/ReleaseEngineering/TryServer for details) for you.  If you do want to test it locally, and have time when you can leave things running (e.g. while you sleep), you could run "mach wpt" -- that's the test suite that's most likely to have failures (or unexpected passes) due to this change.

Thank you for working on this!
Flags: needinfo?(bzbarsky)
Hello, I would like to try working on this bug too!
Please check with kevin to see whether he still is?
Flags: needinfo?(kevchan85)
Um dumb question.. Should I just email him or something?
That would probably work, yes.
Anyways I emailed him and while waiting on the response I'll try understanding what is needed to be done.
Hi, 

I am working on a patch. I had a problem with my patch file and reverting it in HG. I will have something up soon.
Flags: needinfo?(kevchan85)
Hello Boris. I guess kevin will do the patch since he started on it first. However, I still kind of want to understand how to do this as well. Do you mind explaining the parameters for the document-only event.. Do we have to create our own type of message of visibilitychange and etc?
So I made the suggested changes a while back. I did an hg pull, but it stalled. I tried using the Mozilla build VM to rewrite my patch, but it seems that in the harness that some one already wrote a couple of lines that I needed. When I did an export like hg export. > my-change.patch, it included a bunch of stuff that I didn't edit. 

I am trying to now do a fresh Ubuntu VM and am now in the process of downloading the code base. It has been killed a couple of times for some reason unknown to me, but I am trying to get it running again.
Flags: needinfo?(bzbarsky)
> Do you mind explaining the parameters for the document-only event

The first one is the event name.  The second one is the event message enum.  This is meant to identify particular events and should be unique per event type.  A new one does need to be added to widget/EventMessageList.h for this new event.

The third parameter is basically used for determining which elements should have an on* attribute that is considered an event handler attribute.  It's ignored for DOCUMENT_ONLY_EVENT entries, as far as I know.  Could just be left blank, probably.

The last parameter is what sort of internal event class should be used to represent this event.  eBasicEventClass means "Event" with no extra members and is the right thing here.
Flags: needinfo?(bzbarsky)
kevin, I'm not sure what info you're asking for...

Are you trying to get the code via hg clone of the entire repository, or starting with a bundle, then unbundling and pulling on top of it, or something else?
Hello Boris. Is there anyway to test if my change to the code actually added the attribute onvisibilitychange correctly? I've been trying to compile and test it, but problems have come up and that isn't finished yet. Is there another way that you suggest to test it? Thanks!
You really do have to get it compiled to test it.

Once you have it compiled, load http://w3c-test.org/page-visibility/idlharness.html and see whether the "Document interface: attribute onvisibilitychange" and "Document interface: window.document must inherit property "onvisibilitychange" with the proper type (2)" tests pass.
I'm not understanding this build error. DO you have any idea? onvisibilitychange is not a member of nsGkAtoms
Ok, I passed the test to check for onvisibilitychange attribute but getting the window.document to inherit the property of type 2 is giving me problems.. Any tips?
It's hard to say without seeing what your patch looks like.  Attach it, please?
Attached Boris!
Hi,
So I reinstalled everything and I used to use qnew to upload my patches, but that isn't working any more. What are the steps to upload patches via export? The documentation I read gives me a patch without my changes.
kevin, I'm not sure what your local setup looks like, but assuming you're using qnew/mq, it should look like this:

1)  hg qnew to create the new commit.
2)  Edit stuff.
3)  hg qref
4)  hg bzexport if you have that set up, or hg export and redirect to a file, then attach the file.

If you have run hg qnew multiple times, you presumably have multiple commits and hg export will only export the top one.  You can use "hg diff -r qparent" to get a rolled-up diff of all the commits, or you can hg qfold them together.  Again, hard to say remotely without knowing what your setup is.
Ummm Boris.. Any Tips?:(
I had to wait for it to compile to be able to say what's going on with the test, since you didn't actually tell me what the failure message was.  Anyway, the test is buggy.  I filed https://github.com/w3c/web-platform-tests/issues/5116 on that.
oh, so I did implement it correctly I suppose then..
Attached patch Updated patch (obsolete) — Splinter Review
Hi Boris, 
I tried testing on my side but it gets stalled on an audio test. Can we use the Try server instead?
Attachment #8835090 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
I pushed https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb4de45c628effa06035dd396035ec5363d2cc28 but without the unrelated-looking changes to CLOBBER and browser_bug1025195_switchToTabHavingURI_aOpenParams.js
Flags: needinfo?(bzbarsky)
OK, that try push shows the failure mentioned in comment 19 and comment 25.  Note that you can run that test on its own with "mach wpt page-visibility/idlharness.html" to check whether your annotations for it are correct.

The rest of the try run looks good.
Flags: needinfo?(kevchan85)
kevin, are you still working on this?
Hi Boris!
So what should I do next?
Flags: needinfo?(kevchan85)
Flags: needinfo?(bzbarsky)
Well, based on the test results is the patch ready for review from your point of view?  As in, are there any other changes you plan to make to it?

You may want to include https://github.com/w3c/web-platform-tests/pull/5204/files in it to make the problems described in comment 29 happy, or annotate those tests as failing, either way.
Flags: needinfo?(bzbarsky) → needinfo?(kevchan85)
Hi Boris, 

When you say include https://github.com/w3c/web-platform-tests/pull/5204/files how do I do that? Is this done in the testing/web-platform/meta/page-visibility/idlharness.html.ini? 

Sorry for the questions but I read this https://developer.mozilla.org/en-US/docs/Mozilla/QA/web-platform-tests and got more confused.
Flags: needinfo?(kevchan85) → needinfo?(bzbarsky)
Sorry for the lag; I was on vacation.

What I meant was to just take those changes and apply them to your tree, so they are included in your diff.  You can either use https://patch-diff.githubusercontent.com/raw/w3c/web-platform-tests/pull/5204.diff and apply the patch to your tree, or whatever other method you prefer.

At this point, it looks like that pull request was merged, so another option is to wait until we sync those changes into our tree, but that could be several weeks; I don't think there's a fixed schedule for the test syncs.  So probably better to just include the changes.

Again, the other opton is to annotate the relevant test as failing for now.  That would be done in testing/web-platform/meta/page-visibility/idlharness.html.ini file, yes.

Does that help?
Flags: needinfo?(bzbarsky)
Got it! I will give it a go!
Hi Boris. 

I am having an issue with running the web platform test idlharness. At first I thought it was because of my patch, but I reverted to the orginal head and I still got an error in the build. I even erased the clone and redownloaded it, but that didn't solve anything. Can you try to run the test on your side?

here is what I got from my console: 

 0:00.38 LOG: Thread-Log INFO STDOUT: DEBUG:manifest:Opening manifest at /Users/kevinchan/src/mozilla-central/testing/web-platform/meta/MANIFEST.json
 0:02.11 LOG: Thread-Log INFO STDOUT: DEBUG:manifest:Opening manifest at /Users/kevinchan/src/mozilla-central/testing/web-platform/mozilla/meta/MANIFEST.json
 0:02.52 LOG: MainThread INFO Using 1 client processes
 0:02.65 SUITE_START: MainThread 1
 0:02.65 LOG: MainThread INFO Running reftest tests
 0:02.68 LOG: MainThread INFO No reftest tests to run
 0:02.68 LOG: MainThread INFO Running wdspec tests
 0:02.68 LOG: MainThread INFO No wdspec tests to run
 0:02.68 LOG: MainThread INFO Running testharness tests
 0:02.73 LOG: Thread-TestrunnerManager-1 INFO Setting up ssl
 0:02.91 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (certutil) Full command: /Users/kevinchan/src/mozilla-central/obj-x86_64-apple-darwin16.6.0/dist/Nightly.app/Contents/MacOS/certutil -N -d /var/folders/_s/qrmkbnwn69j188wtm79n607m0000gn/T/tmptB7Lqz.mozrunner -f /var/folders/_s/qrmkbnwn69j188wtm79n607m0000gn/T/tmptB7Lqz.mozrunner/.crtdbpw
(certutil) ""
 0:03.04 LOG: MainThread INFO Starting http server on 127.0.0.1:8000
 0:03.04 LOG: MainThread INFO Starting http server on 127.0.0.1:8001
 0:03.04 LOG: MainThread INFO Starting http server on 127.0.0.1:8443
 0:03.08 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (certutil) ""
 0:03.25 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (certutil) "
Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

web-platform-tests                                           CT,, 
"
 0:03.25 LOG: Thread-TestrunnerManager-1 INFO Application command: /Users/kevinchan/src/mozilla-central/obj-x86_64-apple-darwin16.6.0/dist/Nightly.app/Contents/MacOS/firefox --marionette about:blank -foreground -profile /var/folders/_s/qrmkbnwn69j188wtm79n607m0000gn/T/tmptB7Lqz.mozrunner
 0:03.26 LOG: Thread-TestrunnerManager-1 INFO Starting runner
 0:04.77 PROCESS_OUTPUT: ProcessReader (pid:89004) Full command: /Users/kevinchan/src/mozilla-central/obj-x86_64-apple-darwin16.6.0/dist/Nightly.app/Contents/MacOS/firefox --marionette about:blank -foreground -profile /var/folders/_s/qrmkbnwn69j188wtm79n607m0000gn/T/tmptB7Lqz.mozrunner
(pid:89004) "1500408722902	Marionette	INFO	Enabled via --marionette"
 0:07.38 PROCESS_OUTPUT: ProcessReader (pid:89004) "[Child 89006] ###!!! ABORT: LoadSheetSync failed with error 80520015 loading built-in stylesheet 'resource://gre-resources/counterstyles.css': file /Users/kevinchan/src/mozilla-central/layout/style/nsLayoutStylesheetCache.cpp, line 776"
 0:07.38 PROCESS_OUTPUT: ProcessReader (pid:89004) "[Child 89006] ###!!! ABORT: LoadSheetSync failed with error 80520015 loading built-in stylesheet 'resource://gre-resources/counterstyles.css': file /Users/kevinchan/src/mozilla-central/layout/style/nsLayoutStylesheetCache.cpp, line 776"
 0:07.57 PROCESS_OUTPUT: ProcessReader (pid:89004) "1500408725697	Marionette	INFO	Listening on port 2828"
 0:07.83 PROCESS_OUTPUT: ProcessReader (pid:89004) ""
 0:07.83 PROCESS_OUTPUT: ProcessReader (pid:89004) "###!!! [Parent][MessageChannel] Error: (msgtype=0x28007E,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv"
 0:07.83 PROCESS_OUTPUT: ProcessReader (pid:89004) ""
 0:09.51 PROCESS_OUTPUT: ProcessReader (pid:89004) "[Child 89008] ###!!! ABORT: LoadSheetSync failed with error 80520015 loading built-in stylesheet 'resource://gre-resources/counterstyles.css': file /Users/kevinchan/src/mozilla-central/layout/style/nsLayoutStylesheetCache.cpp, line 776"
 0:09.51 PROCESS_OUTPUT: ProcessReader (pid:89004) "[Child 89008] ###!!! ABORT: LoadSheetSync failed with error 80520015 loading built-in stylesheet 'resource://gre-resources/counterstyles.css': file /Users/kevinchan/src/mozilla-central/layout/style/nsLayoutStylesheetCache.cpp, line 776"
 0:09.69 PROCESS_OUTPUT: ProcessReader (pid:89004) ""
 0:09.69 PROCESS_OUTPUT: ProcessReader (pid:89004) "###!!! [Parent][MessageChannel] Error: (msgtype=0x28007E,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv"
 0:09.69 PROCESS_OUTPUT: ProcessReader (pid:89004) ""
 0:10.50 PROCESS_OUTPUT: ProcessReader (pid:89004) "2017-07-18 13:12:08.636 plugin-container[89010:10562166] *** CFMessagePort: bootstrap_register(): failed 1100 (0x44c) 'Permission denied', port = 0xa23b, name = 'com.apple.tsm.portname'"
 0:10.50 PROCESS_OUTPUT: ProcessReader (pid:89004) "See /usr/include/servers/bootstrap_defs.h for the error codes."
 0:10.51 PROCESS_OUTPUT: ProcessReader (pid:89004) "2017-07-18 13:12:08.638 plugin-container[89010:10562166] *** CFMessagePort: bootstrap_register(): failed 1100 (0x44c) 'Permission denied', port = 0xa503, name = 'com.apple.CFPasteboardClient'"
 0:10.51 PROCESS_OUTPUT: ProcessReader (pid:89004) "See /usr/include/servers/bootstrap_defs.h for the error codes."
 0:25.26 TEST_START: Thread-TestrunnerManager-1 /page-visibility/idlharness.html
 0:25.33 LOG: Thread-Log INFO STDERR: Exception in thread Thread-2:
Traceback (most recent call last):
  File "/usr/local/Cellar/python/2.7.13_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/threading.py", line 801, in __bootstrap_inner
    self.run()
  File "/usr/local/Cellar/python/2.7.13_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/threading.py", line 754, in run
    self.__target(*self.__args, **self.__kwargs)
  File "/Users/kevinchan/src/mozilla-central/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py", line 397, in _run
    message += "\n"
TypeError: unsupported operand type(s) for +=: 'MarionetteProtocol' and 'str'
 0:25.34 TEST_END: Thread-TestrunnerManager-1 TIMEOUT, expected OK

 0:25.35 LOG: Thread-TestrunnerManager-1 INFO Pausing until the browser exits
 0:29.24 LOG: Thread-TestrunnerManager-1 ERROR Traceback (most recent call last):
  File "/Users/kevinchan/src/mozilla-central/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py", line 181, in wait
    self.marionette.execute_async_script("")
  File "/Users/kevinchan/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1798, in execute_async_script
    rv = self._send_message("executeAsyncScript", body, key="value")
  File "/Users/kevinchan/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 23, in _
    return func(*args, **kwargs)
  File "/Users/kevinchan/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 739, in _send_message
    self._handle_error(err)
  File "/Users/kevinchan/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 772, in _handle_error
    raise errors.lookup(error)(message, stacktrace=stacktrace)
NoSuchWindowException: Unable to locate window
stacktrace:
	WebDriverError@chrome://marionette/content/error.js:226:5
	NoSuchWindowError@chrome://marionette/content/error.js:444:5
	assert.that/<@chrome://marionette/content/assert.js:379:13
	assert.window@chrome://marionette/content/assert.js:113:10
	GeckoDriver.prototype.executeAsyncScript@chrome://marionette/content/driver.js:899:3
	TaskImpl_run@resource://gre/modules/Task.jsm:331:42
	TaskImpl@resource://gre/modules/Task.jsm:280:3
	asyncFunction@resource://gre/modules/Task.jsm:252:14
	Task_spawn@resource://gre/modules/Task.jsm:166:12
	TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:401:16
	TaskImpl_run@resource://gre/modules/Task.jsm:339:15
	TaskImpl@resource://gre/modules/Task.jsm:280:3
	asyncFunction@resource://gre/modules/Task.jsm:252:14
	Task_spawn@resource://gre/modules/Task.jsm:166:12
	execute@chrome://marionette/content/server.js:524:15
	onPacket@chrome://marionette/content/server.js:495:7
	_onJSONObjectReady/<@chrome://marionette/content/transport.js:499:9
Forgot to add mentor
Flags: needinfo?(bzbarsky)
That sounds like bug 1379784, but you're running via "mach", so I'm not sure what's going on there...
Flags: needinfo?(haftandilian)
Flags: needinfo?(agaynor)
Flags: needinfo?(bzbarsky)
Looks like running web platform tests is just broken on Mac, even via "mach".  The patches in bug 1380690 will help, but in the meantime you should be able to do:

  env MOZ_DISABLE_CONTENT_SANDBOX=t mach wpt

instead of just "mach wpt" to get things to work.
(In reply to Boris Zbarsky [:bz] from comment #40)
> Looks like running web platform tests is just broken on Mac, even via
> "mach".  The patches in bug 1380690 will help, but in the meantime you
> should be able to do:
> 
>   env MOZ_DISABLE_CONTENT_SANDBOX=t mach wpt
> 
> instead of just "mach wpt" to get things to work.

That's right. Another workaround that doesn't disable the sandbox is to set MOZ_DEVELOPER_REPO_DIR and MOZ_DEVELOPER_OBJ_DIR:

  $ MOZ_DEVELOPER_REPO_DIR=/path/to/repo MOZ_DEVELOPER_OBJ_DIR=/path/to/repo/object/dir ./mach wpt
Flags: needinfo?(haftandilian)
So by env MOZ_DISABLE_CONTENT_SANDBOX=t mach wpt did you mean add that to the mozconfig file? I did that... and the tests now run... I have to recompile with the changes I made later.
Flags: needinfo?(bzbarsky)
I actually meant just doing that on your command line when running the tests.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(agaynor)
Hi Boris,

I ran the patch with the new updates from 5204.diff and it was green. 

If there is anything you need please tell me. 

kevin
Attachment #8890126 - Flags: review?(bzbarsky)
Comment on attachment 8890126 [details] [diff] [review]
updated patch with idltestharness included

This only has the idlharness changes, not the actual fix for this bug, right?
Flags: needinfo?(kevchan85)
Attachment #8890126 - Flags: review?(bzbarsky)
Attached patch Correct patchSplinter Review
Uploaded correct patch.
Attachment #8848896 - Attachment is obsolete: true
Attachment #8890126 - Attachment is obsolete: true
Flags: needinfo?(kevchan85)
Attachment #8890416 - Flags: review?(bzbarsky)
And that one _doesn't_ have the idlharness changes...  I'll fold those last two patches together.
Thanks Boris, I wasn't sure about which to upload. I basically worked with the two paches via pushes and ran the tests. I wasn't sure if me adding those changes would affect the github patch 5204.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/898cac60f7c3
Implement onvisibilitychange attribute on Document.  r=bzbarsky
kevin, thank you for sticking with this!
Sorry, it took so long. I must admit that this was difficult for me. Is there anything else I can work on?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #51)
> kevin, thank you for sticking with this!

And sorry you had to deal with those test crashes. That's not the norm.
https://www.joshmatthews.net/bugsahoy/ is a good resource for finding bugs.

For bugs I'm mentor on, bug 531327 might be not too hard.  Bug 925694 might also be reasonable.  And maybe bug 1321738.
Flags: needinfo?(bzbarsky)
Assignee: nobody → kevchan85
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/898cac60f7c3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Attachment #8890416 - Flags: review?(bzbarsky) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.