(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Description • 9 years ago

Letting GeckoView control nsWindow creation enables us to associate the GeckoView with the new nsWindow, which is hard to do if we let the CLH control window creation.

	(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Comment 1 • 9 years ago

I'm running into test failures that only seem to happen on try, but I'm going to post my patches first.

	(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Comment 2 • 9 years ago

GeckoView.Window is a class that acts as the interface between
GeckoView in Java and nsWindow in C++. It will contain native methods
that GeckoView will use to interact with nsWindow.

On initialization, Window.open is called to create a nsWindow and
establish the JNI association between Window and the native nsWindow.
Then, whenever Window instance methods are called, the JNI stubs will
automatically call members of nsWindow.

	(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Comment 3 • 9 years ago

nsWindow will implement native methods of GeckoView.Window. This patch
implements the open method, which opens a new window in the same manner
as the CLH, and associates the new nsWindow with the GeckoView.Window
instance.

	(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Comment 4 • 9 years ago

Currently, BrowserCLH opens a single new window on startup. Now that
GeckoView is able to open windows through GeckoView.Window, we should
make GeckoView open its own window, which we can do earlier in startup,
and will make it possible to support multiple GeckoView's down the road.

	(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Comment 5 • 9 years ago

A C++ class that implments native JNI methods can choose to inherit
UsesGeckoThreadProxy. Once enabled, all native JNI calls on that class
will be automatically dispatched to the Gecko thread as a runnable event.

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 6 • 9 years ago

Comment on attachment 8654867 [details] [diff] [review]
Implement GeckoView.Window.open in nsWindow (v1)

Review of attachment 8654867 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/nsWindow.cpp
@@ +211,5 @@
> +            = do_CreateInstance(NS_SUPPORTS_PRINT32_CONTRACTID);
> +    nsCOMPtr<nsISupportsPRInt32> heightArg
> +            = do_CreateInstance(NS_SUPPORTS_PRINT32_CONTRACTID);
> +
> +    if (!args || !widthArg || !heightArg

I think I would like this better:

NS_FAILED(stuff) ||
NS_FAILED(stuff2) ||
...

@@ +215,5 @@
> +    if (!args || !widthArg || !heightArg
> +            || NS_FAILED(widthArg->SetData(width))
> +            || NS_FAILED(heightArg->SetData(height))
> +            || NS_FAILED(args->AppendElement(widthArg))
> +            || NS_FAILED(args->AppendElement(heightArg))) {

Brace on the next line for multi-line if

@@ +224,5 @@
> +
> +    nsCOMPtr<nsIDOMWindow> window;
> +    if (NS_FAILED(ww->OpenWindow(nullptr, url, "_blank", "chrome,dialog=no,all",
> +                                 args, getter_AddRefs(window)))
> +            || !window) {

Brace on new line again

@@ +238,5 @@
> +        return;
> +    }
> +
> +    auto& nativeWindow = *static_cast<nsWindow*>(widget.get());
> +    nativeWindow.mNatives = mozilla::MakeUnique<Natives>(nativeWindow);

Shouldn't you use 'this' in MakeUnique()? Otherwise, I don't understand the point or see how it works...

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 7 • 9 years ago

Comment on attachment 8654868 [details] [diff] [review]
Let GeckoView open the nsWindow instead of CLH (v1)

Review of attachment 8654868 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok as long as we don't regress startup perf (maybe a tiny hit would be ok)

::: mobile/android/base/GeckoView.java
@@ +204,5 @@
>          }
>      }
>  
> +    @Override
> +    public void onAttachedToWindow()

I highly suspect this will be a startup performance regression. Please run it through autophone so we can be figure that out.

@@ +211,5 @@
> +
> +        if (GeckoThread.isStateAtLeast(GeckoThread.State.PROFILE_READY)) {
> +            Window.open(window, metrics.widthPixels, metrics.heightPixels);
> +        } else {
> +            GeckoThread.queueNativeCallUntil(GeckoThread.State.PROFILE_READY, Window.class,

I wonder if you should just use this in every case, and GeckoThread would just dispatch immediately if we are already in that state. Having these blocks sprinkled everywhere will become pretty tiresome.

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 8 • 9 years ago

Comment on attachment 8654867 [details] [diff] [review]
Implement GeckoView.Window.open in nsWindow (v1)

Review of attachment 8654867 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with nits

::: widget/android/nsWindow.cpp
@@ +238,5 @@
> +        return;
> +    }
> +
> +    auto& nativeWindow = *static_cast<nsWindow*>(widget.get());
> +    nativeWindow.mNatives = mozilla::MakeUnique<Natives>(nativeWindow);

Nevermind, I get it now. MakeUnique confuses me.

	(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Comment 9 • 9 years ago

Originally, the GeckoThread PROFILE_READY state was chosen to correspond
to the profile-do-change event, to give priority to JNI code (e.g.
window creation) over other events that may be registered under
profile-after-change event. However, this leads to broken tests because
our testing infra expects things like window creation to happen during
profile-after-change at the earliest. This is because we have to wait
for addons like SpecialPowers to be loaded between profile-do-change and
profile-after-change.

This patch changes the PROFILE_READY state to correspond to the
profile-after-change event, so things are consistent again. it fixes all
the test failures I was seeing.

	Pulsebot
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/47d818382593
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0e93381822
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e9168fef5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/01e0f65e4d59
https://hg.mozilla.org/integration/mozilla-inbound/rev/c06ee52443a7

	Carsten Book [:Tomcat]
	Comment 11 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/47d818382593
https://hg.mozilla.org/mozilla-central/rev/bc0e93381822
https://hg.mozilla.org/mozilla-central/rev/d0e9168fef5d
https://hg.mozilla.org/mozilla-central/rev/01e0f65e4d59
https://hg.mozilla.org/mozilla-central/rev/c06ee52443a7