1174366 - Remove "Edit Site Settings" from Page menu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Since we expose this UI in the Site Identity doorhanger, which is more accessible than in Tools, we should remove "Edit Site Settings" from Tools.

Remember to update testSettingsMenuItems.

Comment 1

[:Margaret Leibovic]

I don't think it's in Tools anymore, only in the context menu on the urlbar.

But +1 to only having UI for one thing in one place!

Comment 2

[Chenxia Liu [:liuche]]

Actually, it's in the Page menu, but I don't see it in the urlbar context menu.

Comment 3

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38659f42c16c

Comment 4

[Jonathan Almeida [:jonalmeida]]

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

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Gingerbread uses the URLBar context menu.
Everything else uses the Page menu.

Comment 6

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e8a2eae34d3

Comment 7

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26a416813638

Comment 8

[Jonathan Almeida [:jonalmeida]]

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

Comment 9

[Jonathan Almeida [:jonalmeida]]

Attachment: MozReview Request: Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

Comment 10

[Jonathan Almeida [:jonalmeida]]

Attachment: MozReview Request: Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

Fixed an inconsistency in the layout that would fail robocop tests.

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8630541 [details]
MozReview Request: Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

https://reviewboard.mozilla.org/r/12767/#review11341

::: mobile/android/tests/browser/robocop/testClearPrivateData.java:67
(Diff revision 1)
> -        String shareStrings[] = {"Share your location with", "Share", "Don't share", "There are no settings to clear"};
> +        String shareStrings[] = {"Share your location with", "Share", "Don't share"};

Are these strings ones that we can pull from R.strings? If they are (and I'm not sure they are!), let's move them into StringHelper - that way we don't need to maintain strings in two places.

::: mobile/android/tests/browser/robocop/components/ToolbarComponent.java:264
(Diff revision 1)
> +        pressButton(siteSecurityButton, "site security");

Nit: connect these with an underscore. Spaces create ambiguity in parsing.

::: mobile/android/tests/browser/robocop/components/ToolbarComponent.java:317
(Diff revision 1)
> +    private ImageButton waitForDoorHanger() {

Nit: Rename this to waitForDoorhangerButton. That way, it makes sense why we return an ImageButton.

::: mobile/android/tests/browser/robocop/UITest.java:29
(Diff revision 1)
> -abstract class UITest extends BaseRobocopTest
> +abstract class UITest extends BaseTest

Hmmm, we really shouldn't change this from BaseRobocopTest to BaseTest - UITest was created so that we could move away from BaseTest (which has a lot of kind of buggy, race-y problems), and start using better Robocop patterns.

Can you add the pieces of code that you need to either UITest or BaseRobocopTest?

Also, you can chat with mcomella about this - he wrote UITest/BaseRobocopTest, so he can clarify what the intention of UITest is.

Comment 12

[Chenxia Liu [:liuche]]

Comment on attachment 8630542 [details]
MozReview Request: Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

https://reviewboard.mozilla.org/r/12769/#review11343

Fold this into the first patch, actually.

::: mobile/android/tests/browser/robocop/components/ToolbarComponent.java:324
(Diff revision 1)
> -                    return true;
> +                    btn = mSolo.getImageButton(5);

Hm, let's put in a comment here - I'm kind of afraid that this is too fragile :/ and hard-coding indices is generally bad for maintenance, but add a comment and explain where you're getting this index from (or maybe what the other 5 items are, or how to get the right index in case something changes).

Comment 13

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/12767/#review11341

> Hmmm, we really shouldn't change this from BaseRobocopTest to BaseTest - UITest was created so that we could move away from BaseTest (which has a lot of kind of buggy, race-y problems), and start using better Robocop patterns.
> 
> Can you add the pieces of code that you need to either UITest or BaseRobocopTest?
> 
> Also, you can chat with mcomella about this - he wrote UITest/BaseRobocopTest, so he can clarify what the intention of UITest is.

There are uses for `loadUrlAndWait`, `verifyUrlBarTitle` and `waitForText` from BaseTest. It does make sense to just inherit straight from BaseTest instead now that I think about it. I needed to change to the BaseTest because I needed to use the NavigationHelper which is only instantiated from there (not PixelTest, and we don't use anything particular in PixelTest either). Will verify with mcomella as well when I refactor this bit.

> Are these strings ones that we can pull from R.strings? If they are (and I'm not sure they are!), let's move them into StringHelper - that way we don't need to maintain strings in two places.

You're right, they are. I'm not even sure why this is in an array anymore.. Thanks for the catch!

Comment 14

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/12769/#review11343

> Hm, let's put in a comment here - I'm kind of afraid that this is too fragile :/ and hard-coding indices is generally bad for maintenance, but add a comment and explain where you're getting this index from (or maybe what the other 5 items are, or how to get the right index in case something changes).

Hardcoding indexes was the only way to get robocop to find the view. Using the id just didn't work in a tablet view for some unknown reason which is why this fix got delayed. I had to brute force the tests until I found the index that worked (!). Will update soon.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Jonathan, any motion on this?

Comment 16

[Jonathan Almeida [:jonalmeida]]

I haven't actively worked on this since the fix for the tests was taking too long at the time without any progress.

I'll have a go at it again today and see what I can do. Otherwise, I'll unassign myself and leave my latest patch.

Comment 17

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=110da203233f

Comment 18

[Jonathan Almeida [:jonalmeida]]

(In reply to Jonathan Almeida (:jonalmeida) from comment #17)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=110da203233f

So this finally passes the tests somehow..

I'm just going to thank the robocop gods (gbrown) for it and I'll be on my way.

Comment 19

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8630541 [details]
MozReview Request: Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

Comment 20

[Chenxia Liu [:liuche]]

Comment on attachment 8630541 [details]
MozReview Request: Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

https://reviewboard.mozilla.org/r/12767/#review19869

Going to flag mcomella for info on the UITest differences.

Comment 21

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/12765/#review19867

::: mobile/android/tests/browser/robocop/UITest.java:36
(Diff revision 2)
> -abstract class UITest extends BaseRobocopTest
> +abstract class UITest extends BaseTest

What was the deal with UITest vs BaseTest again? BaseTest already inherits from BaseRobocopTest, so perhaps you should just use BaseTest. I recall there being some major differences between UITest and BaseTest, regarding less racy-ness, but I don't actually see timing improvements in UITest that are different.

::: mobile/android/tests/browser/robocop/components/ToolbarComponent.java:265
(Diff revision 2)
> +        pressButton(siteSecurityButton, "site security");

Carrying over the rewiew comment from last time, about using an underscore.

::: mobile/android/tests/browser/robocop/components/ToolbarComponent.java:318
(Diff revision 2)
> +    private ImageButton waitForDoorHanger() {

Carrying over from previous review, rename to waitForDoorhangerButton so it makes a little more sense why this method returns a Button.

::: mobile/android/tests/browser/robocop/testClearPrivateData.java:70
(Diff revision 2)
> -        checkOption(shareStrings[1], "Clear");
> +        checkOption(mStringHelper.GEO_ALLOW, "Clear");

Nit: I think "Clear" and "Cancel" are also strings that you can pull from the product!

Comment 22

[Chenxia Liu [:liuche]]

How do you feel about reparenting UITest to BaseTest instead of BaseRobocopTest?

Comment 23

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=93e1fc39da2b

Comment 24

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/12765/#review19867

> What was the deal with UITest vs BaseTest again? BaseTest already inherits from BaseRobocopTest, so perhaps you should just use BaseTest. I recall there being some major differences between UITest and BaseTest, regarding less racy-ness, but I don't actually see timing improvements in UITest that are different.

These were some changes that carried over from a previous attempt when I was trying to modify UITest to find out why the tests were failing; I removed it from this review. mcomella mentioned then that we shouldn't be doing this.

Thanks for the catch!

> Carrying over from previous review, rename to waitForDoorhangerButton so it makes a little more sense why this method returns a Button.

I could have sworn I fixed these earlier..

> Nit: I think "Clear" and "Cancel" are also strings that you can pull from the product!

I added these strings to StringHelper.

Comment 25

[Jonathan Almeida [:jonalmeida]]

(In reply to Jonathan Almeida (:jonalmeida) from comment #23)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=93e1fc39da2b

Will put this up for review once this runs on try again.

Removing NI as well since it's not needed anymore.

Comment 26

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8630541 [details]
MozReview Request: Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

Comment 27

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8630541 [details]
MozReview Request: Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

Fixed nits and comments.

Comment 28

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Thanks Jonathan!

Comment 29

[Pulsebot]

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

Comment 30

[Carsten Book [:Tomcat]]

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

Comment 31

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-10-19-15-08-25.png

Verified as fixed using:
Device: LG G4 (Android 5.1)
Build: Firefox for Android 44.0a1 (2015-10-19)

"Edit Site Settings" option was removed from Page menu