Closed Bug 1409580 Opened 7 years ago Closed 7 years ago

Run plain mochitests in Headless Mode on MacOS

Categories

(Firefox :: Headless, enhancement, P1)

Product:
Firefox
Component:
Headless
Platform:
Unspecified
macOS
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: bdahl, Assigned: bdahl)

References

Details

Attachments

(6 files)

Bug 1409580 - Support edit command mapping in headless MacOS.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1409580 - Use top level widget for headless screen offset.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1409580 - Use different scroll delta modes on different platforms in headless.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1409580 - Raise headless sheet widgets when shown.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1409580 - Skip test_bug511449.html in headless mode.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1409580 - Skip test_transformed_scrolling_repaints_2.html in headless mode on MacOS.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details 
      No description provided.
No longer depends on: 1408220
Depends on: 1409582
Try run for the above request https://treeherder.mozilla.org/#/jobs?repo=try&revision=77904697c3df0f00fcbf6a3cebca52bf5328dc68.

Please note, that in that try run there are two patches I won't be pushing. [1] has a temp fix to avoid an assertion described in bug 1409582. [2] enables headless MacOS tests on automation, I was asked not to enable this for now since MacOS resources are limited.

[1] https://hg.mozilla.org/try/rev/859c51a39f8f9441cb0c62d3ee5fd83375bcdcd6
[2] https://hg.mozilla.org/try/rev/24cf930cafa7dce60084087028142ce76fdadacf
Attachment #8925069 - Flags: review?(mstange) → review?(masayuki)
Comment on attachment 8925069 [details]
Bug 1409580 - Support edit command mapping in headless MacOS.

https://reviewboard.mozilla.org/r/196294/#review202734

::: widget/TextEvents.h:25
(Diff revision 1)
>  #include "nsISelectionListener.h"
>  #include "nsITransferable.h"
>  #include "nsRect.h"
>  #include "nsStringGlue.h"
>  #include "nsTArray.h"
> -#include "WritingModes.h"
> +#include "mozilla/WritingModes.h"

Please move this line next to "mozilla/TextRange.h".

(Although, "mozilla/FontRange.h" is in the odd place, if you don't mind, make them sorted from A-Z order.)

::: widget/cocoa/TextInputHandler.mm:4436
(Diff revision 1)
> -  }
> -
>    aKeyEvent.mNativeKeyEvent =
> -    [NSEvent     keyEventWithType:eventType
> -                         location:NSMakePoint(0,0)
> -                    modifierFlags:modifierFlags
> +    nsCocoaUtils::MakeNewCococaEventFromWidgetEvent(aKeyEvent,
> +                                                    windowNumber,
> +                                                    [NSGraphicsContext currentContext]);

nit: over 80 characters. How about to store [NSGraphicsContext currentContext] to a variable?

::: widget/cocoa/nsCocoaUtils.h:333
(Diff revision 1)
> +  static NSEvent* MakeNewCococaEventFromWidgetEvent(const mozilla::WidgetKeyboardEvent& aKeyEvent,
> +                                                    NSInteger aWindowNumber,
> +                                                    NSGraphicsContext* aContext);

nit: too long lines. Perhaps, arguments start under "k" of "Make". I.e.,

>  static NSEvent* MakeNewCococaEventFromWidgetEvent(
>                    const mozilla::WidgetKeyboardEvent& aKeyEvent,
>                    NSInteger aWindowNumber,
>                    NSGraphicsContext* aContext);

::: widget/cocoa/nsCocoaUtils.mm:602
(Diff revision 1)
> +nsCocoaUtils::MakeNewCococaEventFromWidgetEvent(const WidgetKeyboardEvent& aKeyEvent,
> +                                                NSInteger aWindowNumber,
> +                                                NSGraphicsContext* aContext)

The first line is too long. Perhaps,

> nsCocoaUtils::MakeNewCococaEventFromWidgetEvent(
>                 const WidgetKeyboardEvent& aKeyEvent,
>                 NSInteger aWindowNumber,
>                 NSGraphicsContext* aContext)

::: widget/headless/HeadlessKeyBindings.h:10
(Diff revision 1)
> +
> +#ifndef mozilla_widget_HeadlessKeyBindings_h
> +#define mozilla_widget_HeadlessKeyBindings_h
> +
> +#include "nsTArray.h"
> +#include "TextEvents.h"

Please use "mozilla/TextEvents.h" and sort the includes as a-z order.

::: widget/headless/HeadlessKeyBindings.h:20
(Diff revision 1)
> +
> +/**
> + * Helper to emulate native key bindings. Currently only MacOS is supported.
> + */
> +
> +class HeadlessKeyBindings final {

nit: put |{| to the next line.

::: widget/headless/HeadlessKeyBindingsCocoa.mm:40
(Diff revision 1)
> +HeadlessKeyBindings::GetEditCommands(nsIWidget::NativeKeyBindingsType aType,
> +                                           const WidgetKeyboardEvent& aEvent,
> +                                           nsTArray<CommandInt>& aCommands)

2nd and 3rd argument's indent are odd.

::: widget/headless/HeadlessWidget.h:121
(Diff revision 1)
>                    LayersBackend aBackendHint = mozilla::layers::LayersBackend::LAYERS_NONE,
>                    LayerManagerPersistence aPersistence = LAYER_MANAGER_CURRENT) override;
>  
>    void SetCompositorWidgetDelegate(CompositorWidgetDelegate* delegate) override;
>  
> +  virtual MOZ_MUST_USE nsresult AttachNativeKeyEvent(WidgetKeyboardEvent& aEvent) override;

Too long line, but looks like the above method declarations too and I don't familiar with local coding style in widget/headless. So, up to you.

::: widget/headless/HeadlessWidget.h:124
(Diff revision 1)
> +                 const mozilla::WidgetKeyboardEvent& aEvent,
> +                 nsTArray<mozilla::CommandInt>& aCommands) override;

you shouldn't need mozilla namespaces because there is in mozilla::widget.

::: widget/headless/HeadlessWidget.cpp:430
(Diff revision 1)
>  
>    return NS_OK;
>  }
>  
>  nsresult
> +HeadlessWidget::AttachNativeKeyEvent(mozilla::WidgetKeyboardEvent& aEvent)

I think that you don't need to use "mozilla::" here.

::: widget/headless/HeadlessWidget.cpp:438
(Diff revision 1)
> +                          const WidgetKeyboardEvent& aEvent,
> +                          nsTArray<CommandInt>& aCommands)

odd indent.
Attachment #8925069 - Flags: review?(masayuki) → review+
I'll try to get to these tomorrow. Sorry about the review delay.
Comment on attachment 8925070 [details]
Bug 1409580 - Use top level widget for headless screen offset.

https://reviewboard.mozilla.org/r/196296/#review205598

::: widget/headless/HeadlessWidget.cpp:290
(Diff revision 1)
>  }
>  
>  LayoutDeviceIntPoint
>  HeadlessWidget::WidgetToScreenOffset()
>  {
> -  return LayoutDeviceIntPoint(mBounds.x, mBounds.y);
> +  auto bounds = mTopLevel->GetBounds();

return mTopLevel->GetBounds().TopLeft()
Attachment #8925070 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8925071 [details]
Bug 1409580 - Use different scroll delta modes on different platforms in headless.

https://reviewboard.mozilla.org/r/196298/#review205602
Attachment #8925071 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8925072 [details]
Bug 1409580 - Raise headless sheet widgets when shown.

https://reviewboard.mozilla.org/r/196300/#review205604
Attachment #8925072 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8925073 [details]
Bug 1409580 - Skip test_bug511449.html in headless mode.

https://reviewboard.mozilla.org/r/196302/#review205606
Attachment #8925073 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8925074 [details]
Bug 1409580 - Skip test_transformed_scrolling_repaints_2.html in headless mode on MacOS.

https://reviewboard.mozilla.org/r/196304/#review205608
Attachment #8925074 - Flags: review?(jmuizelaar) → review+
Sorry about the delay.
Assignee: nobody → bdahl
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/264ccb66a86c
Support edit command mapping in headless MacOS. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/2bd51020fa81
Use top level widget for headless screen offset. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/914b07533a33
Use different scroll delta modes on different platforms in headless. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/d3d90cd557e6
Raise headless sheet widgets when shown. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/1f6c7c67f561
Skip test_bug511449.html in headless mode. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/f7df4e73fadf
Skip test_transformed_scrolling_repaints_2.html in headless mode on MacOS. r=jrmuizel
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: