Closed Bug 1442800 Opened 7 years ago Closed 7 years ago

XULMAP.h functions should take Element* instead nsIContent*

Categories

(Toolkit :: UI Widgets, task)

Product:
Toolkit
Component:
UI Widgets
Type:
task
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(1 file)

Bug 1442800 - Lambda in XULMarkupMapInfo should take Element instead of nsIContent
59 bytes, text/x-review-board-request
surkov
: review+
Details
Blocks: war-on-xbl
No longer depends on: war-on-xbl
Comment on attachment 8956620 [details] Bug 1442800 - Lambda in XULMarkupMapInfo should take Element instead of nsIContent https://reviewboard.mozilla.org/r/225566/#review231438 ::: commit-message-c7543:3 (Diff revision 1) > +Bug 1442800 - Don't check nsIContent->IsElement() in XULMAP.h functions r=surkov > + > +The check turned out to be redundant because there is already a I did not do what's originally planned because `XUL*Accessible()` classes still take `nsIContent` as first argument. Only passes `Element` to lambda will require changes to everything.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #2) > Comment on attachment 8956620 [details] > Bug 1442800 - Don't check nsIContent->IsElement() in XULMAP.h functions > > https://reviewboard.mozilla.org/r/225566/#review231438 > > ::: commit-message-c7543:3 > (Diff revision 1) > > +Bug 1442800 - Don't check nsIContent->IsElement() in XULMAP.h functions r=surkov > > + > > +The check turned out to be redundant because there is already a > > I did not do what's originally planned because `XUL*Accessible()` classes > still take `nsIContent` as first argument. Only passes `Element` to lambda > will require changes to everything. nsIContent is inherited from Element, so you can use Element whenever you are required to use nsIContent
Comment on attachment 8956620 [details] Bug 1442800 - Lambda in XULMarkupMapInfo should take Element instead of nsIContent https://reviewboard.mozilla.org/r/225566/#review231612 r=me, I think it is safe assumption, but I would recommend to switch to Element* in lambdas
Attachment #8956620 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8956620 [details] Bug 1442800 - Lambda in XULMarkupMapInfo should take Element instead of nsIContent You are right, patch updated.
Attachment #8956620 - Flags: review+ → review?(surkov.alexander)
Comment on attachment 8956620 [details] Bug 1442800 - Lambda in XULMarkupMapInfo should take Element instead of nsIContent https://reviewboard.mozilla.org/r/225566/#review231748 thanks!
Attachment #8956620 - Flags: review?(surkov.alexander) → review+
Pushed by timdream@gmail.com: https://hg.mozilla.org/integration/autoland/rev/349222d58373 Let New_Accessible take Element instead of nsIContent r=surkov
Backout by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a07c7148dac6 Backed out changeset 349222d58373 for frequent wpt failures on a CLOSED TREE
I cannot produce the issue locally :'( Try push on a rebased central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c74668cb4956769159cddd41cd2c81643877df3
Flags: needinfo?(timdream)
(In reply to Cosmin Sabou [:cosmin_s] from comment #11) > Backed out for frequent wpt failures: > > Push with failures: > https://treeherder.mozilla.org/#/ > jobs?repo=autoland&revision=349222d58373876cf54a9eda3c4d30a5f134b5c1&filter- > resultStatus=testfailed&filter-resultStatus=busted&filter- > resultStatus=exception&selectedJob=166568779&filter-searchStr=wpt > > Failure log: > https://treeherder.mozilla.org/logviewer. > html#?job_id=166568779&repo=autoland&lineNumber=28470 > > Backout link: > https://hg.mozilla.org/integration/autoland/rev/ > a07c7148dac649770a3444bc4dd15322ada62976 the failure doens't seem related, the patch is pure a11y change that should have zero impact on no a11y test case
Pushed by timdream@gmail.com: https://hg.mozilla.org/integration/autoland/rev/564cb9725e12 Let New_Accessible take Element instead of nsIContent r=surkov
These are failing with every retrigger as far as we have checked. Push : https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=0e28b41ea4c825e8f1a17011f12d3e2d945189f8&selectedJob=166800793 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=166800793&repo=mozilla-central&lineNumber=2146 [task 2018-03-08T18:35:24.720Z] 18:35:24 INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/events/browser_test_docload.js | Unexpected event order: [xpconnect wrapped (nsISupports, nsIAccessibleEvent, nsIAccessibleStateChangeEvent)] - Got 2, expected 0 [task 2018-03-08T18:35:24.720Z] 18:35:24 INFO - Stack trace: [task 2018-03-08T18:35:24.720Z] 18:35:24 INFO - chrome://mochikit/content/browser-test.js:test_is:1271 [task 2018-03-08T18:35:24.721Z] 18:35:24 INFO - chrome://mochitests/content/browser/accessible/tests/browser/events.js:waitForEvents/</<:172 [task 2018-03-08T18:35:24.722Z] 18:35:24 INFO - Not taking screenshot here: see the one that was previously logged [task 2018-03-08T18:35:24.723Z] 18:35:24 INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/events/browser_test_docload.js | Unexpected event order: [xpconnect wrapped nsIAccessibleEvent] - Got 0, expected 1 [task 2018-03-08T18:35:24.723Z] 18:35:24 INFO - Stack trace: [task 2018-03-08T18:35:24.724Z] 18:35:24 INFO - chrome://mochikit/content/browser-test.js:test_is:1271 [task 2018-03-08T18:35:24.724Z] 18:35:24 INFO - chrome://mochitests/content/browser/accessible/tests/browser/events.js:waitForEvents/</<:172 [task 2018-03-08T18:35:24.725Z] 18:35:24 INFO - GECKO(1205) | [1205, Main Thread] WARNING: g_type_add_interface_static: assertion 'g_type_parent (interface_type) == G_TYPE_INTERFACE' failed: 'glib warning', file /builds/worker/workspace/build/src/toolkit/xre/nsSigHandlers.cpp, line 141 [task 2018-03-08T18:35:24.725Z] 18:35:24 INFO - GECKO(1205) | (firefox:1205): GLib-GObject-CRITICAL **: g_type_add_interface_static: assertion 'g_type_parent (interface_type) == G_TYPE_INTERFACE' failed [task 2018-03-08T18:35:24.726Z] 18:35:24 INFO - Not taking screenshot here: see the one that was previously logged [task 2018-03-08T18:35:24.727Z] 18:35:24 INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/events/browser_test_docload.js | Unexpected event order: [xpconnect wrapped nsIAccessibleEvent] - Got 1, expected 2 :timdream can you take a look please?
Flags: needinfo?(timdream)
it's hard to say without debugging why those test fail, another way to investigate the issue is to split the patch into two: HTML part and XUL part and land them separately. I suspect HTML part a culprit.
Please back it out again and I will investigate.
Flags: needinfo?(timdream)
Backed out changeset 564cb9725e12 (bug 1442800) for failing on browser_test_docload.js on a CLOSED TREE Backout link: https://hg.mozilla.org/integration/autoland/rev/ac61f51908f89d7c4fe347cc88f6c5748078dd90 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=564cb9725e123d59f6f12bd8af3625dcee32a292&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=166820525&repo=mozilla-central&lineNumber=2280 [task 2018-03-08T19:31:31.745Z] 19:31:31 INFO - TEST-START | accessible/tests/browser/events/browser_test_docload.js [task 2018-03-08T19:31:32.069Z] 19:31:32 INFO - GECKO(1203) | [1203, Main Thread] WARNING: g_type_add_interface_static: assertion 'g_type_parent (interface_type) == G_TYPE_INTERFACE' failed: 'glib warning', file /builds/worker/workspace/build/src/toolkit/xre/nsSigHandlers.cpp, line 141 [task 2018-03-08T19:31:32.071Z] 19:31:32 INFO - GECKO(1203) | (firefox:1203): GLib-GObject-CRITICAL **: g_type_add_interface_static: assertion 'g_type_parent (interface_type) == G_TYPE_INTERFACE' failed [task 2018-03-08T19:31:32.072Z] 19:31:32 INFO - TEST-INFO | started process screentopng [task 2018-03-08T19:31:32.358Z] 19:31:32 INFO - TEST-INFO | screentopng: exit 0 [task 2018-03-08T19:31:32.363Z] 19:31:32 INFO - Buffered messages logged at 19:31:31 [task 2018-03-08T19:31:32.363Z] 19:31:32 INFO - Entering test bound [task 2018-03-08T19:31:32.364Z] 19:31:32 INFO - Buffered messages finished [task 2018-03-08T19:31:32.365Z] 19:31:32 INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/events/browser_test_docload.js | Unexpected event order: [xpconnect wrapped (nsISupports, nsIAccessibleEvent, nsIAccessibleStateChangeEvent)] - Got 2, expected 0 [task 2018-03-08T19:31:32.365Z] 19:31:32 INFO - Stack trace: [task 2018-03-08T19:31:32.366Z] 19:31:32 INFO - chrome://mochikit/content/browser-test.js:test_is:1271 [task 2018-03-08T19:31:32.366Z] 19:31:32 INFO - chrome://mochitests/content/browser/accessible/tests/browser/events.js:waitForEvents/</<:172 [task 2018-03-08T19:31:32.366Z] 19:31:32 INFO - GECKO(1203) | [1203, Main Thread] WARNING: g_type_add_interface_static: assertion 'g_type_parent (interface_type) == G_TYPE_INTERFACE' failed: 'glib warning', file /builds/worker/workspace/build/src/toolkit/xre/nsSigHandlers.cpp, line 141 [task 2018-03-08T19:31:32.367Z] 19:31:32 INFO - GECKO(1203) | (firefox:1203): GLib-GObject-CRITICAL **: g_type_add_interface_static: assertion 'g_type_parent (interface_type) == G_TYPE_INTERFACE' failed [task 2018-03-08T19:31:32.367Z] 19:31:32 INFO - Not taking screenshot here: see the one that was previously logged
Flags: needinfo?(timdream)
https://hg.mozilla.org/mozilla-central/rev/564cb9725e12
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Backout merged: https://hg.mozilla.org/mozilla-central/rev/ac61f51908f8
Status: RESOLVED → REOPENED
status-firefox60: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---
Looks like the backout from comment 20 brought in some unexpected perf improvements: == Change summary for alert #12011 (as of Thu, 08 Mar 2018 21:20:17 GMT) == Improvements: 5% build times windows2012-64 opt static-analysis taskcluster-c4.4xlarge 2,557.90 -> 2,418.74 5% build times windows2012-32 opt static-analysis taskcluster-c4.4xlarge 2,393.47 -> 2,285.07 2% build times windows2012-64 pgo taskcluster-c4.4xlarge 4,727.67 -> 4,651.99 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12011
(In reply to alexander :surkov from comment #16) > it's hard to say without debugging why those test fail, another way to > investigate the issue is to split the patch into two: HTML part and XUL part > and land them separately. I suspect HTML part a culprit. The new patch contains only the XUL part. Will deal with HTML in another bug.
Flags: needinfo?(timdream)
So it still happens on with only the XUL part... https://treeherder.mozilla.org/#/jobs?repo=try&revision=51e3160530158d023ee26ebda61a56fef5636606 Linux x64 JSDCov opt is a tier-2 platform though. Regardless I should investigate if I could produce this locally.
After a quick rebase I think we can confirm the errors was unrelated https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcd310b5da8e52b6d6f3d415db093a5a44d44bea If this Try goes well I will land this tomorrow.
Pushed by timdream@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/46caf0a4faa8 Let New_Accessible take Element instead of nsIContent r=surkov
https://hg.mozilla.org/mozilla-central/rev/46caf0a4faa8
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: