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

Adrian, can you confirm if this is a regression?

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

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

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 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.

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

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 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.

Corrected the nits!

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.

Period and r=lucasr added! :D

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 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?

(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)

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

(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 ;-)

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