Closed Bug 1357046 Opened 8 years ago Closed 7 years ago

Should add the Private Browsing tour in the onBoarding overlay

Categories

(Firefox :: General, enhancement, P1)

Product:
Firefox
Component:
General
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: Fischer, Assigned: rexboy)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(1 file)

Bug 1357046 - Add private browsing and search tour to onboarding overlay.
(deleted), text/x-review-board-request
mossop
: review+
Fischer
: review+
gasolin
: review+
flod
: review+
Details 
Should add the Private Browsing tour in the onBoarding overlay
Whiteboard: [photon-onboarding]
Target Milestone: --- → Firefox 55
Assignee: nobody → rexboy
Status: NEW → ASSIGNED
Flags: qe-verify+
QA Contact: jwilliams
Priority: -- → P1
Target Milestone: Firefox 55 → Firefox 56
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

This patch tries to put on private browsing tour and search bar tour together on the overlay.
Although this bug is for just one tour, I put two tours together at the same time so we can test the mechanism of switching tour pages since we didn't have this function before.

To Mossop: I got some notice and discussions below, may you give me some advices?

1. The UITour-lib.js included has several features of UI tours that we need. It  was designated for using at an outside webpage, but now we included it internally by putting it into package and loading it to the content window.

2. For now all the strings are hard-coded, but I just got an update that we're now targeting for a release directly, so we need a framework for l10n now. Is there a suitable framework to use? Seems the XUL/DTD mechanism doesn't work for our current patch because we tries to attach elements after the page is fully loaded.

3.I thought in framescript we can manipulate preferences no matter what processes we're in, but once I force about:newtab to be opened in child process (by setting it to DEFAULT_REMOTE_TYPE in E10SUtils.jsm), I can only read the preferences rather than write. Is this by design?

To Fred and Fischer:
This patch is a little bit different than our very first version, please take a look.

Thank you guys!
Attachment #8873368 - Flags: feedback?(gasolin)
Attachment #8873368 - Flags: feedback?(fliu)
Attachment #8873368 - Flags: feedback?(dtownsend)
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review148812

::: browser/extensions/onboarding/content/onboardingTours.js:14
(Diff revision 1)
> + * {
> + *   // Tour Id, should be unique in the tours
> + *   id: "addons",
> + *   // The tour title which would be displayed on the navigation bar
> + *   title: "tour title",
> + *   // The icon which would be displayed on the navigation bar

the icon seems now defined in CSS, so we can remove that sentence

::: browser/extensions/onboarding/content/onboardingTours.js:21
(Diff revision 1)
> + *   // There should be 3 sections in the div,
> + *   // marked with 3 css selectors respectively.
> + *   // They are .onboarding-tour-description, .onboarding-tour-content, .onboarding-tour-button.
> + *   // If no button, add no-button css class in the div.
> + *   // The overlay layout will responsively position and distribute space for these 3 sections based on viewport size
> + *   page() {},

according to the following content, this might be `getPage()`

::: browser/extensions/onboarding/content/onboardingTours.js:75
(Diff revision 1)
> +      let div = win.document.createElement("div");
> +      div.innerHTML = `
> +        <section class="onboarding-tour-description">
> +          <h1>Browser by yourself.</h1>
> +          <p>
> +            There's no reason to share your online life with trackers every time you browse. Wnat to keep something to yourself? Use Private Browsing with Tracking Protection.

typo, `Wnat to keep something` -> `Want to keep something`
(In reply to KM Lee [:rexboy] from comment #3)
> 1. The UITour-lib.js included has several features of UI tours that we need.
> It  was designated for using at an outside webpage, but now we included it
> internally by putting it into package and loading it to the content window.

I'm not sure what you're asking here but it looks pretty safe to use anywhere.

> 2. For now all the strings are hard-coded, but I just got an update that
> we're now targeting for a release directly, so we need a framework for l10n
> now. Is there a suitable framework to use? Seems the XUL/DTD mechanism
> doesn't work for our current patch because we tries to attach elements after
> the page is fully loaded.

If you really need to do this programmatically then use string bundles.

> 3.I thought in framescript we can manipulate preferences no matter what
> processes we're in, but once I force about:newtab to be opened in child
> process (by setting it to DEFAULT_REMOTE_TYPE in E10SUtils.jsm), I can only
> read the preferences rather than write. Is this by design?

Yes, content processes cannot write prefs directly. They have to send a message to the main process in order to do that.
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review148964

Rather than programmatically inserting all these elements into the about:newtab page have we considered just inserting an iframe that loads an xhtml page provided by the add-on? This would allow us to use the standard method of localization with dtd files and would vastly reduce the risk of onboarding code conflicting code running in the host page.

::: browser/extensions/onboarding/content/onboarding.js:38
(Diff revision 1)
>      this._overlayIcon = this._renderOverlayIcon();
>      this._overlay = this._renderOverlay();
>      this._window.document.body.appendChild(this._overlayIcon);
>      this._window.document.body.appendChild(this._overlay);
>  
> +    Services.scriptloader.loadSubScript(UITOUR_LIB_JS_URL, this._window);

We don't need to load this more than once.

::: browser/extensions/onboarding/content/onboarding.js:39
(Diff revision 1)
>      this._overlay = this._renderOverlay();
>      this._window.document.body.appendChild(this._overlayIcon);
>      this._window.document.body.appendChild(this._overlay);
>  
> +    Services.scriptloader.loadSubScript(UITOUR_LIB_JS_URL, this._window);
> +    Services.scriptloader.loadSubScript(ONBOARDING_TOURS_JS_URL);

Why does this need to be external to this file?
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review149042

::: browser/extensions/onboarding/content/onboarding.css:197
(Diff revision 1)
> +}
> +
> +/* Tour Icons */
> +#search {
> +  background-image: url("img/icons_search.svg");
> +}

Is it able to set the tour's icon src in the reusable class selector, such as
```
.search-tour-icon {
  background-image: url("img/icons_search.svg");
}
```

Because the notification bar for the search tour is using the same icon, it would be good that the notification bar could share the same selector.

::: browser/extensions/onboarding/content/onboarding.js:74
(Diff revision 1)
>  
>    toggleOverlay() {
>      this._overlay.classList.toggle("opened");
>    }
>  
> +  gotoPage(tourId) {

Thanks for `gotoPage` function so for notification bar we could call `onboarding.toggle()` then `onboarding.gotToPage(tourId)` to open corresponding tour for that tour notification.

::: browser/extensions/onboarding/content/onboarding.js:128
(Diff revision 1)
> +      // Dynamically create tour pages
> +      let div = tour.getPage(this._window);
> +      div.id = `${tour.id}-tour-page`;
> +      div.classList.add("onboarding-tour-page");
> +      div.style.display = "none";
> +      pagesFrag.appendChild(div);

Would you consider lazy creation of tour page? That is doing this works in `gotoPage` such as 
```
// Inside `gotoPage`.
// this._tourPages is a Map object.
let targetPage = this._tourPages.get(tourId);
if (!targetPage) {
  // Let's create one
}
let targetPageId = `${tourId}-tour-page`;
// Continue the rest of works
```

Consider we will be injected into the Activity-Stream and quite a lof of "activities" heppened inside the Activity-Stream. Lazy creation would help some.

However, currently we only have 6 static tours, maybe this wouldn't impact too much so we could do this improvement in the future if things get more complex to save simplicity for now.
It's your design decision.
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review149134

::: browser/extensions/onboarding/content/onboarding.css:195
(Diff revision 1)
> +.onboarding-tour-button > button:active {
> +  background: #0881dd;
> +}
> +
> +/* Tour Icons */
> +#search {

To mitigate the css conflict with main page, it will be a nice convention to always add a prefix before the class name. ex: `#search` -> `#overlay-tour-search`. We can add that rule at the top of the css file, or enforce so in goToPage function.

Name the style with class instead of id might help as well, then we can use same class for both overlay and notification.
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review149144

::: browser/extensions/onboarding/content/onboarding.css:200
(Diff revision 1)
> +#search {
> +  background-image: url("img/icons_search.svg");
> +}
> +
> +#search.active,
> +#search:hover {

the commit message should include `search` tour to reflect this patch's content

ex:　"Add private browsing and search tour to onboarding overlay"
(In reply to Fred Lin [:gasolin] from comment #8)
> Comment on attachment 8873368 [details]
> Bug 1357046 - Add private browsing tour to onboarding overlay
> 
> https://reviewboard.mozilla.org/r/144810/#review149134
> 
> ::: browser/extensions/onboarding/content/onboarding.css:195
> (Diff revision 1)
> > +.onboarding-tour-button > button:active {
> > +  background: #0881dd;
> > +}
> > +
> > +/* Tour Icons */
> > +#search {
> 
> To mitigate the css conflict with main page, it will be a nice convention to
> always add a prefix before the class name. ex: `#search` ->
> `#overlay-tour-search`. We can add that rule at the top of the css file, or
> enforce so in goToPage function.
> 
> Name the style with class instead of id might help as well, then we can use
> same class for both overlay and notification.
OK, that is a point. Right now the code uses `#onboarding-overlay.opened` to open the overlay. Consider `.opened` should be a common selector. If there was a CSS rule elsewhere, such as
```
.opened {
  border: 10px solid pink; /* Let's highlight with pink boarder!!! */
}
```
Then our overlay could have unexpected pink border.
We should switch to like `.onboarding-opened`. Prefix every CSS selector inside our onboarding component with, say, "onboarding".
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review149138

::: browser/extensions/onboarding/content/onboarding.css:197
(Diff revision 1)
> +}
> +
> +/* Tour Icons */
> +#search {
> +  background-image: url("img/icons_search.svg");
> +}

Just discussed it face to face and we'll just put the selector from notification together with this rule.

::: browser/extensions/onboarding/content/onboarding.js:38
(Diff revision 1)
>      this._overlayIcon = this._renderOverlayIcon();
>      this._overlay = this._renderOverlay();
>      this._window.document.body.appendChild(this._overlayIcon);
>      this._window.document.body.appendChild(this._overlay);
>  
> +    Services.scriptloader.loadSubScript(UITOUR_LIB_JS_URL, this._window);

Not very sure if I understand. Isn't it loaded just once in each content window now?

::: browser/extensions/onboarding/content/onboarding.js:39
(Diff revision 1)
>      this._overlay = this._renderOverlay();
>      this._window.document.body.appendChild(this._overlayIcon);
>      this._window.document.body.appendChild(this._overlay);
>  
> +    Services.scriptloader.loadSubScript(UITOUR_LIB_JS_URL, this._window);
> +    Services.scriptloader.loadSubScript(ONBOARDING_TOURS_JS_URL);

Just thought that may be more readable since it would finally contain 6 pages. I have no strong prefer here. Putting it inside onboarding.js works for me too.
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review148964

Fischer has discussed that with Matt before; We didn't do that because it still need to interact with the notification snippet which pops up in certain condition, and when user clicks them, certain page in the overlay need to be opened. So things may become complicated by using iframe.

Another concern for me is if we need to manipulate preference inside the iframe, we may need a long way to send message back to chrome process?
Flags: needinfo?(dtownsend)
Just revised the patch for issues that already confirmed.(see mozreview)
(In reply to KM Lee [:rexboy] from comment #11)
> ::: browser/extensions/onboarding/content/onboarding.js:38
> (Diff revision 1)
> >      this._overlayIcon = this._renderOverlayIcon();
> >      this._overlay = this._renderOverlay();
> >      this._window.document.body.appendChild(this._overlayIcon);
> >      this._window.document.body.appendChild(this._overlay);
> >  
> > +    Services.scriptloader.loadSubScript(UITOUR_LIB_JS_URL, this._window);
> 
> Not very sure if I understand. Isn't it loaded just once in each content
> window now?

Sorry I misread the line here.

(In reply to KM Lee [:rexboy] from comment #12)
> Comment on attachment 8873368 [details]
> Bug 1357046 - Add private browsing and search tour to onboarding overlay
> 
> https://reviewboard.mozilla.org/r/144810/#review148964
> 
> Fischer has discussed that with Matt before; We didn't do that because it
> still need to interact with the notification snippet which pops up in
> certain condition, and when user clicks them, certain page in the overlay
> need to be opened. So things may become complicated by using iframe.

If it's been discussed already then that's fine.

> Another concern for me is if we need to manipulate preference inside the
> iframe, we may need a long way to send message back to chrome process?

We're going to hit that problem anyway supporting activity stream loaded in the content process. It shouldn't be too much of a problem.
Flags: needinfo?(dtownsend)
Attachment #8873368 - Flags: review?(dtownsend)
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

Patch updated and I modified the l10n part thanks to bug 1369291. (Which is backed out for now but I believe the error is related only to build scripts, so we can use it in advance without conflict)

May you guys take a look? Thanks a lot!
Attachment #8873368 - Flags: review?(gasolin)
Attachment #8873368 - Flags: review?(fliu)
Attachment #8873368 - Flags: review?(dtownsend)
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review149626

::: browser/extensions/onboarding/content/onboarding.css:104
(Diff revision 3)
> +  height: 48px;
> +  border-inline-start: 6px solid transparent;
> +  padding-inline-start: 66px;
> +  line-height: 48px;
> +  background-repeat: no-repeat;
> +  background-position: left 27px center;

left -> start, right -> end

We should ask UX for detail if we want do rtl right.

::: browser/extensions/onboarding/content/onboarding.css:111
(Diff revision 3)
> +  font-size: 16px;
> +  cursor: pointer;
> +}
> +
> +#onboarding-overlay-dialog > nav > ul > li:dir(rtl) {
> +  background-position: right 27px center;

same rtl issue as above
Attachment #8873368 - Flags: review?(gasolin)
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review149042

> Would you consider lazy creation of tour page? That is doing this works in `gotoPage` such as 
> ```
> // Inside `gotoPage`.
> // this._tourPages is a Map object.
> let targetPage = this._tourPages.get(tourId);
> if (!targetPage) {
>   // Let's create one
> }
> let targetPageId = `${tourId}-tour-page`;
> // Continue the rest of works
> ```
> 
> Consider we will be injected into the Activity-Stream and quite a lof of "activities" heppened inside the Activity-Stream. Lazy creation would help some.
> 
> However, currently we only have 6 static tours, maybe this wouldn't impact too much so we could do this improvement in the future if things get more complex to save simplicity for now.
> It's your design decision.

I think lazy loading is a good idea, but maybe loading all tours at the first time opening overlay is enough?
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review149626

> left -> start, right -> end
> 
> We should ask UX for detail if we want do rtl right.

It seems "start" and "end" is not supported by background-position. So actually in the patch I workarounded that by using :dir(rtl) selector below.
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review149628

::: browser/extensions/onboarding/content/onboarding.css:22
(Diff revision 3)
>    color: #4d4d4d;
>    background: rgb(54, 57, 89, 0.8); /* #363959, 0.8 opacity */
>    display: none;
>  }
>  
>  #onboarding-overlay.opened {

Thanks for prefixing icons, please also prefix `.opened` like what was done for icons. We should handle CSS carefully so as to not afftect or be affected by other page's CSS rule.

::: browser/extensions/onboarding/content/onboarding.js:20
(Diff revision 3)
>  const ABOUT_HOME_URL = "about:home";
>  const ABOUT_NEWTAB_URL = "about:newtab";
>  const BUNDLE_URI = "chrome://onboarding/locale/onboarding.properties";
>  
> +const BRAND_SHORT_NAME = Cc["@mozilla.org/intl/stringbundle;1"]
> +                           .getService(Ci.nsIStringBundleService)

Could use `Services.strings` like what you did inside `constructor`
```
this._bundle = Services.strings.createBundle(BUNDLE_URI);
```

::: browser/extensions/onboarding/content/onboarding.js:46
(Diff revision 3)
> + * },
> + **/
> +var onboardingTours = [
> +  {
> +    id: "onboarding-tour-search",
> +    title: "One-click Search",

Why don't the titles go into the strings bundle?

::: browser/extensions/onboarding/content/onboarding.js:134
(Diff revision 3)
>      this._overlayIcon = this._renderOverlayIcon();
>      this._overlay = this._renderOverlay();
>      this._window.document.body.appendChild(this._overlayIcon);
>      this._window.document.body.appendChild(this._overlay);
>  
> +    Services.scriptloader.loadSubScript(UITOUR_LIB_JS_URL, content);

Please use `this._window` or `contentWindow`. We have Bug 1369750 having something wrong with `content`. Bug 1369750 is making sure the passed-in contentWindow is always available.

::: browser/extensions/onboarding/content/onboarding.js:167
(Diff revision 3)
>    }
>  
>    toggleOverlay() {
> +    if (this._tourItems.length == 0) {
> +      // Lazy loading until first toggle.
> +      this._loadTours(onboardingTours);

Thanks for taking care of this. From the current complexity, this degree of laziness should be sufficient.

::: browser/extensions/onboarding/content/onboarding.js:180
(Diff revision 3)
> +    for (let page of this._tourPages) {
> +      page.style.display = page.id != targetPageId ? "none" : "";
> +    }
> +    for (let li of this._tourItems) {
> +      if (li.id == tourId) {
> +        li.classList.add("active");

Maybe the CSS prefix is also required for `active`. Kind of bothersome, but like Fred's comment 8, it will be a nice convention to always add a prefix before the class name. Thanks.
Attachment #8873368 - Flags: review?(fliu)
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review149800

::: browser/extensions/onboarding/content/onboarding.js:154
(Diff revision 3)
>        // Let's toggle the overlay.
>        case "onboarding-overlay":
>          this.toggleOverlay();
>          break;
>      }
> +    if (evt.target.tagName == "li") {

There's a risk of other list items being added elsewhere in the page. Please add a unique class to the li's that we care about

::: browser/extensions/onboarding/content/onboarding.js:234
(Diff revision 3)
> +    let ul = this._overlay.querySelector("nav > ul");
> +    ul.appendChild(itemsFrag);
> +    let footer = this._overlay.querySelector("footer");

Please use unique classes or ids to find these
Attachment #8873368 - Flags: review?(dtownsend)
Comments above seems reasonable to me and I've addressed them in the revised patch.
Please take a look again, thanks!
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review150104

::: browser/extensions/onboarding/content/onboarding.js:195
(Diff revisions 4 - 5)
>      // The security should be fine because this is not from an external input.
>      // We're not shipping yet so l10n strings is going to be closed for now.
>      div.innerHTML = `
>        <div id="onboarding-overlay-dialog">
>          <button id="onboarding-tour-close-btn">X</button>
>          <header>${this._bundle.formatStringFromName("gettingStarted", [BRAND_SHORT_NAME], 1)}</header>

And this ${} evaluation will be replaced by textContent in bug 1357046.

::: browser/extensions/onboarding/content/onboarding.js:226
(Diff revisions 4 - 5)
>        itemsFrag.appendChild(li);
>        // Dynamically create tour pages
>        let div = tour.getPage(this._window, this._bundle);
> +
> +      // Do a traverse for elements in the page that need to be localized.
> +      let l10nElements = div.querySelectorAll('[data-l10n-id]');

L10n team just gave us a comment that we should use textContent to do localization rather than putting localized texts directly in innerHTML for security concern. So I made this change to fill in l10n strings through textContent programmatically.

For detail see bug 1369291.
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review150160

::: browser/extensions/onboarding/content/onboarding.js:14
(Diff revision 5)
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  Cu.import("resource://gre/modules/Services.jsm");
>  
>  const ONBOARDING_CSS_URL = "resource://onboarding/onboarding.css";
> +const ONBOARDING_TOURS_JS_URL = "resource://onboarding/onboardingTours.js";
> +const UITOUR_LIB_JS_URL = "chrome://browser/content/UITour-lib.js";

Just though of that in the future we may run on a content-process page, not sure the below `Services.scriptloader.loadSubScript(UITOUR_LIB_JS_URL, this._window);` would work or not for this chrome protocol.

::: browser/extensions/onboarding/content/onboarding.js:29
(Diff revision 5)
> +/**
> + * Add any number of tours, following the format
> + * {
> + *   // Tour Id, should be unique in the tours
> + *   id: "addons",
> + *   // The tour name which would be displayed on the navigation bar

Nit: "The string id of tour name which would be displayed on the navigation bar"

::: browser/extensions/onboarding/content/onboarding.js:195
(Diff revision 5)
>      // The security should be fine because this is not from an external input.
>      // We're not shipping yet so l10n strings is going to be closed for now.
>      div.innerHTML = `
>        <div id="onboarding-overlay-dialog">
>          <button id="onboarding-tour-close-btn">X</button>
> -        <header>${this.bundle.GetStringFromName("gettingStarted")}</header>
> +        <header>${this._bundle.formatStringFromName("gettingStarted", [BRAND_SHORT_NAME], 1)}</header>

I think what you want to say is that this l10n will be addressed in Bug 1369291.

::: browser/extensions/onboarding/content/onboarding.js:223
(Diff revision 5)
> +      li.textContent = this._bundle.GetStringFromName(tour.itemNameId);
> +      li.id = tour.id;
> +      li.className = "onboarding-tour-item";
> +      itemsFrag.appendChild(li);
> +      // Dynamically create tour pages
> +      let div = tour.getPage(this._window, this._bundle);

Passing in `this._bundle` is no longer needed
Attachment #8873368 - Flags: review?(fliu)
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review150220

::: browser/extensions/onboarding/content/onboarding.js:133
(Diff revision 5)
>      this._overlayIcon = this._renderOverlayIcon();
>      this._overlay = this._renderOverlay();
>      this._window.document.body.appendChild(this._overlayIcon);
>      this._window.document.body.appendChild(this._overlay);
>  
> +    Services.scriptloader.loadSubScript(UITOUR_LIB_JS_URL, this._window);

Just tried this way with Activity-Stream enabled. Will get error like "TypeError: this._window.Mozilla is undefined".
We might need to use UIToour.jsm instead.
(In reply to Fischer [:Fischer] from comment #28)
> Comment on attachment 8873368 [details]
> Bug 1357046 - Add private browsing and search tour to onboarding overlay.
> 
> https://reviewboard.mozilla.org/r/144810/#review150220
> 
> ::: browser/extensions/onboarding/content/onboarding.js:133
> (Diff revision 5)
> >      this._overlayIcon = this._renderOverlayIcon();
> >      this._overlay = this._renderOverlay();
> >      this._window.document.body.appendChild(this._overlayIcon);
> >      this._window.document.body.appendChild(this._overlay);
> >  
> > +    Services.scriptloader.loadSubScript(UITOUR_LIB_JS_URL, this._window);
> 
> Just tried this way with Activity-Stream enabled. Will get error like
> "TypeError: this._window.Mozilla is undefined".
> We might need to use UIToour.jsm instead.

That's a strange error, I'd expect something else if we couldn't load the url. This does make me wonder what the security aspects of loading the code like this are. Can we instead inject a <script> element into the document to make sure it is the document doing the loading?
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review149972

::: browser/extensions/onboarding/content/onboarding.css:49
(Diff revision 4)
> +  cursor: pointer;
>  }
>  
> -#onboarding-overlay.opened > #onboarding-overlay-dialog {
> +#onboarding-overlay.onboarding-opened > #onboarding-overlay-dialog {
>    width: 1200px;
>    height: 550px;

based on recent spec, the width and height are 960x510px, the margin shoould be 0 calc(50% - 480px); 
related UI style should get update accordingly. 

You can fix that in this bug, or just make sure the followup bug is filed

::: browser/extensions/onboarding/content/onboarding.css:44
(Diff revision 5)
>  
>  #onboarding-tour-close-btn {
>    position: absolute;
>    top: 15px;
>    offset-inline-end: 15px;
> +  cursor: pointer;

we can left the change to bug 1369282 (close btn UI) which I'll double check if we need to show the point cursor with verdi

::: browser/extensions/onboarding/content/onboarding.css:91
(Diff revision 5)
>  #onboarding-overlay-dialog > footer {
>    grid-row: footer-start;
>    grid-column: dialog-start / tour-end;
>  }
> +
> +/* Onboarding tour menu */

shoule we describe it as `Onboarding tour list`?

::: browser/extensions/onboarding/content/onboarding.js:30
(Diff revision 5)
> + * Add any number of tours, following the format
> + * {
> + *   // Tour Id, should be unique in the tours
> + *   id: "addons",
> + *   // The tour name which would be displayed on the navigation bar
> + *   itemNameId: "addonsTourItemName",

how about call it `tourNameId`

::: browser/extensions/onboarding/content/onboarding.js:32
(Diff revision 5)
> + *   // Tour Id, should be unique in the tours
> + *   id: "addons",
> + *   // The tour name which would be displayed on the navigation bar
> + *   itemNameId: "addonsTourItemName",
> + *   // Return a div appended with elements for this tours.
> + *   // There should be 3 sections in the div,

Each tour should contain following 3 sections in the div: `.onboarding-tour-description`, `.onboarding-tour-content`, `.onboarding-tour-button.`

::: browser/extensions/onboarding/content/onboarding.js:35
(Diff revision 5)
> + *   itemNameId: "addonsTourItemName",
> + *   // Return a div appended with elements for this tours.
> + *   // There should be 3 sections in the div,
> + *   // marked with 3 css selectors respectively.
> + *   // They are .onboarding-tour-description, .onboarding-tour-content, .onboarding-tour-button.
> + *   // If no button, add no-button css class in the div.

Add `no-button` css class in the div if this tour does not need a button.

::: browser/extensions/onboarding/content/onboarding.js:45
(Diff revision 5)
> + * },
> + **/
> +var onboardingTours = [
> +  {
> +    id: "onboarding-tour-search",
> +    itemNameId: "searchTourItemName",

tourNameId: "onboarding.tour-search"

::: browser/extensions/onboarding/content/onboarding.js:62
(Diff revision 5)
> +          <button data-l10n-id="searchTourButton"></button>
> +        </aside>
> +      `;
> +      div.addEventListener("click", e => {
> +        if (e.target.tagName == "button") {
> +          win.Mozilla.UITour.openSearchPanel(() => {});

Since there are some issues around doing UITour actions in `remote=true` pages(about:home & activity-stream), I suggest not add button actions in this bug and solve the UITour related issues in the follwup bugs.

::: browser/extensions/onboarding/content/onboarding.js:78
(Diff revision 5)
> +      return true;
> +    },
> +  },
> +  {
> +    id: "onboarding-tour-private-browsing",
> +    itemNameId: "privateBrowsingItemName",

tourNameId: "onboarding.tour-private-browsing"

::: browser/extensions/onboarding/locale/en-US/onboarding.properties:7
(Diff revision 5)
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +#
> +# LOCALIZATION NOTE (gettingStarted): %S is brandShortName
>  
> -gettingStarted=Getting started?
> +gettingStarted=Getting started with %S

string are now also prefixed as `onboarding.overlay-` or `onboarding.tour-`, please refer recent change in Bug 1369291
Attachment #8873368 - Flags: review?(gasolin)
(In reply to Fred Lin [:gasolin] from comment #30)
> ::: browser/extensions/onboarding/content/onboarding.js:78
> (Diff revision 5)
> > +      return true;
> > +    },
> > +  },
> > +  {
> > +    id: "onboarding-tour-private-browsing",
> > +    itemNameId: "privateBrowsingItemName",
> 
> tourNameId: "onboarding.tour-private-browsing"
> 
In the future, we may use the tour id as part of the completion pref, such as "onboarding.tour.privateBrowsing.completed".
So not sure if this kind of pref is goof for reading: "onboarding.tour.onboarding.tour-private-browsing.completed"
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review150594

::: browser/extensions/onboarding/content/onboarding.css:49
(Diff revision 5)
> +  cursor: pointer;
>  }
>  
> -#onboarding-overlay.opened > #onboarding-overlay-dialog {
> +#onboarding-overlay.onboarding-opened > #onboarding-overlay-dialog {
>    width: 1200px;
>    height: 550px;

I think we can do the polish things in bug 1370459.

::: browser/extensions/onboarding/content/onboarding.js:14
(Diff revision 5)
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  Cu.import("resource://gre/modules/Services.jsm");
>  
>  const ONBOARDING_CSS_URL = "resource://onboarding/onboarding.css";
> +const ONBOARDING_TOURS_JS_URL = "resource://onboarding/onboardingTours.js";
> +const UITOUR_LIB_JS_URL = "chrome://browser/content/UITour-lib.js";

For activity stream and about:home, they are running in content process already. 

I just did some investigation. Seems for the content process case: The window.Mozilla which UITour-lib attached to its window object could not be accessed by frame script. And seems this causes the strange problem you met earlier in activity stream.
I just came up some alternatives for that, but I agree with Fred that it may be better to do that in another bug, like reopening bug 1357049 so we can unblock your work for notification.

::: browser/extensions/onboarding/content/onboarding.js:62
(Diff revision 5)
> +          <button data-l10n-id="searchTourButton"></button>
> +        </aside>
> +      `;
> +      div.addEventListener("click", e => {
> +        if (e.target.tagName == "button") {
> +          win.Mozilla.UITour.openSearchPanel(() => {});

Agree with you, let's breakdown it to unblock Fischer's notification works earlier.

::: browser/extensions/onboarding/content/onboarding.js:133
(Diff revision 5)
>      this._overlayIcon = this._renderOverlayIcon();
>      this._overlay = this._renderOverlay();
>      this._window.document.body.appendChild(this._overlayIcon);
>      this._window.document.body.appendChild(this._overlay);
>  
> +    Services.scriptloader.loadSubScript(UITOUR_LIB_JS_URL, this._window);

Or I think injecting the event listeners to buttons into the content page may work based on my investigation. Anyway discussing the detail in another bug may get things simpler.

::: browser/extensions/onboarding/content/onboarding.js:223
(Diff revision 5)
> +      li.textContent = this._bundle.GetStringFromName(tour.itemNameId);
> +      li.id = tour.id;
> +      li.className = "onboarding-tour-item";
> +      itemsFrag.appendChild(li);
> +      // Dynamically create tour pages
> +      let div = tour.getPage(this._window, this._bundle);

My bad :-/
Oops. Seems I didn't use the mozreview in the right way. Let me do the reply again.
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review149972

> based on recent spec, the width and height are 960x510px, the margin shoould be 0 calc(50% - 480px); 
> related UI style should get update accordingly. 
> 
> You can fix that in this bug, or just make sure the followup bug is filed

I think we can do the polish in a whole in bug 1370459.

> Since there are some issues around doing UITour actions in `remote=true` pages(about:home & activity-stream), I suggest not add button actions in this bug and solve the UITour related issues in the follwup bugs.

Agree with you, let's breakdown it to unblock Fischer's notification works earlier.
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review150220

> Just tried this way with Activity-Stream enabled. Will get error like "TypeError: this._window.Mozilla is undefined".
> We might need to use UIToour.jsm instead.

Or I think injecting the event listeners to buttons into the content page may work based on my investigation. Anyway discussing the detail in another bug may get things simpler.
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review150160

> Just though of that in the future we may run on a content-process page, not sure the below `Services.scriptloader.loadSubScript(UITOUR_LIB_JS_URL, this._window);` would work or not for this chrome protocol.

For activity stream and about:home, they are running in content process already. 

I just did some investigation. Seems for the content process case: The window.Mozilla which UITour-lib attached to its window object could not be accessed by frame script. And seems this causes the strange problem you met earlier in activity stream.
I just came up some alternatives for that, but I agree with Fred that it may be better to do that in another bug, like reopening bug 1357049 so we can unblock your work for notification.
(In reply to Dave Townsend [:mossop] from comment #29)
> That's a strange error, I'd expect something else if we couldn't load the
> url. This does make me wonder what the security aspects of loading the code
> like this are. Can we instead inject a <script> element into the document to
> make sure it is the document doing the loading?

Injecting script tag doesn't work either. The error keeps the same as long as the window is opened in the content process (which is about:home and activity stream case).
The major change for this version is that buttons and scripts are removed. We can continue that work and discussion on bug 1357049.
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review150848
Attachment #8873368 - Flags: review?(dtownsend) → review+
Blocks: 1357049
No longer blocks: 1357049
Blocks: 1357049
This bug relies on the bug 1369291 which builds the l10n support so should not get checked-in before the bug 1369291 is landed into the mc.
Depends on: 1369291
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review151080

Thanks for the works. Some accidentally mis-placed comments. Please notice this bug depends on the bug 1369291 before checking (See the comment 41)

::: browser/extensions/onboarding/content/onboarding.js:26
(Diff revision 6)
> +
> +/**
> + * Add any number of tours, following the format
> + * {
> + *   // The string id of tour name which would be displayed on the navigation bar
> + *   id: "onboarding-tour-addons",

This should be the unique tour id but not string id

::: browser/extensions/onboarding/content/onboarding.js:28
(Diff revision 6)
> + * Add any number of tours, following the format
> + * {
> + *   // The string id of tour name which would be displayed on the navigation bar
> + *   id: "onboarding-tour-addons",
> + *   // The tour name which would be displayed on the navigation bar
> + *   tourNameId: "onboarding.tour-addon",

This should be "The string id of tour name which would be displayed on the navigation bar"
Attachment #8873368 - Flags: review?(fliu) → review+
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review151176

Looks good, thanks

::: browser/extensions/onboarding/content/onboarding.js:17
(Diff revision 6)
>  const ONBOARDING_CSS_URL = "resource://onboarding/onboarding.css";
>  const ABOUT_HOME_URL = "about:home";
>  const ABOUT_NEWTAB_URL = "about:newtab";
>  const BUNDLE_URI = "chrome://onboarding/locale/onboarding.properties";
>  
> +const BRAND_SHORT_NAME = Services.strings

this line will be lazy loaded when we render the overlay title, so please make sure it's been removed from this place after rebase
Attachment #8873368 - Flags: review?(gasolin) → review+
Blocks: 1357029
No longer blocks: 1371519
Tagging checkin-needed, rebased onto the latest patch of bug 1369291.

Thanks all you guys, lets move the remaining discussion of embedding button to bug 1357049.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0cfb320a3026
Add private browsing and search tour to onboarding overlay. r=Fischer,gasolin,mossop
Keywords: checkin-needed
backed out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=105808925&repo=autoland
Flags: needinfo?(rexboy)
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review151810

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:7
(Diff revision 8)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> -
>  # LOCALIZATION NOTE(onboarding.tour-title): This string will be used in the overlay title
>  # %S is brandShortName
>  onboarding.overlay-title=Getting started with %S
> +onboarding.tour-search=One-Click Search

Was this changed from the original version? I remember seeing "Open One-Click Search"

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:8
(Diff revision 8)
> -
>  # LOCALIZATION NOTE(onboarding.tour-title): This string will be used in the overlay title
>  # %S is brandShortName
>  onboarding.overlay-title=Getting started with %S
> +onboarding.tour-search=One-Click Search
> +onboarding.tour-search.title=Find it faser.

Typo

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:11
(Diff revision 8)
>  onboarding.overlay-title=Getting started with %S
> +onboarding.tour-search=One-Click Search
> +onboarding.tour-search.title=Find it faser.
> +onboarding.tour-search.description=Access all of your favorite search engines with a click. Search the whole Web or just one website right from the search box.
> +onboarding.tour-private-browsing=Private Browsing
> +onboarding.tour-private-browsing.title=Browser by yourself.

Typo I assume (Browse)
At risk of sounding harsh, please reserve to strings the same attention that you reserve to code.

At this point I would suggest to wait post merge day to land this, since I assume it's not needed for 55.
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/111431396384
Backed out changeset 0cfb320a3026 for test failures in browser_misused_characters_in_strings.js
(In reply to Francesco Lodolo [:flod] from comment #50)
> Comment on attachment 8873368 [details]
> Bug 1357046 - Add private browsing and search tour to onboarding overlay.
> 
> https://reviewboard.mozilla.org/r/144810/#review151810
> 
> ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:7
> (Diff revision 8)
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > -
> >  # LOCALIZATION NOTE(onboarding.tour-title): This string will be used in the overlay title
> >  # %S is brandShortName
> >  onboarding.overlay-title=Getting started with %S
> > +onboarding.tour-search=One-Click Search
> 
> Was this changed from the original version? I remember seeing "Open
> One-Click Search"
No it wasn't. The "Open One-Click search" is a button which we decided to check-in in another bug after a few runs of review. So it's not included in this patch.
> 
> ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:8
> (Diff revision 8)
> > -
> >  # LOCALIZATION NOTE(onboarding.tour-title): This string will be used in the overlay title
> >  # %S is brandShortName
> >  onboarding.overlay-title=Getting started with %S
> > +onboarding.tour-search=One-Click Search
> > +onboarding.tour-search.title=Find it faser.
> 
> Typo
> 
> ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:11
> (Diff revision 8)
> >  onboarding.overlay-title=Getting started with %S
> > +onboarding.tour-search=One-Click Search
> > +onboarding.tour-search.title=Find it faser.
> > +onboarding.tour-search.description=Access all of your favorite search engines with a click. Search the whole Web or just one website right from the search box.
> > +onboarding.tour-private-browsing=Private Browsing
> > +onboarding.tour-private-browsing.title=Browser by yourself.
> 
> Typo I assume (Browse)

Oh, it's my bad. sorry for that.
(In reply to Francesco Lodolo [:flod] from comment #51)
> At risk of sounding harsh, please reserve to strings the same attention that
> you reserve to code.
> 
Sorry for that, I'll include l10n review the next time.
> At this point I would suggest to wait post merge day to land this, since I
> assume it's not needed for 55.
Yes, the add-on only get built in Nightly. It's only next Monday but if we can land it earlier that may be a little bit easier for dependent works. I'll ask my team member.
I see a completely different text here from what's in the bug. Is that supposed to happen?
https://bugzilla.mozilla.org/show_bug.cgi?id=1354707#c14
flod, those strings are used for onboarding notifications, not for the overlay. So we can stick on current strings.
(In reply to Fred Lin [:gasolin] from comment #57)
> flod, those strings are used for onboarding notifications, not for the
> overlay. So we can stick on current strings.

Thanks! I was getting really confused.
Sorry, the strings seems need to be updated based on bug 1354707 comment 14. 
I've discussed with Rex and we likely land this PR after the merge day.
The string and order are changed, please update accordingly before landing

[1: Private Browsing]
Navigation link: Private Browsing
Headline: A little privacy goes a long way.
Body: Browse the internet without saving your searches or the sites you visited. When your session ends, the cookies disappear from Firefox like they were never there.
Button: Show Private Browsing in Menu

[4: Search]
Navigation link: One-click Search
Headline: Find the needle or the haystack.
Body: Having a default search engine doesn’t mean it’s the only one you use. Pick a search engine or a site, like Amazon or Wikipedia, to search on the fly. 
Button: Show One-Click Search
(In reply to Fred Lin [:gasolin] from comment #60)
> Body: Having a default search engine doesn’t mean it’s the only one you use.
> Pick a search engine or a site, like Amazon or Wikipedia, to search on the
> fly. 

Note that there's a pending question for this one. Hopefully verdi will clarify it quickly.
Blocks: 1370459
(In reply to Francesco Lodolo [:flod] from comment #61)
> (In reply to Fred Lin [:gasolin] from comment #60)
> > Body: Having a default search engine doesn’t mean it’s the only one you use.
> > Pick a search engine or a site, like Amazon or Wikipedia, to search on the
> > fly. 
> 
> Note that there's a pending question for this one. Hopefully verdi will
> clarify it quickly.

Per bug 1354707 comment 22 the solution seems to be adding a LOCALIZATION NOTE like "If Amazon or Wikipedia is not pre-installed in your language, replace them to pre-installed sites that are non-search "engines" (e.g. Google, Bing, Yahoo, Yandex, Baidu, etc.) that supports one-off search. If there are only one such kind of site, just list one."

Any futher suggestions :flod?
Flags: needinfo?(rexboy) → needinfo?(francesco.lodolo)
In Bug 1354707 Comment 23, UX provide new icons today, please update and set with the right color...
Depends on: 1354707
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review152688

::: browser/extensions/onboarding/content/onboarding.js:210
(Diff revision 9)
> +      // Do a traverse for elements in the page that need to be localized.
> +      let l10nElements = div.querySelectorAll("[data-l10n-id]");
> +      for (let i = 0; i < l10nElements.length; i++) {
> +        let element = l10nElements[i];
> +        element.textContent = this._bundle
> +                              .GetStringFromName(element.dataset.l10nId);

this could be in the same line
After discussion with Fischer, we plan not to update to new icons now and wait for a batch update once we make most v56 work done. That will give UX some extra time to revise the illustration and icons.
Let's use bug 1370459 for the icon case because we started engineering works to catch the schedule before the late visual spec lockdown. Let's put those remaining polishing works for visual assets there after lockdown rather than following the visual updates incrementally.
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

The localization note above is added to the patch. Flod would you take a look, thanks!
Attachment #8873368 - Flags: review?(francesco.lodolo)
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review152734

::: browser/extensions/onboarding/content/onboarding.js:64
(Diff revision 11)
> +      // TODO: set completion to preferences.
> +      return true;
> +    },
> +  },
> +  {
> +    id: "onboarding-tour-private-browsing",

As new spec the private browsing is the first tour, please change the order accordingly...

https://mozilla.invisionapp.com/share/4MBDUMS5W#/screens/230316106
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review152740

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:6
(Diff revision 11)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> -
> -# LOCALIZATION NOTE(onboarding.tour-title): This string will be used in the overlay title
> -# %S is brandShortName
> +# LOCALIZATION NOTE(onboarding.tour-title): This string will be used in the
> +# overlay title. %S is brandShortName.
> +# LOCALIZATION NOTE(onboarding.tour-search.description): If Amazon or Wikipedia

1) Move this comment before the string itself

2) I would reword it a bit (Wikipedia ships in all languages).

# LOCALIZATION NOTE (onboarding.tour-search.description): If Amazon is not part
# of the default searchplugins for your locale, you can replace it with another
# ecommerce website (if you're shipping one), but not with a general purpose
# search engine (Google, Bing, Yandex, etc.). Alternatively, only reference
# Wikipedia and drop Amazon from the text.

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:10
(Diff revision 11)
> -# LOCALIZATION NOTE(onboarding.tour-title): This string will be used in the overlay title
> -# %S is brandShortName
> +# overlay title. %S is brandShortName.
> +# LOCALIZATION NOTE(onboarding.tour-search.description): If Amazon or Wikipedia
> +# is not pre-installed in your language, replace them with pre-installed sites
> +# that are not search "engines" (e.g. Google, Bing, Yahoo, Yandex, Baidu, etc.)
> +# If there are only one such kind of pre-installed site, just list that one.
> +# LOCALIZAITON NOTE(onboarding.tour-private-browsing.description): %S is

typo: localization
Attachment #8873368 - Flags: review?(francesco.lodolo)
:flod thanks for the quick response. I'm still working on the patch so just a minute.
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8873368 [details]
Bug 1357046 - Add private browsing and search tour to onboarding overlay.

https://reviewboard.mozilla.org/r/144810/#review152760

This looks good to me now.

The might be one more bit: usually I see "the Internet" in Firefox (uppercase), not sure if they changed their mind. Bug 1358293 is recent and still uses uppercase.
Attachment #8873368 - Flags: review?(francesco.lodolo) → review+
"the Internet" seems applied in every other strings http://searchfox.org/mozilla-central/search?q=the%20internet&path=

Michael could you help check if we should still use "the internet"(all small case)? By the time I think we should change the string to "the Internet" and land it first.
Flags: needinfo?(mverdi)
I would wait for Michael to clear the question to land the right string.
Keywords: checkin-needed
Updated to upper case first since we looks like to expect that more.
(In reply to Fred Lin [:gasolin] from comment #76)
> "the Internet" seems applied in every other strings
> http://searchfox.org/mozilla-central/search?q=the%20internet&path=
> 
> Michael could you help check if we should still use "the internet"(all small
> case)? By the time I think we should change the string to "the Internet" and
> land it first.

"the internet" all lowercase is correct. I verified this with Michelle Heubusch.
Flags: needinfo?(mverdi)
changed back to "internet" per comment 80.
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
Despite 2 issues shown on the top, there are neither open issues nor fix I an click. I suppose it's a bug of MozReview.
ni? for next step we can take.
Flags: needinfo?(ryanvm)
Flags: needinfo?(cbook)
Sounds like a question for the people who maintain MozReview, not the people who just have to land things from it ;)
Flags: needinfo?(ryanvm)
Flags: needinfo?(mcote)
Flags: needinfo?(cbook)
Since we can confirm the patch on MozReview is actually with all issues resolved and check-in ready, is there a possible way to land the patch first while waiting for MozReview people fixing the status?
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm) → needinfo?(cbook)
i've resolved the incorrect issue count on https://reviewboard.mozilla.org/r/144810/ so it can be autolanded now.
Flags: needinfo?(mcote)
No longer blocks: 1371531
No longer blocks: 1371533
No longer blocks: 1371534
No longer blocks: 1371535
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/610e037ab24d
Add private browsing and search tour to onboarding overlay. r=Fischer,flod,gasolin,mossop
Keywords: checkin-needed
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/610e037ab24d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Depends on: 1373303
This feature "add the Private Browsing tour in the onBoarding overlay" has been implemented with Latest Nightly 56.0a1 on Windows 8.1 (64 bit).

Build ID : 20170616030207
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170614]
This issue has been verified on Win 10 x64.
Status: RESOLVED → VERIFIED
No longer depends on: 1373303
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: