Closed Bug 1338396 Opened 3 years ago Closed 3 years ago

Permaorange in browser_context_menu_autocomplete_interaction.js | Autocomplete shouldn't appear - false == true when Gecko 53 merges to beta 2017-03-06

Categories

(Toolkit :: Password Manager, defect, P2)

53 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 + fixed
firefox54 --- fixed

People

(Reporter: philor, Assigned: MattN)

References

Details

Attachments

(1 file)

May or may not be related to the intermittent bug 1337772, I wouldn't expect that we'll know until someone figures out the cause of one or both.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99f7a3279685024128414d87c169fb51569732aa&group_state=expanded&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=76013352 is the current aurora as though it had been merged to beta, with permaorange in browser_context_menu_autocomplete_interaction.js

[Tracking Requested - why for this release]: permaorange, closed trees, sadness.
Flags: needinfo?(MattN+bmo)
Hmm… I was depending on some event.timeStamp recent changes and maybe they were behind a build flag… Yep, I see bug 1256562 uses a build flag:

https://hg.mozilla.org/mozilla-central/rev/d3343e6ab57e

This will be easy to fix if I switch to Date.now()
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
See Also: → 1256562
Feel free to autoland if it gets r+.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8836736 [details]
Bug 1338396 - LoginManagerContent: Use Date.now() instead of event.timeStamp until high-res timestamps are shipping.

https://reviewboard.mozilla.org/r/112082/#review113340
Attachment #8836736 - Flags: review?(jhofmann) → review+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e830c124ad16
LoginManagerContent: Use Date.now() instead of event.timeStamp until high-res timestamps are shipping. r=johannh
Comment on attachment 8836736 [details]
Bug 1338396 - LoginManagerContent: Use Date.now() instead of event.timeStamp until high-res timestamps are shipping.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1330111
[User impact if declined]: The interaction between the context menu and username autocomplete may be bad.
[Is this code covered by automated tests?]: Yes, it's what noticed the bug
[Has the fix been verified in Nightly?]: Not yet but this same fix was already used on beta.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No, trivial change
[Why is the change risky/not risky?]: Trivial change, only potential for a very slight performance impact of getting the current time with Date.now() but since it's only for contextmenu events on username fields it will be so rare to not be noticeable.
[String changes made/needed]: None
Attachment #8836736 - Flags: approval-mozilla-aurora?
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/e830c124ad16
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8836736 [details]
Bug 1338396 - LoginManagerContent: Use Date.now() instead of event.timeStamp until high-res timestamps are shipping.

Fix a permaorange issue related to context menu and autocomplete. Aurora53+.
Attachment #8836736 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Matt,
Just a drive-by. Bug 1026804 is about to turn on dom.event.highrestimestamp.enabled by default.
So I guess it's time to do
  `gLastContextMenuEventTimeStamp = event.timeStamp`[1] and
  `let timestamp = event.timeStamp`[2]

Should we open a follow-up bug to do that?
p.s If you have no other concern, I'd be happy to help on this.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#135
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#586
Flags: needinfo?(MattN+bmo)
We can but I don't think it's super important
Flags: needinfo?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.