Closed Bug 1168808 Opened 5 years ago Closed 5 years ago

Useless call to getNextView in FennecNativeElement

Categories

(Firefox for Android :: Testing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: Sylvestre, Assigned: milo, Mentored)

Details

(Keywords: coverity, Whiteboard: [good first bug][lang=Java][CID 80993])

Attachments

(1 file, 1 obsolete file)

In 
https://dxr.mozilla.org/mozilla-central/source/build/mobile/robocop/FennecNativeElement.java#71
coverity considers that this call:
ts.getNextView();
is only useful for its return value, which is ignored

We should probably remove this declaration.
Hi! I'm new to Mozilla and I'd like to work on this bug.

Looking at the source, getNextView() doesn't appear to have any side effects, so it should definitely be safe to remove this call. I'll try to figure out how to create a patch file and come back here if I need more help.
Attached patch patch (obsolete) — Splinter Review
Alright! Assuming I didn't mess anything up along the way, a patch should be attached which removes the superflous call.

Concerns: I'm not sure exactly how to test the patch and make sure there are no regressions or problems. Robocop still seems to work fine.

Also: My build of Fennec doesn't pass every single Robocop test, even before I made this change. Is that expected?
Flags: needinfo?(sledru)
Attachment #8613021 - Flags: review?(sledru)
The patch looks good to me but I am not the owner of this part of the code. You will need to find a reviewer (looking at the hg history of the file is a way to find out)

By the way, you don't need to n-i + ask for review, one is enough.
Flags: needinfo?(sledru)
Attachment #8613021 - Flags: review?(sledru)
Assignee: nobody → m.lopiccolo3
Mike Comella might be able to help with this bug
Flags: needinfo?(michael.l.comella)
Comment on attachment 8613021 [details] [diff] [review]
patch

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

I'm not familiar with TextSwitcher.getNextView, but it appears to have no side effects so if we don't use the return value, it serves no purpose. Removing the method works for me.

Thanks, Michael!

By the way, can you reformat your commit message? It should be of the format, "Bug ### - Commit summary". Thanks!
Attachment #8613021 - Flags: review+
(In reply to Michael LoPiccolo (:milo) from comment #2)
> Concerns: I'm not sure exactly how to test the patch and make sure there are
> no regressions or problems. Robocop still seems to work fine.

Ideally, you can run a robocop test or two locally to test it out (specifically tests that would use this code). If Robocop still seems to run fine, then sounds good to me!

Additionally, you can run your commit on our try test servers, which will run the full test suite remotely (it takes much too long to run it locally). In order to do this, you must sign a committer agreement and request level 1 access [1]. For the purposes of this bug, I'll make the push for you, but you can make future pushes if you'd like!

> Also: My build of Fennec doesn't pass every single Robocop test, even before
> I made this change. Is that expected?

We have many intermittents on file (e.g. [2]) - the failures could be related to those. If you're running a test or two locally, you can just re-run the test enough times to know whether there is a correlation from your changes. The trends will be clearer on our test servers though.

Note also that we don't test on every device configuration so something may be failing due to your test device. On the test servers, we have 2.3 and 4.3 emulators, and 4.0 xlarge tablet devices (e.g. I've had failures running locally on a large tablet device).

[1]: https://www.mozilla.org/en-US/about/governance/policies/commit/
[2]: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=product%3A%22Firefox%20for%20Android%22%20intermittent&list_id=12297287
Flags: needinfo?(michael.l.comella)
The push to our try test servers is above.

Once it goes green, feel free to add the checkin-needed keyword [1]. Let me know if you need help reading the results. Note that all patches that use "checkin-needed" must also have an associated green try run.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Mentor: michael.l.comella
Thanks so much, Michael. I'm stunned by how helpful the whole Mozilla community has been.

Uploading a new patch with a proper commit message.
Attachment #8613021 - Attachment is obsolete: true
Attachment #8614222 - Flags: review?(michael.l.comella)
Comment on attachment 8614222 [details] [diff] [review]
Fixed commit message

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

Nice! By the way, if you get an r+ and there are some notes on things to fix, fix it (or explain why you don't think you should have to), and just re-apply the r+ yourself (i.e. you don't have to reflag me for review!).

If you decide that you don't want to fix something and you think it's a contentious issue that should delay landing the patch, you should probably add a question and a needinfo flag too!
Attachment #8614222 - Flags: review?(michael.l.comella) → review+
Doesn't look like a green run. Where do we go from here? I assume we have to figure out which revision caused the problem with testSettingsMenu.
Flags: needinfo?(michael.l.comella)
(In reply to Michael LoPiccolo (:milo) from comment #11)
> Doesn't look like a green run. Where do we go from here? I assume we have to
> figure out which revision caused the problem with testSettingsMenu.

I'm actually starting to suspect this is broken, but I don't see us running robocop on 4.0 devices on our check-ins so no one has caught it yet. I have a similar failure in:

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

I'll investigate.

The failures on 4.3 I suspect are intermittent so I'm re-running them - if they go green, feel free to check in because I don't think your patch is to blame.
Flags: needinfo?(michael.l.comella)
(In reply to Michael Comella (:mcomella) from comment #12)
> > figure out which revision caused the problem with testSettingsMenu.
> 
> I'm actually starting to suspect this is broken

This being the test is broken in the tree, not from your patch.
(In reply to Michael Comella (:mcomella) from comment #13)
> (In reply to Michael Comella (:mcomella) from comment #12)
> > > figure out which revision caused the problem with testSettingsMenu.
> > 
> > I'm actually starting to suspect this is broken
> 
> This being the test is broken in the tree, not from your patch.

I filed bug 1171789.
Alright! So if we can ignore the testSettingsMenuItems failure, and the other failures are intermittent, we can go to checkin-needed! (I hope.)
Keywords: checkin-needed
Michael, as a follow-up, perhaps you'd like to take a look at bug 1105271 (requires tablet) or bug 1003608 (a little more setup is involved here)?
https://hg.mozilla.org/mozilla-central/rev/8738bdb29cb4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.