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.

I would like to work on this bug. Can u help me on it?

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.

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:.*)$");

Open tabs from local client filtered in TabsAccessor prior to upload. New content provider test added to verify it.

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.

Review comments incorporated.

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.

checkin as https://hg.mozilla.org/integration/fx-team/rev/f5712360fc1a Congrats Vivek and Thanks for contributing to Mozilla!

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

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

Thanks, Ed! Vivek, I guess this means you get to fix that regex, and make sure the tests run and pass on Try.

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

Updated patch that removes the try chooser commit message present in previous patch.

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

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

Verified fixed on Nightly 36.0a1 (2014-10-19)