920935 - Reading list items opened using keywords are not opened in Reading List

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Adrian Tamas (:AdrianT)]

Nightly 27.0a1 2013-09-25
Samsung Galaxy Tab 2 (Android 4.1)

Steps to reproduce:
1) Load news.google.com and load any article that supports reader mode
2) Add the article to reading list by long tapping on the reader mode button in the title bar
3) Go to the Reading List page and add a keyword to the item
4) Close all tabs and open the reader item by typing the keyowrd in the url bar and pressing enter

Expected results:
The item is opened in reading list

Actual results:
The item is opened as a normal bookmark not in Reader Mode

Comment 1

[Aaron Train [:aaronmt]]

Adrian, can you confirm if this is a regression?

Comment 2

[Adrian Tamas (:AdrianT)]

Not a regression. Reproducible on Firefox Mobile 24 and 25 beta 2

Comment 3

[Sunny [:darkowlzz]]

Attachment: Patch1

Comment 4

[Sunny [:darkowlzz]]

Attachment: Tested and modified suggestion list items to open in ReaderMode

Taking this bug and submitting a patch, which seems to work fine :)

Comment 5

[Mark Capella [:capella]]

Comment on attachment 819454 [details] [diff] [review]
Tested and modified suggestion list items to open in ReaderMode

This reflects what we discussed on irc :)

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 819454 [details] [diff] [review]
Tested and modified suggestion list items to open in ReaderMode

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

Thanks for the patch Sunny! Unfortunately, this will cause browser search to open any URLs in reading list directly in reader mode which is not the intended design. For instance, this is why we added the 'Open in Reader Mode' item in the context menu for the reading list items in the search results.

I've never looked into how we handle bookmark keywords, but the right thing to do here is probably to check if the bookmark with keyword is in the reading list before opening it.

Comment 7

[Lucas Rocha (:lucasr)]

The change should probably go somewhere around here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1467

Comment 8

[Sunny [:darkowlzz]]

Attachment: Added a condition to check for pages with keyword to be present in ReadingList

The previous patch was the result of misunderstanding the term `keyword` :)

This patch seems to do what is expected.
Please test it and suggest any improvement in code format, if required.

Thanks!

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 821691 [details] [diff] [review]
Added a condition to check for pages with keyword to be present in ReadingList

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

Looks good to me :-)

::: mobile/android/base/BrowserApp.java
@@ +1472,5 @@
>                      Tabs.getInstance().loadUrl(url, Tabs.LOADURL_USER_ENTERED);
>                      return;
>                  }
>  
> +                // If the keywordUrl is in ReadingList, convert the url to AboutReader url and load

nit: "convert the url to an about:reader url and load it"

@@ +1473,5 @@
>                      return;
>                  }
>  
> +                // If the keywordUrl is in ReadingList, convert the url to AboutReader url and load
> +                final boolean inReadingList = BrowserDB.isReadingListItem(getContentResolver(), keywordUrl);

Maybe this looks a bit cleaner?
final ContentResolver cr = getContentResolver();
final boolean inReadingList = BrowserDB.isReadingListItem(cr, keywordUrl);

@@ +1476,5 @@
> +                // If the keywordUrl is in ReadingList, convert the url to AboutReader url and load
> +                final boolean inReadingList = BrowserDB.isReadingListItem(getContentResolver(), keywordUrl);
> +                if (inReadingList) {
> +                    final String readerUrl;
> +                    readerUrl = ReaderModeUtils.getAboutReaderForUrl(keywordUrl);

nit: do the assingment in the same line than the var declaration:
final String readerUrl = ReaderModeUtils.getAboutReaderForUrl(keywordUrl);

@@ +1477,5 @@
> +                final boolean inReadingList = BrowserDB.isReadingListItem(getContentResolver(), keywordUrl);
> +                if (inReadingList) {
> +                    final String readerUrl;
> +                    readerUrl = ReaderModeUtils.getAboutReaderForUrl(keywordUrl);
> +                    Tabs.getInstance().loadUrl(readerUrl);

You should still keep the Tabs.LOADURL_USER_ENTERED flag in the loadUrl() call.

Comment 10

[Sunny [:darkowlzz]]

Attachment: Added a condition to check for pages with keyword to be present in ReadingList

Corrected the nits!

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 823928 [details] [diff] [review]
Added a condition to check for pages with keyword to be present in ReadingList

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

Nice, thanks!

::: mobile/android/base/BrowserApp.java
@@ +1473,5 @@
>                      Tabs.getInstance().loadUrl(url, Tabs.LOADURL_USER_ENTERED);
>                      return;
>                  }
>  
> +                // If the keywordUrl is in ReadingList, convert the url to an about:reader url and load it

nit: add a period at the end of the comment.

Comment 12

[Sunny [:darkowlzz]]

Attachment: Added a condition to check for pages with keyword to be present in ReadingList

Period and r=lucasr added! :D

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/90d6f7ce0fc2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90d6f7ce0fc2

Comment 15

[Aaron Train [:aaronmt]]

This doesn't work at all for me. I add a keyword to a reading list item, query the item with the string used in the keyword field for the reading list item and my query yields 0 results.

Also, this patch has no review + on it?

Comment 16

[Sunny [:darkowlzz]]

I have tested it and yes, it isn't working.
Logs shows that the conversion of url to about:reader works fine, but when the url is passed to loadUrl, it doesn't load.

lucasr any idea why doesn't it loading?

Comment 17

[Lucas Rocha (:lucasr)]

(In reply to Aaron Train [:aaronmt] from comment #15)
> This doesn't work at all for me. I add a keyword to a reading list item,
> query the item with the string used in the keyword field for the reading
> list item and my query yields 0 results.
> 
> Also, this patch has no review + on it?

I gave r+ (see comment #11)

Comment 18

[Sunny [:darkowlzz]]

Hi Lucas,

This patch seems to work now, but Editing option is going away from Reading List as per [1].
So this bug becomes a WONTFIX.
But the patch has already been checked-in. How do we back it out?

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=930589#c5

Comment 19

[Lucas Rocha (:lucasr)]

(In reply to Sunny [:darkowlzz] from comment #18)
> Hi Lucas,
> 
> This patch seems to work now, but Editing option is going away from Reading
> List as per [1].
> So this bug becomes a WONTFIX.
> But the patch has already been checked-in. How do we back it out?
> 
> [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=930589#c5

Done: https://hg.mozilla.org/integration/fx-team/rev/ec8042ab4075

Thanks for the patch anyway ;-)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/ec8042ab4075