Last Comment Bug 758660 - Panorama telemetry gatherer checks global PB state
: Panorama telemetry gatherer checks global PB state
Status: RESOLVED FIXED
[mentor=jdm][lang=js]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 16
Assigned To: Saurabh Anand [:sawrubh]
:
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks: PBnGen
  Show dependency treegraph
 
Reported: 2012-05-25 09:02 PDT by Josh Matthews [:jdm] (on vacation until Dec 5)
Modified: 2016-04-12 14:00 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (1.03 KB, patch)
2012-06-17 10:56 PDT, Saurabh Anand [:sawrubh]
ehsan: feedback-
Details | Diff | Splinter Review
Patch v2 (1.90 KB, patch)
2012-06-17 22:31 PDT, Saurabh Anand [:sawrubh]
josh: feedback-
Details | Diff | Splinter Review
Patch v3 (1.70 KB, patch)
2012-06-27 16:01 PDT, Saurabh Anand [:sawrubh]
ehsan: feedback+
Details | Diff | Splinter Review
Patch v4 (1.60 KB, patch)
2012-06-28 12:29 PDT, Saurabh Anand [:sawrubh]
ttaubert: feedback+
Details | Diff | Splinter Review
Patch v5 (755 bytes, patch)
2012-06-29 03:35 PDT, Saurabh Anand [:sawrubh]
no flags Details | Diff | Splinter Review
Patch v6 (756 bytes, patch)
2012-06-29 03:49 PDT, Saurabh Anand [:sawrubh]
ttaubert: review+
Details | Diff | Splinter Review
Patch v7 (1.24 KB, patch)
2012-06-29 03:59 PDT, Saurabh Anand [:sawrubh]
ttaubert: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (on vacation until Dec 5) 2012-05-25 09:02:07 PDT
We should change this check to use something like

window.getInterface(Ci.nsIWebNavigation)
             .QueryInterface(Ci.nsIDocShellTreeItem)
             .treeOwner
             .QueryInterface(Ci.nsIInterfaceRequestor)
             .getInterface(Ci.nsIXULWindow)
             .docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing;

to determine if the window is private or not.
Comment 1 Saurabh Anand [:sawrubh] 2012-06-17 10:56:33 PDT
Created attachment 633912 [details] [diff] [review]
Patch v1

Why has Josh advised to use the QI goop he says and not             

let temp = window.QueryInterface(Ci.nsIInterfaceRequestor)
                 .getInterface(Ci.nsIWebNavigation)
                 .QueryInterface(Ci.nsIDocShellTreeItem)
                 .rootTreeItem
                 .QueryInterface(Ci.nsIInterfaceRequestor)
                 .getInterface(Ci.nsIDOMWindow)
                 .wrappedJSObject;
and then just accesing |temp.gPrivateBrowsingUI.privateWindow|. Is there any difference in what I propose (although I feel both will give the same result) ?
Comment 2 :Ehsan Akhgari 2012-06-17 11:03:44 PDT
I think Josh's suggestion predates the privateWindow API.  Have you run this through the try server?
Comment 3 :Ehsan Akhgari 2012-06-17 11:05:37 PDT
Comment on attachment 633912 [details] [diff] [review]
Patch v1

Please use the privateWindow API.
Comment 4 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-06-17 13:25:48 PDT
Rereading the code now, I can't imagine that using window works here. I think we need to modify the collection code to avoid private browsing windows and remove the check from the observe method.
Comment 5 Saurabh Anand [:sawrubh] 2012-06-17 21:51:47 PDT
@Ehsan, one silly question :P, is the privateWindow API something special that I don't know about or is it just the process that I have written in my comment ?

@Josh, do you mean that the QI goop that I've said in my comment(which uses window to QI and get chrome window) will not work ?

@Both, for the try run should I do |try: -b do -p all -u all -t none| or all the talos suites also ?
Comment 6 Saurabh Anand [:sawrubh] 2012-06-17 22:31:52 PDT
Created attachment 633953 [details] [diff] [review]
Patch v2

Tried to combine Ehsan's and Josh's comments. Still have a doubt(as Josh said) that using window might not work. So please check if the QI goop that I used is correct or not.
Comment 7 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-06-17 23:35:30 PDT
Comment on attachment 633953 [details] [diff] [review]
Patch v2

This still won't work, since there's no window object to manipulate. http://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/groupitems.js defines the groupItems that is being manipulated in _collect - we should iterate through the result of groupItem.getChildren and obtain window objects from each one, and only then can we do the QI dance. We'll also want to have counter values that hold the totals of non-private windows we encounter.
Comment 8 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-06-17 23:36:36 PDT
With regards to tests, running mochitest-browser-chrome should cover it.
Comment 9 Dão Gottwald [:dao] 2012-06-18 00:39:27 PDT
Use gWindow.gPrivateBrowsingUI.privateWindow.
Comment 10 :Ehsan Akhgari 2012-06-18 12:35:35 PDT
(In reply to Saurabh Anand [:sawrubh] from comment #5)
> @Ehsan, one silly question :P, is the privateWindow API something special
> that I don't know about or is it just the process that I have written in my
> comment ?

What you wrote in your comment.

> @Both, for the try run should I do |try: -b do -p all -u all -t none| or all
> the talos suites also ?

No, you generally don't need to run talos tests, unless you're trying to measure performance.
Comment 11 Saurabh Anand [:sawrubh] 2012-06-18 17:07:19 PDT
@Josh, About the counter that you say is needed , what do you want me to do with it, set up a telemetry attribute for it or just return it ?
Comment 12 Dão Gottwald [:dao] 2012-06-19 01:05:14 PDT
Please disregard comment 7, you don't need a counter. Your patch is on the right track, except that it should use gWindow.gPrivateBrowsingUI.privateWindow.
Comment 13 Saurabh Anand [:sawrubh] 2012-06-27 16:01:17 PDT
Created attachment 637300 [details] [diff] [review]
Patch v3

Is there anything else that needs to be done in this patch ?
Comment 14 :Ehsan Akhgari 2012-06-28 08:44:59 PDT
Comment on attachment 637300 [details] [diff] [review]
Patch v3

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

Looks good with the nit below.  Please ask Tim Taubert to review this, though, since it's been a while since I've looked at the Panorama code.

::: browser/components/tabview/telemetry.js
@@ +30,5 @@
>      let childCounts = [];
> +    // Assuming the default state is Normal mode
> +    let pbFlag = false;
> +    if (gWindow && ("gPrivateBrowsingUI" in gWindow))
> +      pbFlag = gWindow.gPrivateBrowsingUI.privateWindow;

Nit: instead of storing the result in pbFlag, just move this code down and check directly in the if condition.
Comment 15 Saurabh Anand [:sawrubh] 2012-06-28 12:29:39 PDT
Created attachment 637632 [details] [diff] [review]
Patch v4

Fixed the nit pointed by Ehsan.
Comment 16 Saurabh Anand [:sawrubh] 2012-06-28 12:36:44 PDT
https://tbpl.mozilla.org/?tree=Try&rev=a2deb9e92927
Comment 17 Saurabh Anand [:sawrubh] 2012-06-28 15:59:01 PDT
Comment on attachment 637632 [details] [diff] [review]
Patch v4

The tree is green.
Comment 18 Tim Taubert [:ttaubert] 2012-06-29 01:36:55 PDT
Comment on attachment 637632 [details] [diff] [review]
Patch v4

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

Thanks Saurabh, that looks good.

::: browser/components/tabview/telemetry.js
@@ +28,4 @@
>    _collect: function Telemetry_collect() {
>      let stackedGroupsCount = 0;
>      let childCounts = [];
> +    

Nit: please just revert this change.

@@ +46,5 @@
>        let middle = Math.floor(aChildCounts.length / 2);
>        return aChildCounts[middle];
>      }
> +    
> +    if (gWindow && ("gPrivateBrowsingUI" in gWindow) && !gWindow.gPrivateBrowsingUI.privateWindow) {

You don't need to check for 'gWindow', that'll always exist. Please move this check back to its original position - the observe method.
Comment 19 Saurabh Anand [:sawrubh] 2012-06-29 03:35:20 PDT
Created attachment 637830 [details] [diff] [review]
Patch v5

Fixed the nits and comments.
Comment 20 Tim Taubert [:ttaubert] 2012-06-29 03:37:32 PDT
Comment on attachment 637830 [details] [diff] [review]
Patch v5

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

::: browser/components/tabview/telemetry.js
@@ +56,4 @@
>     * Observes for gather telemetry topic.
>     */
>    observe: function Telemetry_observe(aSubject, aTopic, aData) {
> +    if (("gPrivateBrowsingUI" in gWindow) && !gWindow.gPrivateBrowsingUI.privateWindow)

This must be:

> if (!("gPrivateBrowsingUI" in gWindow) || !gWindow.gPrivateBrowsingUI.privateWindow)

because we'll want to collect when there's no private browsing UI.
Comment 21 Saurabh Anand [:sawrubh] 2012-06-29 03:49:37 PDT
Created attachment 637834 [details] [diff] [review]
Patch v6

Sorry for the wrong logic.
Comment 22 Tim Taubert [:ttaubert] 2012-06-29 03:53:41 PDT
Comment on attachment 637834 [details] [diff] [review]
Patch v6

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

Please also remove the 'gPrivateBrowsing' getter defined in browser/components/tabview/tabview.js, it's not used anymore. r=me with that change. Thank you!
Comment 23 Saurabh Anand [:sawrubh] 2012-06-29 03:59:18 PDT
Created attachment 637837 [details] [diff] [review]
Patch v7
Comment 24 Tim Taubert [:ttaubert] 2012-06-29 04:00:29 PDT
Comment on attachment 637837 [details] [diff] [review]
Patch v7

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

That should be r++ now. Can you please push it to try before we land it?
Comment 25 Saurabh Anand [:sawrubh] 2012-06-29 04:11:26 PDT
https://tbpl.mozilla.org/?tree=Try&rev=ed38fbf4c316
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-06-30 12:44:41 PDT
https://hg.mozilla.org/mozilla-central/rev/6ad21d37ab91
Comment 28 Dão Gottwald [:dao] 2012-07-03 04:02:24 PDT
(In reply to Tim Taubert [:ttaubert] from comment #20)
> Comment on attachment 637830 [details] [diff] [review]
> Patch v5
> 
> Review of attachment 637830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/tabview/telemetry.js
> @@ +56,4 @@
> >     * Observes for gather telemetry topic.
> >     */
> >    observe: function Telemetry_observe(aSubject, aTopic, aData) {
> > +    if (("gPrivateBrowsingUI" in gWindow) && !gWindow.gPrivateBrowsingUI.privateWindow)
> 
> This must be:
> 
> > if (!("gPrivateBrowsingUI" in gWindow) || !gWindow.gPrivateBrowsingUI.privateWindow)
> 
> because we'll want to collect when there's no private browsing UI.

gPrivateBrowsingUI must exist here. What's the point of the check?
Comment 29 Tim Taubert [:ttaubert] 2012-07-03 04:29:22 PDT
(In reply to Dão Gottwald [:dao] from comment #28)
> > > if (!("gPrivateBrowsingUI" in gWindow) || !gWindow.gPrivateBrowsingUI.privateWindow)
> > 
> > because we'll want to collect when there's no private browsing UI.
> 
> gPrivateBrowsingUI must exist here. What's the point of the check?

Yeah, in retrospect, that's unnecessary. Panorama is of course active in browser windows only. Doesn't really hurt but can be removed in the future.

Note You need to log in before you can comment on or make changes to this bug.