Fix devtools tooltip tests to support async XUL panel show/hide

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Developer Tools: Shared Components
P3
enhancement
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

Trunk
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

10 months ago
Devtools tooltip tests sometimes implicitly relied on the openPopup()/hidePopup() methods of the XUL panel being synchronous.

Bug 1206133 will make the open/hide methods asynchronous, so devtools tests need to be updated to wait for the proper events.

Try on top of current fx-team: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0f19e0c1744 

Try on top of the async patch from Bug 1206133 : https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7f7791db7e74638315450e82bfe5891958aadfa
(Assignee)

Updated

10 months ago
Assignee: nobody → jdescottes
Blocks: 1206133
Status: NEW → ASSIGNED
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

10 months ago
mozreview-review
Comment on attachment 8778880 [details]
Bug 1293211 - wait for xul wrapper popupshown event in HTMLTooltip;

https://reviewboard.mozilla.org/r/70016/#review67366
Attachment #8778880 - Flags: review?(bgrinstead) → review+

Comment 6

10 months ago
mozreview-review
Comment on attachment 8778881 [details]
Bug 1293211 - fire ready event in SwatchBasedEditorTooltips after widget initialization;

https://reviewboard.mozilla.org/r/70018/#review67370

::: devtools/client/shared/widgets/Tooltip.js:637
(Diff revision 1)
>          }
>        });
> +
> +      return onShown;
>      }
> +    return null;

Should this return Promise.resolve() instead of null?  I know this is being used in Tasks so they handle this fine with `yield` but returning a promise is what the comment above indicates should happen
Attachment #8778881 - Flags: review?(bgrinstead) → review+

Comment 7

10 months ago
mozreview-review
Comment on attachment 8778882 [details]
Bug 1293211 - wait for EditorTooltip ready event in tests;

https://reviewboard.mozilla.org/r/70020/#review67384
Attachment #8778882 - Flags: review?(bgrinstead) → review+

Comment 8

10 months ago
mozreview-review
Comment on attachment 8778883 [details]
Bug 1293211 - hide all tooltips before tests ending;

https://reviewboard.mozilla.org/r/70022/#review67386

::: devtools/client/inspector/rules/test/head.js:183
(Diff revision 1)
>   * Use this function to close the tooltip and make sure the test waits for the
>   * ruleview-changed event.
> - * @param {Tooltip} tooltip
> + * @param {SwatchColorPickerTooltip} colorpicker tooltip
>   * @param {CSSRuleView} view
>   */
> -function* hideTooltipAndWaitForRuleViewChanged(tooltip, view) {
> +function* hideTooltipAndWaitForRuleViewChanged(colorpicker, view) {

I see this used for more than just colorpickers: https://dxr.mozilla.org/mozilla-central/search?q=hideTooltipAndWaitForRuleViewChanged&redirect=true.  Why should this be specific to color pickers?
Attachment #8778883 - Flags: review?(bgrinstead)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

10 months ago
Thanks for the reviews! Updated the patches based on your comments and pushed to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bd0e7a7aa3b

Comment 14

10 months ago
mozreview-review
Comment on attachment 8778883 [details]
Bug 1293211 - hide all tooltips before tests ending;

https://reviewboard.mozilla.org/r/70022/#review67590
Attachment #8778883 - Flags: review?(bgrinstead) → review+

Comment 15

10 months ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/b850bd6aa5a3
part1: wait for xul wrapper popupshown event in HTMLTooltip;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/a98ce7344c9a
part2: fire ready event in SwatchBasedEditorTooltips after widget initialization;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/05ec8a857037
part3: wait for EditorTooltip ready event in tests;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/a3fb4eb11fcf
part4: ide all tooltips before tests ending;r=bgrins

Comment 16

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b850bd6aa5a3
https://hg.mozilla.org/mozilla-central/rev/a98ce7344c9a
https://hg.mozilla.org/mozilla-central/rev/05ec8a857037
https://hg.mozilla.org/mozilla-central/rev/a3fb4eb11fcf
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.