Closed Bug 1174366 Opened 6 years ago Closed 5 years ago

Remove "Edit Site Settings" from Page menu

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox41 affected, firefox44 verified)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox41 --- affected
firefox44 --- verified

People

(Reporter: liuche, Assigned: jonalmeida, Mentored)

References

Details

(Whiteboard: [good first bug][lang=java])

Attachments

(2 files, 1 obsolete file)

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.
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!
Actually, it's in the Page menu, but I don't see it in the urlbar context menu.
Summary: Remove "Edit Site Settings" from Tools menu → Remove "Edit Site Settings" from Page menu
Assignee: nobody → jalmeida
Gingerbread uses the URLBar context menu.
Everything else uses the Page menu.
Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche
Attachment #8630541 - Flags: review?(liuche)
Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

Fixed an inconsistency in the layout that would fail robocop tests.
Attachment #8630542 - Flags: review?(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.
Attachment #8630541 - Flags: review?(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).
Attachment #8630542 - Flags: review?(liuche)
Attachment #8630541 - Flags: feedback+
Attachment #8630542 - Flags: feedback+
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!
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.
Jonathan, any motion on this?
Flags: needinfo?(jonalmeida942)
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.
Flags: needinfo?(jonalmeida942)
(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.
Attachment #8630541 - Flags: feedback+ → review?(liuche)
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
Attachment #8630542 - Attachment is obsolete: true
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.
Attachment #8630541 - Flags: review?(liuche) → review+
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!
How do you feel about reparenting UITest to BaseTest instead of BaseRobocopTest?
Flags: needinfo?(michael.l.comella)
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.
(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.
Flags: needinfo?(michael.l.comella)
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
Attachment #8630541 - Flags: review+ → review?(liuche)
Comment on attachment 8630541 [details]
MozReview Request: Bug 1174366 - Remove "Edit Site Settings" from Page menu. r=liuche

Fixed nits and comments.
Attachment #8630541 - Flags: review?(liuche) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b2fb2a45b780
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.