Closed Bug 944144 Opened 7 years ago Closed 7 years ago

Implement NavigationHelper.reload

Categories

(Firefox for Android :: Testing, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: mcomella, Assigned: vivek)

References

Details

(Whiteboard: [mentor=mcomella][lang=java])

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → s.kaspari
bug 944142 implemented the AppMenuComponent so this is dependent.
Status: NEW → ASSIGNED
Depends on: 944142
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()
Attachment #8379209 - Flags: review?(michael.l.comella)
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!
Attachment #8379209 - Flags: review?(michael.l.comella) → review-
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).
Assignee: s.kaspari → vivekb.balakrishnan
Whiteboard: [mentor=mcomella][lang=java]
Blocks: 967139
No longer depends on: 967139
Oh, okay. Yeah, it took way too much time for me to continue on that after my vacation.
Attached patch 944144.patchSplinter Review
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.
Attachment #8379209 - Attachment is obsolete: true
Attachment #8419066 - Flags: review?(michael.l.comella)
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?
Attachment #8419066 - Flags: review?(michael.l.comella) → review+
Keywords: checkin-needed
Duplicate of this bug: 967139
vivek, can you please post a link to a recent Try run before requesting checkin? Thanks!
Keywords: checkin-needed
The try server logs can be found here
https://tbpl.mozilla.org/?tree=Try&rev=95509612118b
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!
Keywords: checkin-needed
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
Keywords: checkin-needed
Whiteboard: [mentor=mcomella][lang=java] → [mentor=mcomella][lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/567c1b6a0988
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=mcomella][lang=java][fixed-in-fx-team] → [mentor=mcomella][lang=java]
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.