[Form Autofill] Show a doorhanger to allow opting out of autofill saving when users submit a form for the first time

RESOLVED FIXED in Firefox 56

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: lchang, Assigned: steveck)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [form autofill:M3][ETA:612])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
When users first time submit a form, a door hanger will be prompted form the top left to ask users to save the form as autofill profile. Users can click on "save" or "Don't save" to decide whether to save the profile or not.
Whiteboard: [form autofill:M1] → [form autofill]
(Reporter)

Updated

2 years ago
Whiteboard: [form autofill] → [form autofill:MVP]
Whiteboard: [form autofill:MVP] → [form autofill:M2]
(Reporter)

Comment 1

2 years ago
According to the latest UX Spec, the door hanger of saving profile will only appear once.
Summary: [Form Autofill] Prompt a door hanger to ask users to save profile when users summit a new form → [Form Autofill] Prompt a door hanger to notify users that profile has been saved when users summit a new form for the first time
(Assignee)

Updated

2 years ago
Blocks: 990199
(Assignee)

Comment 2

2 years ago
Hi Matt, is there any way we can inject the assets/string into browser.xul for the new doorhanger, or we should modify the browser.xul directly? I didn't see anyone tried to implement a new one as an addon, but maybe there's a better way like reusing the existing doorhanger?
Flags: needinfo?(MattN+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
Comment on attachment 8869372 [details]
Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger.

Hi Matt, it's a WIP about the first time saving doorhanger. It's simple popupNotifications wrap up object instead of prototype or class, which I don't think we can have much benefit from. It still has some problem in preferences link and lack of visual assets, but other basic functions should work. I'll raise some questions in the patch later.
Attachment #8869372 - Flags: feedback?(MattN+bmo)
(Assignee)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8869372 [details]
Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger.

https://reviewboard.mozilla.org/r/141022/#review145202

::: browser/extensions/formautofill/FormAutofillContent.jsm:323
(Diff revision 2)
>     * Send the profile to parent for doorhanger and storage saving/updating.
>     *
>     * @param {Object} profile Submitted form's address/creditcard guid and record.
>     */
> -  _onFormSubmit(profile) {
> -    Services.cpmm.sendAsyncMessage("FormAutofill:OnFormSubmit", profile);
> +  _onFormSubmit(profile, domWin) {
> +    let win = this.messageManagerFromWindow(domWin);

Not sure if we have better way to reference the browser and chrome window in parent process. Here I align the solution in passwordmgr that sending message via content message manager.

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:42
(Diff revision 2)
> +      accessKey: "A",
> +      callbackState: "apply-autofill",
> +    }],
> +    options: {
> +      persistWhileVisible: true,
> +      learnMoreURL: "about:preferences#privacy",

We probably can't leverage the existing learnMoreURL element in popupNotification since the the string and way of opening the url seems unable to change. Should we get the learnMoreURL element the overwrite it, or create another learnMoreURL-like element and insert it?

::: browser/extensions/formautofill/FormAutofillParent.jsm:270
(Diff revision 2)
>    },
>  
>    _onFormSubmit(data, target) {
>      let {address} = data;
>  
> +    FormAutofillDoorhanger.init(target);

Using init here seems not appropriate because the browser and chrome win might vary. Maybe we only need show method and set the browser as one of the parameter?
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Flags: needinfo?(MattN+bmo)
(Assignee)

Updated

2 years ago
Attachment #8869372 - Flags: feedback?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
I'm still working on the mochitest part, but the overall doorhanger's functionality should be ready in the 2 commits.
Assignee: nobody → schung
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8869372 [details]
Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger.

https://reviewboard.mozilla.org/r/141022/#review148598

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:5
(Diff revision 6)
> +/*
> + * Implements doorhanger singleton that wraps up the PopupNotifications and handles
> + * the doorhager UI for formautofill related features.
> + */

We probably shouldn't add another JSM and instead put this in the Parent JSM but I believe there are plans to share the JSM compartment which will remove the overhead IIUC.

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:32
(Diff revision 6)
> +
> +const CONTENT = {
> +  firstTimeUse: {
> +    notificationId: "autofill-address",
> +    message: GetStringFromName("saveAddressMessage"),
> +    anchorId: null,

As we discussed before, you can append the anchor <image> to the window (see the link I sent you on IRC a while ago) to use as the anchor.

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:66
(Diff revision 6)
> +let FormAutofillDoorhanger = {
> +  _win: null,
> +  _browser: null,
> +  _type: null,
> +  _notificationcontent: null,

Why is this a singleton when there can be multiple instances at once? This JSM is shared with the whole parent process but there can be multiple (non-FTU) doorhangers at once in multiple windows.

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:109
(Diff revision 6)
> +      this._notificationcontent = chromeDoc.createElement("popupnotificationcontent");
> +      let privacyLinkElement = chromeDoc.createElement("label");
> +      privacyLinkElement.className = "text-link";
> +      privacyLinkElement.setAttribute("value", GetStringFromName("viewAutofillOptions"));
> +      privacyLinkElement.addEventListener("click", this);
> +      this._notificationcontent.appendChild(privacyLinkElement);

Why do this programmatically when you can do it declaratively like usual? You can put the binding in https://dxr.mozilla.org/mozilla-central/source/browser/extensions/formautofill/content/formautofill.xml

I guess this keeps the code closer together and makes it easier to use .properties so it can stay

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:113
(Diff revision 6)
> +    if (!this._notificationcontent) {
> +      this._notificationcontent = chromeDoc.createElement("popupnotificationcontent");
> +      let privacyLinkElement = chromeDoc.createElement("label");
> +      privacyLinkElement.className = "text-link";
> +      privacyLinkElement.setAttribute("value", GetStringFromName("viewAutofillOptions"));
> +      privacyLinkElement.addEventListener("click", this);

For XUL elements you should use the "command" event so that it also handles keyboard activation

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:126
(Diff revision 6)
> +  init(browser) {
> +    log.debug("init");
> +    this._win = browser.ownerGlobal;
> +    this._browser = browser;
> +  },

I suspect we could make this work without state specific to the window or browser if we pass the browser to `show`

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:153
(Diff revision 6)
> +  show(type) {
> +    log.debug("show doorhanger with type:", type);
> +    this._type = type;
> +    return new Promise((resolve) => {

Use an `async` function?

::: browser/extensions/formautofill/FormAutofillParent.jsm:50
(Diff revision 6)
>  
>  this.log = null;
>  FormAutofillUtils.defineLazyLogGetter(this, this.EXPORTED_SYMBOLS[0]);
>  
>  const ENABLED_PREF = "extensions.formautofill.addresses.enabled";
> +const FIRST_TIME_USE = "extensions.formautofill.firstTimeUse";

I would just inline the pref as it reduces the indirection when looking usage up.

::: browser/extensions/formautofill/FormAutofillParent.jsm:293
(Diff revision 6)
> +          this.profileStorage.addresses.getAll().forEach((record) => {
> +            this.profileStorage.addresses.remove(record.guid);

Last time I discussed this with Juwei she said we're not supposed to remove from storage when disabled from preferences… why would this be different? If a user has sync setup and gets this doorhanger on one computer and doesn't want to use autofill on this computer, it shouldn't delete the sync profiles on all their devices.

Can we instead delay the .add until the doorhanger is dismissed? Or if we really have to, use the return value of .add to only delete the record that was added?

::: browser/extensions/formautofill/content/icon-address-save.svg:2
(Diff revision 6)
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- Generator: Adobe Illustrator 21.1.0, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->

Nit: Remove this. You should probably follow https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines which include style guidelines and some tools to help with cleaning SVG files up.

::: browser/extensions/formautofill/locale/en-US/formautofill.properties:8
(Diff revision 6)
> +saveAddressMessage = Firefox now saves your form data to help you fill out forms faster!
> +saveAddressMainLabel = Don't Use Autofill
> +saveAddressSecondaryLabel = Use Autofill
> +viewAutofillOptions = View Form Autofill options...

We should probably wait for Michelle to confirm these before landing to avoid having to change the string IDs later after translations already started.
Attachment #8869372 - Flags: review?(MattN+bmo)
Status: NEW → ASSIGNED
Summary: [Form Autofill] Prompt a door hanger to notify users that profile has been saved when users summit a new form for the first time → [Form Autofill] Show a doorhanger to allow opting out of autofill saving when users submit a form for the first time
(Assignee)

Updated

2 years ago
Whiteboard: [form autofill:M2] → [form autofill:M2][ETA:612]
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1303511
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1340466
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

2 years ago
mozreview-review-reply
Comment on attachment 8869372 [details]
Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger.

https://reviewboard.mozilla.org/r/141022/#review148598

> For XUL elements you should use the "command" event so that it also handles keyboard activation

It seems like the elements supported command event are limited(at least label element could not support). Do we need to changed the element to button or something else(which will need less style changes to make it looks like a link)?
(Assignee)

Comment 26

2 years ago
mozreview-review-reply
Comment on attachment 8869372 [details]
Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger.

https://reviewboard.mozilla.org/r/141022/#review148598

> Use an `async` function?

Not sure how we can apply await/async inside the show function... or you mean await doorhanger.show in parent?
Comment on attachment 8870765 [details]
Bug 1303510 - Part 2: Add browser test for doorhanger part,

https://reviewboard.mozilla.org/r/142264/#review150596

Thanks
Attachment #8870765 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8871636 [details]
Bug 1303510 - Part 3: Add mochitest-plain for saving submitted form.

https://reviewboard.mozilla.org/r/143146/#review150600

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:9
(Diff revision 3)
> +const VALID_ADDRESS_FIELDS = [
> +  "given-name",
> +  "additional-name",
> +  "family-name",
> +  "organization",
> +  "street-address",
> +  "address-level2",
> +  "address-level1",
> +  "postal-code",
> +  "country",
> +  "tel",
> +  "email",
> +];

This seems to be unused. If we need this then we should probably access it from ProfileStorage

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:91
(Diff revision 3)
> +function checkAddresses(expectedAddresses) {
> +  return new Promise(resolve => {

Hmm… are async functions supported here?
Comment on attachment 8869372 [details]
Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger.

https://reviewboard.mozilla.org/r/141022/#review150498

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:42
(Diff revision 8)
> +  /*
> +  update: {
> +    notificationId: "autofill-address",
> +    message:
> +    anchorId:
> +    mainAction: {
> +      label:
> +      accessKey:
> +      callbackState:
> +    },
> +    secondaryAction: [{
> +      label:
> +      accessKey:
> +      callbackState:
> +    }],
> +    options: {
> +      persistWhileVisible:,
> +      popupIconURL: "chrome://formautofill/content/icon-address-save.svg",
> +    },
> +  },
> +  */

Let's try not to land too much commented out code as we're starting to stabilize more. Can you move this to a different commit in another bug.

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:66
(Diff revision 8)
> +  },
> +  */
> +};
> +
> +let FormAutofillDoorhanger = {
> +  _notificationcontent: null,

This is stil going to be incorrectly shared across windows I think. Can you either make this a WeakMap with the chromewindow/browser.xul document as the key or simply lookup the created element by ID in the appropriate browser document each time? Since `_appendPrivacyPanelLink` is the only API that needs this then it shouldn't be too much of a perf. issue to do the one getElementById every time especially since it's not often shown.

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:94
(Diff revision 8)
> +    secondaryActionParams.forEach((params) => {
> +      let cb = resolve.bind(null, params.callbackState);

Why not use for…of?

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:96
(Diff revision 8)
> +      secondaryActions.push(
> +        {label: params.label, accessKey: params.accessKey, callback: cb}
> +      );

Nit: One property per line with a trailing comma to make it easier to follow blame for single line changes

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:118
(Diff revision 8)
> +      privacyLinkElement.className = "text-link";
> +      privacyLinkElement.setAttribute("value", GetStringFromName("viewAutofillOptions"));
> +      this._notificationcontent.appendChild(privacyLinkElement);

If you set the attribute "useoriginprincipal" to "true" then you can simply set the "href" attribute to "about:preferences#privacy" and avoid the event listener.

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:125
(Diff revision 8)
> +      this._notificationcontent.appendChild(privacyLinkElement);
> +    }
> +    notification.append(this._notificationcontent);
> +  },
> +  /**
> +   * Create an image element for notification anchor.

…if it doesn't already exist.

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:165
(Diff revision 8)
> +  show(browser, type) {
> +    log.debug("show doorhanger with type:", type);
> +    return new Promise((resolve) => {

Nit: Make this an async function since it returns a promise?

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:178
(Diff revision 8)
> +          case "shown":
> +            this._appendPrivacyPanelLink(browser, content.notificationId);

Doesn't this have to be during "showing" instead of "shown" in order to avoid a flicker?

::: browser/extensions/formautofill/content/icon-address-save.svg:1
(Diff revision 8)
> +<svg id="Layer_1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32">

Nit: Remove @id

::: browser/extensions/formautofill/content/icon-address-save.svg:1
(Diff revision 8)
> +<svg id="Layer_1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32">

Double-check if SVG files are supposed to have a license

::: browser/extensions/formautofill/content/icon-address-save.svg:3
(Diff revision 8)
> +    .st0{fill:#999899;}
> +  </style>
> +  <path class="st0" d="M22 13.7H9.4c-.6 0-1.2.5-1.2 1.2 0 .6.5 1.2 1.2 1.2H22c.6 0 1.2-.5 1.2-1.2s-.6-1.2-1.2-1.2zM6.1 26.6V5.5c0-.8.7-1.5 1.5-1.5h16c.9 0 1.5.6 1.5 1.5V16h2V3.8c0-1-.7-1.8-1.8-1.8H5.9c-1 0-1.8.8-1.8 1.8v24.5c0 1 .8 1.7 1.8 1.7h9.3v-2H7.6c-.8 0-1.5-.6-1.5-1.4zm21.1-1.9h-2.5V20c0-.4-.3-.8-.8-.8h-3.1c-.4 0-.8.3-.8.8v4.6h-2.5c-.6 0-.8.4-.3.8l4.3 4.2c.2.2.5.3.8.3s.6-.1.8-.3l4.3-4.2c.6-.4.4-.7-.2-.7zm-11.3-5.6H9.4c-.6 0-1.2.5-1.2 1.2s.5 1.2 1.2 1.2h6.5c.6 0 1.2-.5 1.2-1.2s-.6-1.2-1.2-1.2zM22 7.8H9.4c-.6 0-1.2.5-1.2 1.2s.5 1.2 1.2 1.2H22c.6 0 1.2-.5 1.2-1.2s-.6-1.2-1.2-1.2z"/>

Nit: You can remove the class and move the fill property to an attribute of the <path> I think

::: browser/extensions/formautofill/locale/en-US/formautofill.properties:9
(Diff revision 8)
> +saveAddressMainLabel = Don't Use Autofill
> +saveAddressSecondaryLabel = Use Autofill

Please remove the unused strings

::: browser/extensions/formautofill/locale/en-US/formautofill.properties:12
(Diff revision 8)
>  savedProfiles = Saved Profiles…
> +saveAddressMessage = Firefox now saves your form data to help you fill out forms faster!
> +saveAddressMainLabel = Don't Use Autofill
> +saveAddressSecondaryLabel = Use Autofill
> +viewAutofillOptions = View Form Autofill options...
> +openAutofillMessagePanel = Open autofill message panel

Nit: Open Form Autofill message panel

This should really be part of the spec and reviewed by Michelle. I think Form Autofill should be capitalized but I'm not sure.
Attachment #8869372 - Flags: review?(MattN+bmo)
Comment on attachment 8869372 [details]
Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger.

https://reviewboard.mozilla.org/r/141022/#review150602

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:80
(Diff revision 8)
> +  _createActions(mainActionParams, secondaryActionParams, resolve) {
> +    if (!mainActionParams) {
> +      return [null, null];
> +    }

I guess we don't really need this method now and it doesn't get tested. It's probably okay to land now.
Comment on attachment 8871636 [details]
Bug 1303510 - Part 3: Add mochitest-plain for saving submitted form.

https://reviewboard.mozilla.org/r/143146/#review150644

::: browser/extensions/formautofill/test/mochitest/test_address_submission.html:54
(Diff revision 3)
> +    <p><label>organization: <input id="organization" name="organization" autocomplete="organization" type="text"></label></p>
> +    <p><label>streetAddress: <input id="street-address" name="street-address" autocomplete="street-address" type="text"></label></p>
> +    <p><label>tel: <input id="tel" name="tel" autocomplete="tel" type="text"></label></p>
> +    <p><label>country: <input id="country" name="country" autocomplete="country" type="text"></label></p>
> +    <p><input type="submit"></p>
> +    <p><button type="reset">Reset</button></p>

Please remove any unused attributes/elements as they make the test harder to read.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

2 years ago
mozreview-review-reply
Comment on attachment 8869372 [details]
Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger.

https://reviewboard.mozilla.org/r/141022/#review150498

> Doesn't this have to be during "showing" instead of "shown" in order to avoid a flicker?

I tried to append the content while showing before but sadly the notification element seems not appended to doc... the notification element is reachable only when the state is shown :/

> Double-check if SVG files are supposed to have a license

Seems like not all the svg assets have license... Thanks for the reminder
Comment on attachment 8869372 [details]
Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger.

https://reviewboard.mozilla.org/r/141022/#review151370

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:42
(Diff revision 9)
> +  /* TODO: Implement update in bug 1303515
> +  update: {
> +  },
> +  */

It would be better to just omit this.

::: browser/extensions/formautofill/FormAutofillParent.jsm:278
(Diff revision 9)
>      if (address.guid) {
>        // TODO: Show update doorhanger(bug 1303513) and set probe(bug 990200)
> -      // if (!profileStorage.addresses.mergeIfPossible(address.guid, address.record)) {
> +      // if (!this.profileStorage.addresses.mergeIfPossible(address.guid, address.record)) {
>        // }
>      } else {
> -      // TODO: Add first time use probe(bug 990199) and doorhanger(bug 1303510)
> +      this.profileStorage.addresses.add(address.record);

IIUC, this should be a mergeIfPossible otherwise we will save duplicates of manually created profiles.
Attachment #8869372 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8871636 [details]
Bug 1303510 - Part 3: Add mochitest-plain for saving submitted form.

https://reviewboard.mozilla.org/r/143146/#review151380
Attachment #8871636 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Whiteboard: [form autofill:M2][ETA:612] → [form autofill:M3][ETA:612]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

2 years ago
Thanks for all the reviews! Although the UX spec might be changed later, we could still apply these changes in bug 1303515
Blocks: 1303515
Keywords: checkin-needed

Comment 45

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e406ff2a2f82 -d 99fe8edb8296: rebasing 400974:e406ff2a2f82 "Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger. r=MattN"
merging browser/app/profile/firefox.js
warning: conflicts while merging browser/app/profile/firefox.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 49

2 years ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df527bcc28dc
Part 1: Implement doorhanger helper and parameter for first time use doorhanger. r=MattN
https://hg.mozilla.org/integration/autoland/rev/0d885ad1419d
Part 2: Add browser test for doorhanger part, r=MattN
https://hg.mozilla.org/integration/autoland/rev/19bc87470994
Part 3: Add mochitest-plain for saving submitted form. r=MattN
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=105770765&repo=autoland
Flags: needinfo?(schung)

Comment 51

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a12026007a59
Backed out changeset 19bc87470994 
https://hg.mozilla.org/integration/autoland/rev/5eeca8c89ac2
Backed out changeset 0d885ad1419d 
https://hg.mozilla.org/integration/autoland/rev/7fc5e5d91891
Backed out changeset df527bcc28dc for test failures in browser_first_time_use_doorhanger.js
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8869372 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8870765 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 60

2 years ago
Comment on attachment 8876455 [details]
Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger.

Hi Matt, this review was cleared accidentally but I didn't change code(only an intermittent error fixing) after your review. So I will redirect the review to Luke for meeting the 612 deadline.
Flags: needinfo?(schung)
Attachment #8876455 - Flags: review?(MattN+bmo) → review?(lchang)
(Assignee)

Updated

2 years ago
Attachment #8876456 - Flags: review?(MattN+bmo) → review?(lchang)
(Reporter)

Comment 61

2 years ago
mozreview-review
Comment on attachment 8876455 [details]
Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger.

https://reviewboard.mozilla.org/r/147792/#review152296
Attachment #8876455 - Flags: review?(lchang) → review+
(Reporter)

Comment 62

2 years ago
mozreview-review
Comment on attachment 8876456 [details]
Bug 1303510 - Part 2: Add browser test for doorhanger part,

https://reviewboard.mozilla.org/r/147794/#review152298
Attachment #8876456 - Flags: review?(lchang) → review+
(Reporter)

Comment 63

2 years ago
Carry over Matt's r+.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 67

2 years ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24c1512f3991
Part 1: Implement doorhanger helper and parameter for first time use doorhanger. r=lchang
https://hg.mozilla.org/integration/autoland/rev/5e5c3a085c86
Part 2: Add browser test for doorhanger part, r=lchang
https://hg.mozilla.org/integration/autoland/rev/b9b8f73f50a1
Part 3: Add mochitest-plain for saving submitted form. r=MattN
Keywords: checkin-needed

Comment 68

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/24c1512f3991
https://hg.mozilla.org/mozilla-central/rev/5e5c3a085c86
https://hg.mozilla.org/mozilla-central/rev/b9b8f73f50a1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

2 years ago
Blocks: 1374508
(Assignee)

Updated

2 years ago
Blocks: 1397115
You need to log in before you can comment on or make changes to this bug.