777451 - Allow better communication between pages and java in robocop tests

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Two things I've wanted for robocop:

1.) The ability to load pages with chrome privileges. Those pages can then register for Java notifications and respond to them.
2.) The ability for java to get back responses from the pages.

We're already shipping a roboextender extension. We can provide some easy hooks to push chrome pages with it. Those pages can then register as observers for messages from java. Java will have to also register for messages from pages, but we can pass messages with those so that Java can get data back from pages.

Sound good? Patch coming.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

This copies files from mobile/android/base/tests/roboextender to the roboextender base directory during build, and also includes a chrome.manifest file so that the chrome://roboextender/content directory is recognized.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This adds a waitForReturn method to the eventExpecter which can be used to get any data sent from js.

Comment 3

[Geoff Brown [:gbrown]]

Comment on attachment 645843 [details] [diff] [review]
Patch 2/2

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

Good idea. Do you have another patch that makes use of it?

::: build/mobile/robocop/FennecNativeActions.java.in
@@ +112,5 @@
>              return null;
>          }
>      }
>  
> +    public class GeckoEventExpecter implements EventExpecter {

why does this need to change to public?

@@ +117,4 @@
>          private final String mGeckoEvent;
>          private final Object[] mRegistrationParams;
>          private boolean mEventReceived;
> +        private String mEventName;

I don't see mEventName accessed/read anywhere. Will it be used?

@@ +137,5 @@
>              FennecNativeDriver.log(FennecNativeDriver.LogLevel.DEBUG,
>                  "unblocked on expecter for " + mGeckoEvent);
>          }
>  
> +        public synchronized String waitForReturn() {

nit - I would prefer a name that parallels the existing method names: blockForEventAndGetData / blockForEventData / blockForEventReturn?

Comment 4

[Geoff Brown [:gbrown]]

Comment on attachment 645841 [details] [diff] [review]
Patch

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

::: testing/mochitest/roboextender/Makefile.in
@@ +25,5 @@
>  libs:: $(_TEST_FILES)
>  	$(MKDIR) -p $(TEST_EXTENSIONS_DIR)/[e-mail]
>  	$(INSTALL) $(foreach f,$^,"$f") $(TEST_EXTENSIONS_DIR)/[e-mail]/
> +	$(MKDIR) -p $(TEST_EXTENSIONS_DIR)/[e-mail]/base
> +	cp $(TESTPATH)/* $(TEST_EXTENSIONS_DIR)/[e-mail]/base

Neither patch creates or populates TESTPATH. Do you have another patch for that? Without it, make fails:

mkdir -p ../../../_tests/testing/mochitest/extensions/[e-mail]/base
cp /home/mozdev/src/mobile/android/base/tests/roboextender/* ../../../_tests/testing/mochitest/extensions/[e-mail]/base
cp: cannot stat `/home/mozdev/src/mobile/android/base/tests/roboextender/*': No such file or directory

Comment 5

[Wesley Johnston (:wesj)]

Whoops. sorry about that. I have some partially working tests that use this for prompts that I was hoping to put in bug 757481. The tests in there right now are not up to date using this. The tests that do use it are racy and fail sometimes when calling clickOnButton("OK"). So I shelved that project for a bit.

I'll update this so it doesn't fail if the test path doesn't exist. Margaret was also looking for something similar. Maybe she's got some tests that could use this?

Comment 6

[:Margaret Leibovic]

Comment on attachment 645843 [details] [diff] [review]
Patch 2/2

Marking this as obsolete because I stole it and updated it for bug 840825.

Comment 7

[Geoff Brown [:gbrown]]

(In reply to Wesley Johnston (:wesj) from comment #0)
> Two things I've wanted for robocop:
> 
> 1.) The ability to load pages with chrome privileges. Those pages can then
> register for Java notifications and respond to them.
> 2.) The ability for java to get back responses from the pages.

This need has re-surfaced in bug 855146; see especially comment 16.

Comment 8

[Geoff Brown [:gbrown]]

And the evolution continues...see bug 870908.

I am much happier with the capabilities we have today. I think we can probably close this bug now.

Comment 9

[Wesley Johnston (:wesj)]

Sonds good to me.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch v2

I'm back to wanting this again for bug 757481. This creates the directory if it doesn't exist. It also ignores errors in the copy command because an error gets thrown if the dir is empty.

Comment 11

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/10e47ad3f982

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/10e47ad3f982

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

This needed build peer review.

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 811624 [details] [diff] [review]
Patch v2

No idea who to ask for this stuff.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 811624 [details] [diff] [review]
Patch v2

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

::: testing/mochitest/roboextender/Makefile.in
@@ +5,5 @@
>  
> +DEPTH           = ../../..
> +topsrcdir       = @top_srcdir@
> +srcdir          = @srcdir@
> +VPATH           = @srcdir@

You don't need everything up through here.

@@ +10,5 @@
> +relativesrcdir  = testing/mochitest/roboextender
> +TESTPATH        = $(topsrcdir)/mobile/android/base/tests/roboextender
> +
> +include $(DEPTH)/config/autoconf.mk
> +

You can define relativesrcdir and TESTPATH here.

@@ +26,5 @@
>  	$(MKDIR) -p $(TEST_EXTENSIONS_DIR)/[e-mail]
>  	$(INSTALL) $(foreach f,$^,"$f") $(TEST_EXTENSIONS_DIR)/[e-mail]/
> +	$(MKDIR) -p $(TEST_EXTENSIONS_DIR)/[e-mail]/base
> +	$(MKDIR) -p $(TESTPATH)
> +	-cp $(TESTPATH)/* $(TEST_EXTENSIONS_DIR)/[e-mail]/base

This whole block is suboptimal.

You should use INSTALL_TARGETS for copying files. See toolkit/components/telemetry/Makefile.in.

Comment 16

[Gregory Szorc [:gps]]

Reopening to track fixing the build config badness.

Comment 17

[Gregory Szorc [:gps]]

(In reply to Wesley Johnston (:wesj) from comment #14)
> Comment on attachment 811624 [details] [diff] [review]
> Patch v2
> 
> No idea who to ask for this stuff.

I blame our tools. Read my blog post about Phabricator and try out the demo site I hosted. You can configure Phabricator to have it automatically request review from people when certain files change. It's awesome. I'm using it to audit changes to Makefile.in that didn't go through build peer review. http://gregoryszorc.com/blog/2013/10/14/phabricator-is-awesome/

Comment 18

[Wesley Johnston (:wesj)]

Not going to be able to work on this. nalexander might want to glance at it though :( Sorry for the mess

Comment 19

[Wesley Johnston (:wesj)]

i.e. I know you've seen this code nick. Just making sure you know this bug exists.

Comment 20

[Nick Alexander :nalexander [he/him]]

I think this is RESO FIXED by Robocop's JavascriptTest and Bug 983811.