Closed Bug 1293211 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: Shared Components, enhancement, P3)

enhancement

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(4 files)

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: nobody → jdescottes
Blocks: 1206133
Status: NEW → ASSIGNED
Priority: -- → P3
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 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 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 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)
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 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+
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.