Closed Bug 1347530 Opened 7 years ago Closed 7 years ago

Use insertTree API to import bookmarks from 360se (and/or remove the importer)

Categories

(Firefox :: Migration, enhancement, P3)

Product:
Firefox
Component:
Migration
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: Gijs, Assigned: hectorz)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(2 files)

Bug 1347530 - Part 2: Use insertTree API to import bookmarks from 360se.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1347530 - Part 1: Add a unit test for 360se bookmark imports.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
+++ This bug was initially created as a clone of Bug #1344759 +++

In bug 1344282 I created an insertTree Bookmarks.jsm API that inserts multiple bookmarks in one go. Initial evidence suggests this dramatically speeds up bookmarks import and reduces jank/hangs while the import is ongoing. We should use it from the various importers.

bug 1344759 started using it for Chrome, IE, Edge and Safari imports. The only other importer where we are inserting bookmarks is the 360se one. However, there are no automated tests for this import, and it sees very little usage, so I didn't touch it in bug 1344759.

In this bug we should either update the migrator or remove it.
Priority: -- → P3
(In reply to :Gijs from comment #0)
> 
> In this bug we should either update the migrator or remove it.

I'm not sure when I'm able to work on this, but please don't just remove it, it's the most used one here in China. Thanks.
(In reply to Hector Zhao [:hectorz] from comment #1)
> (In reply to :Gijs from comment #0)
> > 
> > In this bug we should either update the migrator or remove it.
> 
> I'm not sure when I'm able to work on this, but please don't just remove it,
> it's the most used one here in China. Thanks.

Does Firefox in China not report Telemetry? Because the data we have suggests otherwise.
Flags: needinfo?(bzhao)
(In reply to :Gijs from comment #2)
> (In reply to Hector Zhao [:hectorz] from comment #1)
> > (In reply to :Gijs from comment #0)
> > > 
> > > In this bug we should either update the migrator or remove it.
> > 
> > I'm not sure when I'm able to work on this, but please don't just remove it,
> > it's the most used one here in China. Thanks.
> 
> Does Firefox in China not report Telemetry? Because the data we have
> suggests otherwise.

I'm looking at https://sql.telemetry.mozilla.org/queries/305#505 , and I think 9 is 360se? Also you'll need to consider that unlike other global brand, most 360se users would be from China.
Flags: needinfo?(bzhao)
(In reply to Hector Zhao [:hectorz] from comment #3)
> (In reply to :Gijs from comment #2)
> > (In reply to Hector Zhao [:hectorz] from comment #1)
> > > (In reply to :Gijs from comment #0)
> > > > 
> > > > In this bug we should either update the migrator or remove it.
> > > 
> > > I'm not sure when I'm able to work on this, but please don't just remove it,
> > > it's the most used one here in China. Thanks.
> > 
> > Does Firefox in China not report Telemetry? Because the data we have
> > suggests otherwise.
> 
> I'm looking at https://sql.telemetry.mozilla.org/queries/305#505 , and I
> think 9 is 360se? Also you'll need to consider that unlike other global
> brand, most 360se users would be from China.

The telemetry dist viewer is currently broken for non-nightly (on Nightly, it represents 0.04% of the incoming pings on that histogram), but the distributions I've seen in the past are more pessimistic than that (ie approximately no beta users at all for startup migration, and no funnelcake (up to 10% of release population) users either). Perhaps historical data is skewing that particular sql query, or people use it after installing Firefox but not during initial migration, or that particular SQL query is just broken (it doesn't seem to filter out by update channel or validate any other data about the ping submissions).

In any case, there are no automated tests for this migrator at all, and installing the browser and doing manual testing is onerous as (a) there are no localized builds available and the engineers who regularly work on this code do not speak Chinese, and (b) the installer/uninstaller is basically malware that tries very hard to get users to install as much crapware as possible and then the uninstaller tries to avoid users actually uninstalling the browser and/or said crapware. As a result, the cost/benefit ratio here does not seem good to me.
(In reply to :Gijs from comment #4)
> 
> The telemetry dist viewer is currently broken for non-nightly (on Nightly,
> it represents 0.04% of the incoming pings on that histogram),

I assume by "telemetry dist viewer", you mean https://telemetry.mozilla.org/ ? I remember reading somewhere it's calculated with the opt-in population.

> but the
> distributions I've seen in the past are more pessimistic than that (ie
> approximately no beta users at all for startup migration,

I'm not sure about this, but my guess would be lower than average percentage of Chinese users are on beta channel.

> and no funnelcake
> (up to 10% of release population) users either).

Most users in China don't get their Firefox from mozilla.org. For those who do, I believe most funnelcakes are targeting en-US users only.

> Perhaps historical data is
> skewing that particular sql query, or people use it after installing Firefox
> but not during initial migration, or that particular SQL query is just
> broken (it doesn't seem to filter out by update channel or validate any
> other data about the ping submissions).

:MattN, since you created the query, maybe you can comment on whether it's still accurate? Thanks.

> 
> In any case, there are no automated tests for this migrator at all,

That's definitely my fault.

> and 
> installing the browser and doing manual testing is onerous as (a) there are
> no localized builds available and the engineers who regularly work on this
> code do not speak Chinese,

As I've said in bug 1276694 comment 4, we at Beijing office can help with the manual testing here. Our desktop QA Yanfang have been testing it before every release and at least one bug (bug 1310596) was discovered in the process.

> and (b) the installer/uninstaller is basically
> malware that tries very hard to get users to install as much crapware as
> possible and then the uninstaller tries to avoid users actually uninstalling
> the browser and/or said crapware. As a result, the cost/benefit ratio here
> does not seem good to me.

Unfortunately, it is also the most popular used desktop browser here in China. Removing the ability to import their bookmark doesn't really help any user who shows an interest in Firefox.
Flags: needinfo?(MattN+bmo)
(In reply to Hector Zhao [:hectorz] from comment #5)
> (In reply to :Gijs from comment #4)
> > 
> > The telemetry dist viewer is currently broken for non-nightly (on Nightly,
> > it represents 0.04% of the incoming pings on that histogram),
> 
> I assume by "telemetry dist viewer", you mean https://telemetry.mozilla.org/
> ? I remember reading somewhere it's calculated with the opt-in population.

So is SQL.tmo, so that doesn't explain the difference.

> > but the
> > distributions I've seen in the past are more pessimistic than that (ie
> > approximately no beta users at all for startup migration,
> 
> I'm not sure about this, but my guess would be lower than average percentage
> of Chinese users are on beta channel.
> 
> > and no funnelcake
> > (up to 10% of release population) users either).
> 
> Most users in China don't get their Firefox from mozilla.org. For those who
> do, I believe most funnelcakes are targeting en-US users only.

Ah, the en-US point is a good one. That explains the funnelcake, at least.

> > Perhaps historical data is
> > skewing that particular sql query, or people use it after installing Firefox
> > but not during initial migration, or that particular SQL query is just
> > broken (it doesn't seem to filter out by update channel or validate any
> > other data about the ping submissions).
> 
> :MattN, since you created the query, maybe you can comment on whether it's
> still accurate? Thanks.

If people in China don't use mozilla.org, maybe that explains the difference, though the low number of beta users is still somewhat surprising to me.
(In reply to :Gijs from comment #6)
> > 
> > I assume by "telemetry dist viewer", you mean https://telemetry.mozilla.org/
> > ? I remember reading somewhere it's calculated with the opt-in population.
> 
> So is SQL.tmo, so that doesn't explain the difference.

About this, please have a look at bug 1270225 again.
(In reply to :Gijs from comment #6)
> 
> If people in China don't use mozilla.org, maybe that explains the
> difference, though the low number of beta users is still somewhat surprising
> to me.

I don't have more recent stats at hand, but in March about 0.25% of users in China are using beta channel. I'm not sure how this compares with the global average.
(In reply to Hector Zhao [:hectorz] from comment #5)
> (In reply to :Gijs from comment #4)
> > Perhaps historical data is
> > skewing that particular sql query, or people use it after installing Firefox
> > but not during initial migration, or that particular SQL query is just
> > broken (it doesn't seem to filter out by update channel or validate any
> > other data about the ping submissions).
> 
> :MattN, since you created the query, maybe you can comment on whether it's
> still accurate? Thanks.

https://sql.telemetry.mozilla.org/queries/305/source#505

My understanding:
* The probe is question is fx_migration_source_browser which is set to opt-out in Histograms.json
* I'm using the longitudinal dataset which is a 1% sample of release users with pings from the last 6 months (I believe non-release were removed near the beginning of the year)
* Since it's only counting where fx_migration_source_browser isn't null I don't think much more filtering is needed unless someone is intentionally submitting garbage for that histogram.
* The data from other popular browsers seems plausible

From my understanding I think the query is fine.

Questions:
* Are Mozilla Online default telemetry settings the same as MoCo builds?
* Do Mozilla Online builds submit telemetry to the same MoCo telemetry server?
* If so, do pings successfully reach the server or are they blocked by the great firewall?

I believe that at ping ingestion time the server does a GeoIP lookup on the client IP address and stores the country. One could write a Spark Analysis job to look at the number of pings from China to help answer some of the above.
Flags: needinfo?(MattN+bmo)
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #9)

Thanks.

> 
> Questions:
> * Are Mozilla Online default telemetry settings the same as MoCo builds?

Yes. Lower percentage of users in China opt in than global average, though.[1]

> * Do Mozilla Online builds submit telemetry to the same MoCo telemetry
> server?

Yes.

> * If so, do pings successfully reach the server or are they blocked by the
> great firewall?

Comparing the client count from telemetry with the ADI tracked by our own extension, I'd say yes, most of them.

> 
> I believe that at ping ingestion time the server does a GeoIP lookup on the
> client IP address and stores the country. One could write a Spark Analysis
> job to look at the number of pings from China to help answer some of the
> above.

From the client_count dataset, ~3% of users are in China.[2]

[1]: https://sql.telemetry.mozilla.org/queries/2056#table
[2]: https://sql.telemetry.mozilla.org/queries/3234#6241
Comment on attachment 8874732 [details]
Bug 1347530 - Part 2: Use insertTree API to import bookmarks from 360se.

https://reviewboard.mozilla.org/r/146108/#review150202

This really needs a test as part of this fix.
Attachment #8874732 - Flags: review?(gijskruitbosch+bugs) → review-
(In reply to :Gijs from comment #12)
> 
> This really needs a test as part of this fix.

Added, based on the Safari one.
Comment on attachment 8875114 [details]
Bug 1347530 - Part 1: Add a unit test for 360se bookmark imports.

https://reviewboard.mozilla.org/r/146498/#review150700

::: commit-message-5801a:1
(Diff revision 1)
> +Bug 1347530 - Part 1: Add a unit test for 360se bookmark imports. r?Gijs

This adds an empty file (on which I can't comment directly). Please remove that file.

::: browser/components/migration/tests/unit/AppData/Roaming/360se6/apps/data/users/login.ini:7
(Diff revision 1)
> +# Will be excluded from sourceProfiles due to missing files
> +[20070606]
> +NickName=ıÖÇÍøÂç
> +email=test@mozillaonline.com
> +UserMd5=46a579b8b64358fd45616247df4ea604
> +# Will be excluded from sourceProfiles as duplication of NowLogin

These comments suggest there should only be 1 profile, with both others being excluded, but the check below asserts that profiles.length == 2. What is actually happening?
Attachment #8875114 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8874732 [details]
Bug 1347530 - Part 2: Use insertTree API to import bookmarks from 360se.

https://reviewboard.mozilla.org/r/146108/#review150702

::: browser/components/migration/360seProfileMigrator.js:145
(Diff revision 2)
> -            }
> -            idToGuid.set("fallback", parentGuid);
> -          }
>  
> +          if (is_folder) {
> +            let folder = { _childrenRef: [] };

Instead of creating a separate object, please just add bmToInsert into the map at the same key, and manipulate its .children property directly.

::: browser/components/migration/360seProfileMigrator.js:179
(Diff revision 2)
> +      if (toolbarBMs.length) {
> +        let parentGuid = PlacesUtils.bookmarks.toolbarGuid;
> +        if (!MigrationUtils.isStartupMigration) {
> +          parentGuid =
> +            await MigrationUtils.createImportedBookmarksFolder("360se", parentGuid);
> +        }
> +        await MigrationUtils.insertManyBookmarksWrapper(toolbarBMs, parentGuid);
> +      }
> +      if (unfiledBMs.length) {
> +        let parentGuid = PlacesUtils.bookmarks.unfiledGuid;
> +        if (!MigrationUtils.isStartupMigration) {
> +          parentGuid =
> +            await MigrationUtils.createImportedBookmarksFolder("360se", parentGuid);
> +        }
> +        await MigrationUtils.insertManyBookmarksWrapper(unfiledBMs, parentGuid);
> +      }

This code is the same, so you could simplify it by doing:

```js
let {toolbarGuid, unfiledGuid} = PlacesUtils.bookmarks;
let data = [[toolbarGuid, toolbarBMs], [unfiledGuid, unfiledBMs]];
for (let [parentGuid, bookmarks] of data) {
  if (bookmarks.length) {
    if (!MigrationUtils.isStartupMigration) {
      parentGuid = 
        await MigrationUtils.createImportedBookmarksFolder("360se", parentGuid);
    }
    await MigrationUtils.insertManyBookmarksWrapper(bookmarks, parentGuid);
  }
}
```
Attachment #8874732 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #16)
> 
> This adds an empty file (on which I can't comment directly). Please remove
> that file.

The empty file exists to satisfy the check at [1], maybe I should add a comment as its content.

> 
> These comments suggest there should only be 1 profile, with both others
> being excluded, but the check below asserts that profiles.length == 2. What
> is actually happening?

There's a special default profile which is not listed in login.ini. [2]

(In reply to :Gijs from comment #17)
> 
> Instead of creating a separate object, please just add bmToInsert into the
> map at the same key, and manipulate its .children property directly.

I'll update this tomorrow.

> 
> This code is the same, so you could simplify it by doing:
> 
> ```js
> let {toolbarGuid, unfiledGuid} = PlacesUtils.bookmarks;
> let data = [[toolbarGuid, toolbarBMs], [unfiledGuid, unfiledBMs]];
> for (let [parentGuid, bookmarks] of data) {
>   if (bookmarks.length) {
>     if (!MigrationUtils.isStartupMigration) {
>       parentGuid = 
>         await MigrationUtils.createImportedBookmarksFolder("360se",
> parentGuid);
>     }
>     await MigrationUtils.insertManyBookmarksWrapper(bookmarks, parentGuid);
>   }
> }
> ```

Reading this again, I just realized with the SQL query at [3], any bookmark with a nonexistent parent_id won't be returned. Maybe I should just drop the unfiled folder related part.

[1]: https://dxr.mozilla.org/mozilla-central/rev/5801aa478de1/browser/components/migration/360seProfileMigrator.js#108
[2]: https://dxr.mozilla.org/mozilla-central/rev/5801aa478de1/browser/components/migration/360seProfileMigrator.js#274
[3]: https://dxr.mozilla.org/mozilla-central/rev/5801aa478de1/browser/components/migration/360seProfileMigrator.js#126
(In reply to Hector Zhao [:hectorz] from comment #18)
> 
> Reading this again, I just realized with the SQL query at [3], any bookmark
> with a nonexistent parent_id won't be returned. Maybe I should just drop the
> unfiled folder related part.

This sounds like a good idea, also because the code as-is could otherwise insert folders into unfiled bookmarks. While this is technically supported, from a UI perspective it is suboptimal.
Assignee: nobody → bzhao
Status: NEW → ASSIGNED
(In reply to Hector Zhao [:hectorz] from comment #20)
> Comment on attachment 8875114 [details]
> Bug 1347530 - Part 1: Add a unit test for 360se bookmark imports.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/146498/diff/1-2/

(In reply to Hector Zhao [:hectorz] from comment #18)
> (In reply to :Gijs from comment #16)
> > 
> > This adds an empty file (on which I can't comment directly). Please remove
> > that file.
> 
> The empty file exists to satisfy the check at [1], maybe I should add a
> comment as its content.

The comment doesn't show up for me in the interdiff, please see https://reviewboard-hg.mozilla.org/gecko/diff/9d1655c741c0/browser/components/migration/tests/unit/AppData/Roaming/360se6/apps/data/users/0f3ab103a522f4463ecacc36d34eb996/360sefav.db
Hi Hector, seems there are open issues in mozreview that need to be fixed before we land this. Thanks!
Flags: needinfo?(bzhao)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #24)
> Hi Hector, seems there are open issues in mozreview that need to be fixed
> before we land this. Thanks!

Oops, I've pushed the updates but forgot to mark them as fixed in mozreview.
Flags: needinfo?(bzhao)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/300d2598ec40
Part 1: Add a unit test for 360se bookmark imports. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/99fe8edb8296
Part 2: Use insertTree API to import bookmarks from 360se. r=Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/300d2598ec40
https://hg.mozilla.org/mozilla-central/rev/99fe8edb8296
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
No longer depends on: 1658418
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: