Closed Bug 977167 Opened 10 years ago Closed 10 years ago

Open tabs should be filtered to exclude about:, chrome: etc. URLs prior to flushing to DB

Categories

(Firefox for Android Graveyard :: Data Providers, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 32

People

(Reporter: rnewman, Assigned: vivek, Mentored)

References

Details

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

Attachments

(1 file, 4 obsolete files)

I'd take as an alternative that these should be filtered out prior to upload, in which case move this to Android Background Services : Android Sync.

See also Bug 977164, which is to not display these in the UI when they end up in the DB.
Blocks: 977161
Whiteboard: [mentor=rnewman][lang=java][good first bug]
I would like to work on this bug. Can u help me on it?
Assignee: nobody → vivekb.balakrishnan
Status: NEW → ASSIGNED
Flags: needinfo?(rnewman)
vivek: you'll need to build Firefox for Android:

https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions#Getting_started
https://wiki.mozilla.org/Mobile/Fennec/Android

Let us know when you get to the point of being able to run Robocop tests.
Flags: needinfo?(rnewman)
I've a working Fennec build.
Great!

Your goal is to produce the following:

* Changes to mobile/android/base/TabsAccessor.java to ensure that insertLocalTabs doesn't write unwanted tabs to the DB.
* JUnit3 (Bug 903528) or Robocop (https://wiki.mozilla.org/Auto-tools/Projects/Robocop) tests for TabsAccessor to verify this behavior.

The check should be a separate static method, and should mirror 

services/sync/services-sync.js
33:pref("services.sync.engine.tabs.filteredUrls", "^(about:.*|chrome://weave/.*|wyciwyg:.*|file:.*)$");
Attached patch bug 977167 patch (obsolete) — Splinter Review
Open tabs from local client filtered in TabsAccessor prior to upload. New content provider test added to verify it.
Attachment #8409098 - Flags: review?(rnewman)
Comment on attachment 8409098 [details] [diff] [review]
bug 977167 patch

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

This looks great, Vivek!

Comments inline.

::: mobile/android/base/TabsAccessor.java
@@ +49,5 @@
>      private static final String TABS_SELECTION = BrowserContract.Tabs.CLIENT_GUID + " IS NOT NULL";
>  
>      private static final String LOCAL_CLIENT_SELECTION = BrowserContract.Clients.GUID + " IS NULL";
>      private static final String LOCAL_TABS_SELECTION = BrowserContract.Tabs.CLIENT_GUID + " IS NULL";
> +    private static final Pattern FILTERED_URL_PATTERN = Pattern.compile("^(about:.*|chrome://weave/.*|wyciwyg:.*|file:.*)$");

Let's make that chrome:.* -- no need for limiting it to weave URLs.

We should probably make this simply

  "^(about|chrome|wyciwyg|file):"

@@ +150,5 @@
>          ArrayList<ContentValues> valuesToInsert = new ArrayList<ContentValues>();
>  
>          int position = 0;
>          for (Tab tab : tabs) {
> +            // Skip this tab if it has a null URL or is in private browsing mode or is a Filtered Url

// Skip this tab if it has a null URL, is in private browsing mode, or is a filtered URL.

@@ +194,5 @@
>          insertLocalTabs(cr, tabs);
>          updateLocalClient(cr);
>      }
> +
> +    // Matches the url against the pattern to check if it is a filteredUrl

JavaDoc:

/**
 * Matches the supplied URL string against the set of URLs to filter.
 *
 * @return true if the supplied URL should be skipped; false otherwise.
 */

@@ +195,5 @@
>          updateLocalClient(cr);
>      }
> +
> +    // Matches the url against the pattern to check if it is a filteredUrl
> +    private static boolean isFilteredUrl(String url) {

isFilteredURL

::: mobile/android/base/tests/testFilterOpenTab.java
@@ +50,5 @@
> +                               null);
> +    }
> +
> +    private Tab createTab(int id, String url, boolean external, int parentId, String title) {
> +        return new Tab((Context)getActivity(), id, url, external, parentId, title);

Space between cast and value.

@@ +53,5 @@
> +    private Tab createTab(int id, String url, boolean external, int parentId, String title) {
> +        return new Tab((Context)getActivity(), id, url, external, parentId, title);
> +    }
> +
> +    private Tab createPirvateTab(int id, String url, boolean external, int parentId, String title) {

s/Pirvate/Private

@@ +54,5 @@
> +        return new Tab((Context)getActivity(), id, url, external, parentId, title);
> +    }
> +
> +    private Tab createPirvateTab(int id, String url, boolean external, int parentId, String title) {
> +        return new PrivateTab((Context)getActivity(), id, url, external, parentId, title);

Space.

@@ +101,5 @@
> +            tabs.add(tab4);
> +            tabs.add(tab5);
> +            tabs.add(tab6);
> +
> +            // Persist the created tabs

Comments should be sentences -- closing period, please.
Attachment #8409098 - Flags: review?(rnewman) → feedback+
Attached patch 977167.patch (obsolete) — Splinter Review
Review comments incorporated.
Attachment #8409098 - Attachment is obsolete: true
Attachment #8412139 - Flags: review?(rnewman)
Comment on attachment 8412139 [details] [diff] [review]
977167.patch

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

::: mobile/android/base/TabsAccessor.java
@@ +49,5 @@
>      private static final String TABS_SELECTION = BrowserContract.Tabs.CLIENT_GUID + " IS NOT NULL";
>  
>      private static final String LOCAL_CLIENT_SELECTION = BrowserContract.Clients.GUID + " IS NULL";
>      private static final String LOCAL_TABS_SELECTION = BrowserContract.Tabs.CLIENT_GUID + " IS NULL";
> +    private static final Pattern FILTERED_URL_PATTERN = Pattern.compile("^(about|chrome|wyciwyg|file):.*$");

I actually meant to leave off the trailing ".*$" -- unless I'm very much mistaken, it's equivalent to omitting that part altogether.
Attachment #8412139 - Flags: review?(rnewman) → review+
Attachment #8412139 - Flags: checkin?
Keywords: checkin-needed
checkin as https://hg.mozilla.org/integration/fx-team/rev/f5712360fc1a

Congrats Vivek and Thanks for contributing to Mozilla!
Keywords: checkin-needed
Whiteboard: [mentor=rnewman][lang=java][good first bug] → [mentor=rnewman][lang=java][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f5712360fc1a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=rnewman][lang=java][good first bug][fixed-in-fx-team] → [mentor=rnewman][lang=java][good first bug]
Target Milestone: --- → Firefox 31
Comment on attachment 8412139 [details] [diff] [review]
977167.patch

In the future, setting both checkin? and checkin-needed just creates extra work for people, so just use checkin-needed :)
Attachment #8412139 - Flags: checkin?
I've had to back this out because the line added to robocop.ini was in the wrong place, which meant that it was intercepting the skip-if line of another test (skip-ifs apply to the test listed above it in the file, not below) - and that test is intermittently failing frequently. I would have just tweaked robocop.ini, but this also means that the test added here has actually never run, so I'm guessing there may be further surprises, so just going to back this out for now. Please can you fix & re-run the test on try :-)

Backout:
remote:   https://hg.mozilla.org/integration/fx-team/rev/6fa6a3e42d65
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Thanks, Ed!

Vivek, I guess this means you get to fix that regex, and make sure the tests run and pass on Try.
Attached patch 977167.patch (obsolete) — Splinter Review
Please find the updated patch attached herewith.

Richard Newman, we need the trailing wildcard in regex. Otherwise the pattern matches only those url where just the scheme is specified.

Try server run logs are available in at
https://tbpl.mozilla.org/?tree=Try&rev=4063116d0e96

I can see from the above logs that the testFilterOpenTab has run in Android 2.2 Opt rc1, Android 2.3 Opt rc2, Android 4.0 Opt rc2 and in Android 4.2 x86 Opt S4.
Further, I've also verified that testDoorHanger and testFindInPage are skipped in Android 2.3 as defined.

Android 2.2 Armv6 Opt rc1 failed with a know bug https://bugzilla.mozilla.org/show_bug.cgi?id=975867
Attachment #8412139 - Attachment is obsolete: true
Attachment #8415781 - Flags: review?(rnewman)
Attached patch 977167.patch (obsolete) — Splinter Review
Updated patch that removes the try chooser commit message present in previous patch.
Attachment #8415781 - Attachment is obsolete: true
Attachment #8415781 - Flags: review?(rnewman)
Attachment #8415785 - Flags: review?(rnewman)
(In reply to vivek from comment #14)

> Richard Newman, we need the trailing wildcard in regex. Otherwise the
> pattern matches only those url where just the scheme is specified.

Only if we use .matches. I'm thinking of .lookingAt. Sorry for being unclear!

http://docs.oracle.com/javase/6/docs/api/java/util/regex/Matcher.html#lookingAt%28%29
(In reply to Richard Newman [:rnewman] from comment #16)
> (In reply to vivek from comment #14)
> 
> > Richard Newman, we need the trailing wildcard in regex. Otherwise the
> > pattern matches only those url where just the scheme is specified.
> 
> Only if we use .matches. I'm thinking of .lookingAt. Sorry for being unclear!
> 
> http://docs.oracle.com/javase/6/docs/api/java/util/regex/Matcher.
> html#lookingAt%28%29

I'll update the patch with lookingAt().

Do you want me to run this is in try server again ?
(In reply to vivek from comment #17)

> Do you want me to run this is in try server again ?

Nope!
Review comment related to lookingAt incorporated.
Attachment #8415785 - Attachment is obsolete: true
Attachment #8415785 - Flags: review?(rnewman)
Attachment #8416078 - Flags: review?(rnewman)
Comment on attachment 8416078 [details] [diff] [review]
977167_latest.patch

Great, thanks for the quick turnaround!

I added the necessary patch description and landed this:

https://hg.mozilla.org/integration/fx-team/rev/922b1b88d4a1
Attachment #8416078 - Flags: review?(rnewman) → review+
Status: REOPENED → ASSIGNED
Target Milestone: Firefox 31 → Firefox 32
Blocks: 977164
https://hg.mozilla.org/mozilla-central/rev/922b1b88d4a1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Mentor: rnewman
Whiteboard: [mentor=rnewman][lang=java][good first bug] → [lang=java][good first bug]
Verified fixed on Nightly 36.0a1 (2014-10-19)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.