Closed Bug 1442800 Opened 6 years ago Closed 6 years ago

XULMAP.h functions should take Element* instead nsIContent*

Categories

(Toolkit :: XUL Widgets, task)

Product:
Toolkit
Component:
XUL 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: 6 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: 6 years ago → 6 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.