944144 - Implement NavigationHelper.reload

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Comment 1

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

bug 944142 implemented the AppMenuComponent so this is dependent.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 944144_NavigationHelper_reload.patch

This is my patch for NavigationHelper.reload() and enabling the related test case.

I wonder if it would make sense to add another test case that verifies that the website has actually been reloaded. The current test case only checks if the title of the page is the same as before.

Maybe a test case loading a page that sets the title using Performance.now()[1] and then, after reloading, verifies that the title has been changed to a number higher than the first load? 

[1] https://developer.mozilla.org/en-US/docs/Web/API/Performance.now()

Comment 3

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

Comment on attachment 8379209 [details] [diff] [review]
944144_NavigationHelper_reload.patch

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

The ToolbarTextChangeVerifier will have to timeout in waitForPageLoad - I wonder if we shouldn't remove the ToolbarTextChangeVerifier in this case (perhaps by taking in a `boolean expectTitleChange` or similar). Handle this or file a followup!

::: mobile/android/base/tests/helpers/NavigationHelper.java
@@ +94,5 @@
> +            sToolbar.pressReloadButton();
> +            return;
> +        }
> +
> +        sToolbar.assertIsNotEditing();

I think we should make this assertion for both device types (especially with bug 965548 alive).

@@ +95,5 @@
> +            return;
> +        }
> +
> +        sToolbar.assertIsNotEditing();
> +        WaitHelper.waitForPageLoad(new Runnable() {

Wait for pageload on tablet too!

Comment 4

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

Sebastian, vivek is taking bug 967139 so I'm going to assign him to this too. Let me know if you want me to find you anymore bugs. :)

vivek, feel free to use Sebastian's WIP (or not).

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Oh, okay. Yeah, it took way too much time for me to continue on that after my vacation.

Comment 6

[Vivek Balakrishnan[:vivek]]

Attachment: 944144.patch

This patch is equivalent to Kaspari's patch with a nit corrected and comment added.Fn pressReloadButton asserts toolbar is not in editing state and waits for page load. Added a comment to clarify it.

Comment 7

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

Comment on attachment 8419066 [details] [diff] [review]
944144.patch

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

lgtm.

(In reply to Michael Comella (:mcomella) from comment #3)
> The ToolbarTextChangeVerifier will have to timeout in waitForPageLoad - I
> wonder if we shouldn't remove the ToolbarTextChangeVerifier in this case
> (perhaps by taking in a `boolean expectTitleChange` or similar). Handle this
> or file a followup!

Can you file a followup for this?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

vivek, can you please post a link to a recent Try run before requesting checkin? Thanks!

Comment 10

[Vivek Balakrishnan[:vivek]]

The try server logs can be found here
https://tbpl.mozilla.org/?tree=Try&rev=95509612118b

Comment 11

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

vivek, don't forget to re-add the "checkin-needed" flag!

Also, if you think a try run is relevant to the bug (e.g. large changes, adding/changing tests), it's generally a good idea to post a Try run - use your judgment!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Thanks for the patch, Vivek! To clarify, in general, we request that there always be a Try run posted when requesting checkin on anything that affects the code that gets built or the tests that are run. Too many instances of things going wrong in the past :)

Also, for future patches, please make sure you include a commit message with the patch. Thanks!
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/567c1b6a0988