758660 - Panorama telemetry gatherer checks global PB state

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Josh Matthews [:jdm]]

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]]

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

[(no longer active)]

I think Josh's suggestion predates the privateWindow API.  Have you run this through the try server?

Comment 3

[(no longer active)]

Comment on attachment 633912 [details] [diff] [review]
Patch v1

Please use the privateWindow API.

Comment 4

[Josh Matthews [:jdm]]

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]]

@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]]

Attachment: 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]]

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]]

With regards to tests, running mochitest-browser-chrome should cover it.

Comment 9

[Dão Gottwald [:dao]]

Use gWindow.gPrivateBrowsingUI.privateWindow.

Comment 10

[(no longer active)]

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

@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]]

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]]

Attachment: Patch v3

Is there anything else that needs to be done in this patch ?

Comment 14

[(no longer active)]

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]]

Attachment: Patch v4

Fixed the nit pointed by Ehsan.

Comment 16

[Saurabh Anand [:sawrubh]]

https://tbpl.mozilla.org/?tree=Try&rev=a2deb9e92927

Comment 17

[Saurabh Anand [:sawrubh]]

Comment on attachment 637632 [details] [diff] [review]
Patch v4

The tree is green.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: Patch v5

Fixed the nits and comments.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: Patch v6

Sorry for the wrong logic.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: Patch v7

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

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]]

https://tbpl.mozilla.org/?tree=Try&rev=ed38fbf4c316

Comment 26

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad21d37ab91

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6ad21d37ab91

Comment 28

[Dão Gottwald [:dao]]

(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] (inactive)]

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