393881 - update metrics extension to use places

Status: RESOLVED FIXED

(Toolkit Graveyard :: Data Collection/Metrics, defect)

Description

[Alex Polvi]

Attachment: updates extension to current api

Here is the patch I've come up with to get the metrics extension to work with the most up to date version of places.

The first patch does what *I think* is needed to make the extension work with the current places api. The second one nukes all the ifdef MOZ_PLACES_BOOKMARKS stuff and assumes that is what is happening. I've also updated the install.rdf to the extension wont be installed on pre-places clients.

Review is definitely needed.

Comment 1

[Alex Polvi]

Attachment: clears out MOZ_PLACES_BOOKMARKS ifdefs

Comment 2

[Pete Collins (MDG)]

taking bug, I have a patch that compiles on the trunk as the existing patch was calling on some interfaces that are now gone ...

Comment 3

[Pete Collins (MDG)]

Attachment: Complete single patch - compile w/ trunk

Comment 4

[Pete Collins (MDG)]

cc'ing Jan. Jan can you peer review this patch for me?

Comment 5

[Jan Varga [:janv]]

sure, I'll review it

Comment 6

[Jan Varga [:janv]]

Comment on attachment 288354 [details] [diff] [review]
Complete single patch - compile w/ trunk

>-      PRInt64 folderID = 0;
>-      folder->GetFolderId(&folderID);
>+      PRInt64 folderID;
>+      child->GetItemId(&folderID);  

no need to init it to 0 anymore?

>@@ -473,94 +465,7 @@
> nsUICommandCollector::LogBookmarkInfo(const nsString& id,
>                                       nsIMetricsEventItem* parentItem)
> {
>-#ifdef MOZ_PLACES_BOOKMARKS
>   // TODO: write me!
>   // see bug #356606
>   return NS_OK;
>-
>-#else
>-

no implementation, does it work w/o out ?

Comment 7

[Pete Collins (MDG)]

> >-      PRInt64 folderID = 0;
> >+      PRInt64 folderID;

> no need to init it to 0 anymore?

I agree it should be initialized.

> >-#ifdef MOZ_PLACES_BOOKMARKS
> >   // TODO: write me!
> >   // see bug #356606
> >   return NS_OK;

> no implementation, does it work w/o out ?

Yes, from my testing things appear to be working so far. 

I'm not sure writing this code is a prerequisite to landing a working patch on the trunk ...

Alex? 

Comment 8

[Alex Polvi]

(In reply to comment #7)
> > >-      PRInt64 folderID = 0;
> > >+      PRInt64 folderID;
> 
> > no need to init it to 0 anymore?
> 
> I agree it should be initialized.
> 
> > >-#ifdef MOZ_PLACES_BOOKMARKS
> > >   // TODO: write me!
> > >   // see bug #356606
> > >   return NS_OK;
> 
> > no implementation, does it work w/o out ?
> 
> Yes, from my testing things appear to be working so far. 
> 
> I'm not sure writing this code is a prerequisite to landing a working patch on
> the trunk ...

We do not need the functionality for now. As long as everything else is working
technically with this in place, it is OK from a product point of view for it to
just return. 

Comment 9

[Jan Varga [:janv]]

I tested the patch and it looks it works fine. I just added
|user_pref("metrics.upload.enable", true);| to pref.js and created metrics-config.xml

<?xml version="1.0"?>                                                           
                                                                                
<response xmlns="http://www.mozilla.org/metrics">                               
  <config>                                                                      
    <collectors>                                                                
<!--      <collector type="document"/> -->                                      
<!--      <collector type="window"/> -->                                        
<!--      <collector type="profile"/> -->                                       
      <collector type="uielement"/>                                             
      <collector type="autocomplete"/>                                          
    </collectors>                                                               
    <limit events="200"/>                                                       
    <upload interval="600"/>                                                    
  </config>                                                                     
</response>

Once I quit the application, metrics.xml is correctly created with all the output.

Comment 10

[Jan Varga [:janv]]

Alex, who would be a good sr for this ?

Comment 11

[Reed Loden [:reed]]

(In reply to comment #10)
> Alex, who would be a good sr for this ?

SR isn't required, as per shaver. I'll land this sometime today.

Comment 12

[Reed Loden [:reed]]

Oh, it seems Pete has commit access. Pete, do you want to land this yourself? If not, re-add the checkin-needed keyword.

Comment 13

[Pete Collins (MDG)]

Yes, I can land this today.

Comment 14

[Pete Collins (MDG)]

Ok, this patch has landed. 

Marking FIXED.

We can open new bugs for specific issues we find w/ the metrics implementation.