Closed Bug 1196387 Opened 7 years ago Closed 6 years ago

Remove unneeded "reading time" code and layouts


(Firefox for Android Graveyard :: General, defect)

Not set


(firefox43 fixed)

Firefox 43
Tracking Status
firefox43 --- fixed


(Reporter: sebastian, Assigned: inouju, Mentored)


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


(1 file, 2 obsolete files)

Our reading list had a feature to calculate and display the estimated reading time. This feature had some issues (UX and localization) so we disabled it in bug 1110461. However up to now we still keep the code and layout around.

Let's remove the unused code and the layout. It's in Mercurial so we can always get it back.

To start, set up a build environment - you can see the instructions here:

1. To fix this bug, you need to remove the unused parts in these files:

2. For bonus points, after removing the TextView, you can simplify the structure of reading_list_row_view.xml.

3. For even more bonus points, after cleaning everything up: The Adapter in ReadingListPanel [1] inflates reading_list_item_row.xml for every row . This layout file just contains a ReadingListRow (It's actually a LinearLayout) which again inflates reading_list_row_view.xml in itself. This two step approach could be simplified by moving everything into reading_list_item_row (inside of ReadingListRow) and then getting rid of reading_list_row_view.xml. The findViewById calls in the constructor of ReadlingListRow would need to be moved to onFinishInflate() because we need to wait for the child views to be created.


If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "sebastian" and you can find me and a bunch of other helpful people in #mobile. If you need IRC setup instructions, see
Attached patch bug-1196387-fix (obsolete) — Splinter Review
How's this fix? I went for 3 and removed reading_list_row_view.xml. 

Side question: should unused imports and variables be removed? I removed them in the patch, but they were left in before so I wasn't quite sure on what to do.
Attachment #8653016 - Flags: review?(s.kaspari)
Assignee: nobody → inouju
Comment on attachment 8653016 [details] [diff] [review]

Review of attachment 8653016 [details] [diff] [review]:

Great patch! Just remove the "resources" (see below) and this is ready to land! Thank you! :)

::: mobile/android/base/home/
@@ +32,5 @@
>      public ReadingListRow(Context context, AttributeSet attrs) {
>          super(context, attrs);
> +        resources = context.getResources();

It seems like we assign resources but never use it. Remove it completely. :)
Attachment #8653016 - Flags: review?(s.kaspari) → review+
Attached patch bug-1196387-fix (obsolete) — Splinter Review
I removed the "resources". Also, does that mean that my change broke the build?
Attachment #8653016 - Attachment is obsolete: true
Flags: needinfo?(s.kaspari)
Comment on attachment 8653878 [details] [diff] [review]

Review of attachment 8653878 [details] [diff] [review]:

> I removed the "resources". Also, does that mean that my change broke the
> build?

The errors seems to be unrelated to your changes and to be triggered by some mercurial issue. I re-triggered the jobs on try. I ran a build with the patches locally and don't really expect errors here. :)

::: mobile/android/base/home/
@@ +18,5 @@
>  import android.widget.LinearLayout;
>  import android.widget.TextView;
>  public class ReadingListRow extends LinearLayout {
> +    

NIT: There are some trailing whitespaces in this line.
Attachment #8653878 - Flags: review+
Attached patch bug-1196387-fixSplinter Review
Sorry! Must've overlooked that and an unused import. Should be fixed now.
Attachment #8653878 - Attachment is obsolete: true
Flags: needinfo?(s.kaspari)
Attachment #8653952 - Flags: review?(s.kaspari)
Comment on attachment 8653952 [details] [diff] [review]

Review of attachment 8653952 [details] [diff] [review]:

Great patch. Thank you!
Attachment #8653952 - Flags: review?(s.kaspari) → review+
Sorry for the delay. Make sure to add the "review" flag so that the reviewer will be notified about outstanding reviews. :)

I pushed the patch to try again:

If everything is going to be green then feel free to add the "checkin-needed" keyword to get this patch landed.
Attachment #8653952 - Flags: checkin?(inouju)
Attachment #8653952 - Flags: checkin?(inouju) → checkin?(s.kaspari)
Attachment #8653952 - Flags: checkin?(s.kaspari)
Sorry for the confusion I caused. What I meant is adding the "checkin-needed" keyword to the bug, not on the patch itself. :)
Keywords: checkin-needed
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.