936195 - UITour: Give webpage a way to query what UI targets are available

Status: VERIFIED FIXED

(Firefox :: General, defect, P3)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Currently, when a tour webpage requests a highlight/info panel for a UI element that has been removed via customization, we just do nothing. This is relatively safe to do.

However, it would be better if the page had a way to query what elements are available, so it can adjust its content to be appropriate.

Comment 1

[Drew Willcoxon :adw]

Attachment: Add UITour.getAvailableTargets

This seems to work.  It's a little tricky mapping widget IDs/names to targets.

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8372717 [details] [diff] [review]
Add UITour.getAvailableTargets

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

::: browser/modules/UITour.jsm
@@ +256,5 @@
>          this.getConfiguration(contentDocument, data.configuration, data.callbackID);
>          break;
>        }
> +
> +      case "getAvailableTargets": {

Drive-by comment: I was thinking we would reuse getConfiguration for this

@@ +836,5 @@
>      }
>      this.sendPageCallback(aContentDocument, aCallbackId, config);
>    },
> +
> +  getAvailableTargets: function(aContentDocument, aCallbackId) {

I think this is complex enough to deserve tests.

Comment 3

[Drew Willcoxon :adw]

Thanks, Matt.  I'd like to get an r+ or at least a feedback+ from Blair (or anybody else I guess who's familiar with this bug) that the patch is doing the right thing before working on tests.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8372717 [details] [diff] [review]
Add UITour.getAvailableTargets

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

Hm, this approach doesn't handle targets that aren't widgets managed by CustomizableUI, such as "quit" and "customize". It also doesn't take into account widgets that aren't available in all windows - such as those not available in private-browsing windows.

Seems easiest/most reliable to just kick off a bunch of parallel getTarget() calls. And either way, we should cache the results per-window (the clear the cache any time we see a customization event).

Also, we should advertise the availability of "pinnedTab", which is a target we dynamically create (so that shouldn't go through getTarget).

::: browser/modules/UITour.jsm
@@ +256,5 @@
>          this.getConfiguration(contentDocument, data.configuration, data.callbackID);
>          break;
>        }
> +
> +      case "getAvailableTargets": {

(In reply to Matthew N. [:MattN] from comment #2)
> Drive-by comment: I was thinking we would reuse getConfiguration for this

What Matt said.

@@ +836,5 @@
>      }
>      this.sendPageCallback(aContentDocument, aCallbackId, config);
>    },
> +
> +  getAvailableTargets: function(aContentDocument, aCallbackId) {

aCallbackID - or someone's going to end up screaming profanities sometime in the future because of us using Id and ID in the same module.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

DOOMED

Comment 6

[Matthew N. [:MattN]]

I don't think we're going to need this for the Australis update tour because most things will be in a default position for Australis. Holly, is that fine that this doesn't make it for 29?

Comment 7

[Drew Willcoxon :adw]

Attachment: add UITour.getConfiguration("availableTargets")

I see, thanks.

(In reply to Blair McBride [:Unfocused] from comment #4)
> @@ +836,5 @@
> >      }
> >      this.sendPageCallback(aContentDocument, aCallbackId, config);
> >    },
> > +
> > +  getAvailableTargets: function(aContentDocument, aCallbackId) {
> 
> aCallbackID - or someone's going to end up screaming profanities sometime in
> the future because of us using Id and ID in the same module.

But this module uses both!  Look a couple of lines above the two added lines I just quoted (and you quoted). :-)  ID seems to be more popular, so I did change it.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8384086 [details] [diff] [review]
add UITour.getConfiguration("availableTargets")

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

Most excellent.

::: browser/modules/UITour.jsm
@@ +935,5 @@
>      });
>      aWindow.gBrowser.selectedTab = tab;
>    },
>  
>    getConfiguration: function(aContentDocument, aConfiguration, aCallbackId) {

*sigh* Don't suppose you'd like to just rename this to aCallbackID while you're at it? :)

::: browser/modules/test/browser_UITour_availableTargets.js
@@ +67,5 @@
> +];
> +
> +function ok_targets(actualData, expectedTargets) {
> +  // Depending on how soon after page load this is called, the selected tab icon
> +  // may or may not be showing the loading throbber.  Check for its presence and

Ugh, we should fix that. File a bug?

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Matthew N. [:MattN] from comment #6)
> I don't think we're going to need this for the Australis update tour because
> most things will be in a default position for Australis. Holly, is that fine
> that this doesn't make it for 29?

I don't consider this a blocker. But saying that, it does allow fixing breakage in the tour, so I would like it uplifted to Aurora. I don't think it comes with much risk in doing so - especially given we can just opt to not use it if it ends up breaking.

Comment 10

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/23b6dc0350c8

(In reply to Blair McBride [:Unfocused] from comment #8)
> >    getConfiguration: function(aContentDocument, aConfiguration, aCallbackId) {
> 
> *sigh* Don't suppose you'd like to just rename this to aCallbackID while
> you're at it? :)

Fixed, and fixed the couple of other instances, involving "aId".

> > +  // Depending on how soon after page load this is called, the selected tab icon
> > +  // may or may not be showing the loading throbber.  Check for its presence and
> 
> Ugh, we should fix that. File a bug?

bug 979103

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23b6dc0350c8

Comment 12

[Drew Willcoxon :adw]

Attachment: Aurora (29) patch

(In reply to Blair McBride [:Unfocused] from comment #9)
> I don't consider this a blocker. But saying that, it does allow fixing
> breakage in the tour, so I would like it uplifted to Aurora. I don't think
> it comes with much risk in doing so - especially given we can just opt to
> not use it if it ends up breaking.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): It's a new feature.
User impact if declined: Parts of Australis UITour content pages may be broken until Firefox 30.
Testing completed (on m-c, etc.): browser-chrome test on m-c and fx-team.
Risk to taking this patch (and alternatives if risky): Small.
String or IDL/UUID changes made by this patch: None.

Comment 13

[Drew Willcoxon :adw]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb3cbffab64b

Comment 14

[Alex Gibson [:agibson]]

Can we please update australis-uitour.js to reflect the change here? Thanks

Comment 15

[Drew Willcoxon :adw]

Alex, who are you asking?  Aren't you the author of https://github.com/mozilla/bedrock/blob/master/media/js/firefox/australis/australis-uitour.js?

Comment 16

[Matthew N. [:MattN]]

No, Unfocused is: https://github.com/Unfocused/mozilla-uitour

I said I would do it but didn't have time on Friday and I'm on PTO today and tomorrow so if someone else wants to that would be good.

Comment 17

[Drew Willcoxon :adw]

Oh.  Alex, if it's OK that your repo doesn't match Blair's, you can just replace your getSyncConfiguration with the getConfiguration from this test: http://mxr.mozilla.org/mozilla-central/source/browser/modules/test/uitour.js#168

Then call it with "availableTargets" as the configName.

Comment 18

[Drew Willcoxon :adw]

And "sync" for the old getSyncConfiguration behavior.

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oy. We need to get better at this.

https://github.com/Unfocused/mozilla-uitour/commit/eb89330e45a3504e94c84c0150ba2bd5ff85ba75

Comment 20

[Alex Gibson [:agibson]]

Thanks, Blair

One question about this API, does it only return a list of available targets, or can it also tell me where in the chrome a target is located? I seem to remember a discussion that the latter would be useful, but I wasn't sure if this bug covers that.

Comment 21

[Matthew N. [:MattN]]

(In reply to Alex Gibson [:agibson] from comment #20)
> One question about this API, does it only return a list of available
> targets, or can it also tell me where in the chrome a target is located? I
> seem to remember a discussion that the latter would be useful, but I wasn't
> sure if this bug covers that.

It's just an array of targets that aren't in the palette. I filed bug 988151 to get more visibility/location information returned.

Comment 22

[Catalin Varga [QA][:VarCat]]

Is there a way to verify this manually?

I removed Add-ons and Bookmarks  button from Menu Panel and Toolbar but from an user point of view nothing was changed postfix. The pages from the Ui Tour related to Add-ons and Bookmarks are still shown whether the buttons are visible or not.

Builds used for testing:
Prefix: 
Aurora 29
Build Id: 20140303004002

Postfix:
Beta 29 RC candidate
Build Id:20140421221237

Comment 23

[Alex Gibson [:agibson]]

(In reply to Catalin Varga [QA][:VarCat] from comment #22)
> Is there a way to verify this manually?
> 
> I removed Add-ons and Bookmarks  button from Menu Panel and Toolbar but from
> an user point of view nothing was changed postfix. The pages from the Ui
> Tour related to Add-ons and Bookmarks are still shown whether the buttons
> are visible or not.

Note: this API is currently not used in the tour itself, so it will need testing separately.

Comment 24

[Matthew N. [:MattN]]

No human verification is needed at this time.