1228170 - Remove legacy menu code from AppMenuComponent

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

In bug 1209967, we're always showing the overflow menu and the legacy menu is never shown – remove the code!

Comment 1

[Tushar Saini (:shatur)]

Hii mcomella. I think this will ba a great opportunity for me to work on my first bug.
Please let me know, if I can work on it and be of any help.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to shatur from comment #1)
> Hii mcomella. I think this will ba a great opportunity for me to work on my
> first bug.
> Please let me know, if I can work on it and be of any help.

Hi Shatur! The bug is not claimed yet, so you can start working on it. :) However you might need to wait until bug 1209967 lands - the patches there will introduce the necessary prerequisite to remove the code here.

Do you already have a build environment set up? You can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android

AppMenuComponent is used in automated tests. We are using robocop to run them, more here: https://wiki.mozilla.org/Mobile/Fennec/Android/Testing#robocop

If you need any help, you can find mcomella and me on IRC (irc.mozilla.org #mobile): https://wiki.mozilla.org/IRC

Comment 3

[Tushar Saini (:shatur)]

Thanks Sebastian for this Information. I will keep an eye on bug 1209967 myself, but in case I couldn't, please do ping me when it is resolved.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

I'll try to ping you.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hey shatur – bug 1209967 has landed so this bug is all ready to go!

You'll be able to find the testing code in `/mobile/android/tests/browser/robocop/src/`. See AppMenuComponent [1].

Let me or Sebastian know if you need more details on what to do here.

[1]: http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/AppMenuComponent.java

Comment 6

[Tushar Saini (:shatur)]

Hey Michael. 

  I went through the code "mozilla-central/source/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/AppMenuComponent.java". 
      Can you point me to right direction, which part I have to remove or modify.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

You want to remove any branches with the `hasLegacyMenu` call and the method itself.

Comment 8

[Tushar Saini (:shatur)]

Attachment: AppMenuComponent.java

Removed any branches 'hasLegacyMenu' call.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8703248 [details] [diff] [review]
AppMenuComponent.java

Hey, shatur.

Your patch looks improperly formatted and I can't read it on bugzilla (e.g. click the review button). It looks like entire file is uploaded, rather than just the delta (which is a patch file). Check out the documentation at [1] and if you still need help, let me know.

Clearing review until the patch is properly formatted.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 10

[Tushar Saini (:shatur)]

Attachment: Removed 'hasLegacyMenu' method and all its branches.

Hey Michael.

Sorry for the inconvenience caused.:)
    
   Tell me if patch needs any modification.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8706002 [details] [diff] [review]
Removed 'hasLegacyMenu' method and all its branches.

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

This is looking pretty good, Shatur! Just a few nits and I think we can land it.

Sorry for the delay in responding to this review – I've been caught up in this work so you can also flag :sebastian for your reviews as well if you want to make sure they're responded to in a timely fashion.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/AppMenuComponent.java
@@ +37,1 @@
>  

nit: You can remove the additional line of whitespace here – we generally never leave two vertical lines of whitespace.

@@ +85,5 @@
>      private void assertMenuIsNotOpen() {
>          fAssertFalse("Menu is not open", isMenuOpen());
>      }
>  
> +    

nit: Same for two lines. Also, there is a few spaces here – we generally try to remove excess whitespace as far as spaces are concerned as well.

@@ +94,1 @@
>              // Non-legacy devices have hierarchical menu, check for parent menu item "page".

nit: You should fix the indentation on this code now that the containing if block is removed.

@@ +108,5 @@
>              } else {
>                  fAssertFalse("The parent 'page' menu item is not enabled", parentMenuItemView.isEnabled());
>                  fAssertEquals("The parent 'page' menu item is visible", View.VISIBLE, parentMenuItemView.getVisibility());
>              }
> +     

nit: Remove the excess whitespace (there's a space here).

@@ +179,1 @@
>              fAssertTrue("Menu is open", isMenuOpen(menuItemView));

nit: Fix the indentation.

@@ +191,1 @@
>              pressMenuItem(parentMenuItemTitle);

nit: fix indentation

@@ +191,5 @@
>              pressMenuItem(parentMenuItemTitle);
>  
>              // Child menu item is not pressed yet, Click on it.
>              pressMenuItem(childMenuItemTitle);
> +        

nit: whitespace

Comment 12

[Tushar Saini (:shatur)]

Attachment: AppMenuComponent.patch

Hii Sebastian.
  
  I have removed knits, pointed out by Michael. Please tell me if anything needs modification.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8711406 [details] [diff] [review]
AppMenuComponent.patch

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

Hi Shatur! Something went wrong with creating this patch. It includes the previous patch (AppMenuComponent.patch).

Try to fold those changes into one patch. If you want to update your last commit then use "hg commit --amend".

It seems likere there are some unnecessary whitespaces left too, you can see them in red here:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1228170&attachment=8711406

Comment 14

[Tushar Saini (:shatur)]

Attachment: AppMenuComponent.patch

Hi Sebastian.
         I followed the method stated by you. Please do tell me if any further changes is required.

Comment 15

[Tushar Saini (:shatur)]

Attachment: AppMenuComponent.patch

Hi Sebastian.
   Followed your described process and thanks for the help.

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8712703 [details] [diff] [review]
AppMenuComponent.patch

Hi Shatur!

The patch applies cleanly but I see a bunch of errors when compiling:
https://pastebin.mozilla.org/8857999

Does this build for you? Are you sure your patch contains all your changes? (Try "hg log -f" to see whether you maybe created multiple commits).

Comment 17

[Tushar Saini (:shatur)]

Attachment: AppMenuComponent.patch

Hi Sebastian.
 I am able to build the code without any errors. The process I followed to build is |./mach build |, after committing my changes. And I am getting output |Your build was successful!|

          I am uploading my patch again. Please do tell me If I am doing something wrong. And sorry for the inconvenience caused by me. :-)

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8714006 [details] [diff] [review]
AppMenuComponent.patch

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

(In reply to shatur from comment #17)
> Created attachment 8714006 [details] [diff] [review]
> AppMenuComponent.patch
> 
> Hi Sebastian.
>  I am able to build the code without any errors. The process I followed to
> build is |./mach build |, after committing my changes. And I am getting
> output |Your build was successful!|

This is good. So locally for you everything seems to be fine.

>           I am uploading my patch again. Please do tell me If I am doing
> something wrong. And sorry for the inconvenience caused by me. :-)

I still see the build errors and looking at the patch something seems to be missing. For example in pressMenuItem() an if statement is being removed but the "else" part is still there.

Did you export the patch again? Did you check your log (hg log -f) if there are multiple commits and you just exported one? How did you create the patch? Are you using patch queues or bookmarks?

Comment 19

[Tushar Saini (:shatur)]

Attachment: AppMenuComponent.patch

Hi Sebastian.
   I am uploading this patch, which I build from scratch. (Truly sorry for above patches)

   I have also removed these two methods, which I don't think will be needed as the calling method has also been removed.
     -  private void openLegacyMoreMenu()
     - private boolean isLegacyMoreMenuOpen()

    Hope this will fix this bug.
Thanks

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8715290 [details] [diff] [review]
AppMenuComponent.patch

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

Thank your for updating the patch! This looks good. I'll push the patch to try and then let's see if all tests pass.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/AppMenuComponent.java
@@ +89,4 @@
>      public void assertMenuItemIsDisabledAndVisible(PageMenuItem pageMenuItem) {
>          openAppMenu();
>  
> +        // Non-legacy devices have hierarchical menu, check for parent menu item "page".

NIT: Remove "Non-legacy devices have hierarchical menu" from the comment.

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29b6ca05e755

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

The tests have passed. Let's land this. :)

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/62437a812387

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62437a812387

Comment 25

[Geoff Brown [:gbrown]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7cf9bfcf9033