846257 - Robocop: Extend 'Web Content Context Menu' test to cover the context menu options for images

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Paul Feher]

This bug extends the current 'Web Content Context Menu' tests to cover all context menu options. Also it adds tests that cover the testing on images context menu options.

Comment 1

[Paul Feher]

Attachment: WebContentContextMenu V1

Comment 2

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Comment on attachment 719411 [details] [diff] [review]
WebContentContextMenu V1

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

this patch doesn't apply cleanly and the differences looked like a 15+ minute job to port over, can you please upload a patch that applies to the latest sources on mozilla-central.

Comment 3

[Paul Feher]

Attachment: WebContentContextMenu V2

I've applied the patch on latest sources on mozilla-central.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Comment on attachment 720643 [details] [diff] [review]
WebContentContextMenu V2

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

we need to fix the indentation on this patch.

also the switch statement is overkill.  Since you have a small number of conditions, I would rather use if/else if/else clauses.  Each case is specifically hardcoded for a known link.  Please explain if there is a reason for using switch.

On the positive side this passes consistently on my local tegra and panda, they also pass on try server!

::: build/mobile/robocop/Makefile.in
@@ +35,5 @@
>  _JAVA_TESTS = $(patsubst $(TESTPATH)/%.in,%,$(wildcard $(TESTPATH)/*.java.in))
>  
>  _TEST_FILES = \
>    $(wildcard $(TESTPATH)/*.html) \
> +  $(wildcard $(TESTPATH)/*.jpg) \

why do we have this?  I don't see us adding a .jpg file in this patch.

::: mobile/android/base/tests/BaseTest.java.in
@@ +665,5 @@
>                  }
>              }
>          }
> +
> +		public void bookmark() {

4 space indentation.

@@ +667,5 @@
>          }
> +
> +		public void bookmark() {
> +			mAsserter.ok(true, "Type = " +devType+ " OS" +osVersion, "");
> +			if (devType == "tablet"){

.equals("tablet")

@@ +668,5 @@
> +
> +		public void bookmark() {
> +			mAsserter.ok(true, "Type = " +devType+ " OS" +osVersion, "");
> +			if (devType == "tablet"){
> +				if (osVersion != "4.x"){

!osVersion.equals("4.x)

and:
space between ){

::: mobile/android/base/tests/testWebContentContextMenu.java.in
@@ +74,5 @@
> +	// Load a BigLink test page and test for allowed menu actions
> +	public void verifyLinkContextMenu(String items [], String urls [], String pageTitles []) {
> +		for (int opt = 0; opt < items.length;) {
> +			switch(opt) {
> +				case 0:{

nit: 4 space indentation please!

Comment 5

[Paul Feher]

Attachment: WebContentContextMenu V3

I've made the necessary changes.

Comment 6

[Paul Feher]

(In reply to Joel Maher (:jmaher) from comment #4)
> Comment on attachment 720643 [details] [diff] [review]
> WebContentContextMenu V2
> 
> Review of attachment 720643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> we need to fix the indentation on this patch.

On my text editor gedit the indentation is set to 4 spaces. I've tried other text editor like Notepad++. The patch seems ok on my pc but when it's uploaded the indentation becomes 8 spaces long. I don't know what the problem is.

> 
> also the switch statement is overkill.  Since you have a small number of
> conditions, I would rather use if/else if/else clauses.  Each case is
> specifically hardcoded for a known link.  Please explain if there is a
> reason for using switch.
> 
> On the positive side this passes consistently on my local tegra and panda,
> they also pass on try server!
> 
> ::: build/mobile/robocop/Makefile.in
> @@ +35,5 @@
> >  _JAVA_TESTS = $(patsubst $(TESTPATH)/%.in,%,$(wildcard $(TESTPATH)/*.java.in))
> >  
> >  _TEST_FILES = \
> >    $(wildcard $(TESTPATH)/*.html) \
> > +  $(wildcard $(TESTPATH)/*.jpg) \
> 
> why do we have this?  I don't see us adding a .jpg file in this patch.

Yes we add a .jpg file in order to test the pictures context menu. The final part of the test handles this.

> 
> ::: mobile/android/base/tests/BaseTest.java.in
> @@ +665,5 @@
> >                  }
> >              }
> >          }
> > +
> > +		public void bookmark() {
> 
> 4 space indentation.
> 
> @@ +667,5 @@
> >          }
> > +
> > +		public void bookmark() {
> > +			mAsserter.ok(true, "Type = " +devType+ " OS" +osVersion, "");
> > +			if (devType == "tablet"){
> 
> .equals("tablet")
> 
> @@ +668,5 @@
> > +
> > +		public void bookmark() {
> > +			mAsserter.ok(true, "Type = " +devType+ " OS" +osVersion, "");
> > +			if (devType == "tablet"){
> > +				if (osVersion != "4.x"){
> 
> !osVersion.equals("4.x)
> 
> and:
> space between ){
> 
> ::: mobile/android/base/tests/testWebContentContextMenu.java.in
> @@ +74,5 @@
> > +	// Load a BigLink test page and test for allowed menu actions
> > +	public void verifyLinkContextMenu(String items [], String urls [], String pageTitles []) {
> > +		for (int opt = 0; opt < items.length;) {
> > +			switch(opt) {
> > +				case 0:{
> 
> nit: 4 space indentation please!

Comment 7

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Comment on attachment 723438 [details] [diff] [review]
WebContentContextMenu V3

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

please fix the indentation to be 4 spaces only.  I suggestion you use spaces only, not tabs.

::: mobile/android/base/tests/BaseTest.java.in
@@ +666,5 @@
>              }
>          }
> +
> +		public void bookmark() {
> +			mAsserter.ok(true, "Type = " +devType+ " OS" +osVersion, "");

this looks like a debugging item, not a check we should be reporting on, please remove this.

@@ +671,5 @@
> +			if (devType.equals("tablet")) {
> +				if (!osVersion.equals("4.x")){
> +					Element bookmarkBtn = mDriver.findElement(getActivity(), "bookmark");
> +                	bookmarkBtn.click();
> +				}

the above code is indented too much. please use spaces only, not tabs.

@@ +673,5 @@
> +					Element bookmarkBtn = mDriver.findElement(getActivity(), "bookmark");
> +                	bookmarkBtn.click();
> +				}
> +                else {
> +				mActions.sendSpecialKey(Actions.SpecialKey.MENU);

this one line is indented too much.

::: mobile/android/base/tests/testWebContentContextMenu.java.in
@@ +13,5 @@
>  
> +	@Override
> +	protected int getTestType() {
> +		return TEST_MOCHITEST;
> +	

nit extra line here, and above return is indented too much.

@@ +27,4 @@
>  
> +		verfyLinkContextMenu(linkMenuItems, urls);
> +		verfyMailtoContextMenu(mailtoMenuItems, urls);
> +		verfyPhotoContextMenu(photoMenuItems, urls);

4 space indentation please, this looks like 8 space indentation.

Comment 8

[Paul Feher]

Attachment: WebContentContextMenu V4

I've made the necessary adjustments

Comment 9

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Comment on attachment 725321 [details] [diff] [review]
WebContentContextMenu V4

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

I need to test this on try server still, but my one comment in reviewing the code is below.  Thanks for fixing the indentation!

::: mobile/android/base/tests/testWebContentContextMenu.java.in
@@ +122,5 @@
> +
> +                                // Pause briefly, to ensure that the bookmark addition, above, updates database tables and
> +                                // completes before checking that the bookmark exists.
> +                                // TODO: Find a better way to wait for completion of bookmark operations.
> +                                try { Thread.sleep(2000); } catch(Exception e) {}

does this need a 2000 sleep?  I appreciate this is documented as a TODO, but would 500 or 1000 work here?

Comment 10

[Paul Feher]

(In reply to Joel Maher (:jmaher) from comment #9)
> Comment on attachment 725321 [details] [diff] [review]
> WebContentContextMenu V4
> 
> Review of attachment 725321 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I need to test this on try server still, but my one comment in reviewing the
> code is below.  Thanks for fixing the indentation!
> 
> ::: mobile/android/base/tests/testWebContentContextMenu.java.in
> @@ +122,5 @@
> > +
> > +                                // Pause briefly, to ensure that the bookmark addition, above, updates database tables and
> > +                                // completes before checking that the bookmark exists.
> > +                                // TODO: Find a better way to wait for completion of bookmark operations.
> > +                                try { Thread.sleep(2000); } catch(Exception e) {}
> 
> does this need a 2000 sleep?  I appreciate this is documented as a TODO, but
> would 500 or 1000 work here?

This was included in the initial test so i kept it. I guess that this could be reduced but i don't have any guarantees that the bookmark operations will be completed since we can't test this on Tegra boards wich are very slow devices.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Comment on attachment 725321 [details] [diff] [review]
WebContentContextMenu V4

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

this looks good on try server.

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/af9fa6a7c567