Closed Bug 691849 Opened 8 years ago Closed 4 years ago

Replace Sync notification bars with standard notifications

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox45 --- fixed

People

(Reporter: dholbert, Assigned: pablo_lm1)

References

Details

(Whiteboard: [good first bug][polish])

Attachments

(3 files, 5 obsolete files)

Attached image screenshot
STR:
 1. Try to sync right now, triggering "server maintenance" notification

ACTUAL RESULTS:
The notification is really wide, and it effectively sets a large "minimum width" for Firefox Window.  If I shrink Firefox to be skinnier than that UI gets cut off.

As soon as I close the notification bar, Firefox UI goes back to normal.

See attached screenshots.
Yeah, that's some FAIL. Thanks for reporting.

(Also, two periods? WTF?)
Whiteboard: [good-first-bug][polish]
Whiteboard: [good-first-bug][polish] → [good first bug][polish]
This is still an issue, FWIW.  I encountered this again today during the Sync server-maintenance downtime[1].

If my browser is skinnier than ~1300 px, the searchbar / scrollbar / etc. get clipped and are inaccessible.

[1] https://twitter.com/#!/mozservices/status/162268149788573696
I encounter something similar with a NoScript-provided notification at the TOP of the content region but, starting in 17.0a (current Aurora), it instead forces the actual OS-level browser window to resize.

(And since it's NoScript's ABE reporting that it blocked a request for a URL with a giant query parameter, the window ends up about five times as wide as my 2560x1024 desktop)

My bug 788234 may be a dupe of this.
Got the bug described in C#0 in French Nightly today (and we have very long strings in French).
Duplicate of this bug: 850683
This bug seems not to be resolved yet. I'm working on a fix, please assign it to me.
Thanks! Assigned.

(I'm also CC'ing markh, who works on Sync & might be able to help if you run into trouble, or might at least know who to direct questions to.)
Assignee: nobody → pablo_lm1
Attached patch Proposed fix (obsolete) — Splinter Review
Proposed fix attached
Attachment #8676468 - Flags: review?(gijskruitbosch+bugs)
FWIW, we have recently removed all such error bars from the product, except for ones related to Sync migration - so while this patch may be fine, it's not going to have any actual affect. I didn't steal review from Gijs as I'm not familiar with "overflow-x: -moz-hidden-unscrollable;", but even notwithstanding the removal of most bars, if it would solve the problem I'd be fine with this landing.
(In reply to Mark Hammond [:markh] from comment #10)
> FWIW, we have recently removed all such error bars from the product, except
> for ones related to Sync migration - so while this patch may be fine, it's
> not going to have any actual affect. I didn't steal review from Gijs as I'm
> not familiar with "overflow-x: -moz-hidden-unscrollable;", but even
> notwithstanding the removal of most bars, if it would solve the problem I'd
> be fine with this landing.

Can we just use the default bottom notification box for sync instead of this one, if there aren't multiple notifications anymore? That would make this a code removal good-first-bug... :-)

(it'd also solve some other styling oddities that are now cropping up for the sync bars on Windows because of bug 1162635)
Flags: needinfo?(markh)
(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Mark Hammond [:markh] from comment #10)
> > FWIW, we have recently removed all such error bars from the product, except
> > for ones related to Sync migration - so while this patch may be fine, it's
> > not going to have any actual affect. I didn't steal review from Gijs as I'm
> > not familiar with "overflow-x: -moz-hidden-unscrollable;", but even
> > notwithstanding the removal of most bars, if it would solve the problem I'd
> > be fine with this landing.
> 
> Can we just use the default bottom notification box for sync instead of this
> one, if there aren't multiple notifications anymore? That would make this a
> code removal good-first-bug... :-)
> 
> (it'd also solve some other styling oddities that are now cropping up for
> the sync bars on Windows because of bug 1162635)

If we do please link me the bug so I can do it, since I'm familiar with those files already. 

If I got it right, the only difference is that the sync notification box holds multiple notifications at once ordered by priority, and the global one shows one at a time.
(In reply to pablo_lm1 from comment #12)
> (In reply to :Gijs Kruitbosch from comment #11)
> > (In reply to Mark Hammond [:markh] from comment #10)
> > > FWIW, we have recently removed all such error bars from the product, except
> > > for ones related to Sync migration - so while this patch may be fine, it's
> > > not going to have any actual affect. I didn't steal review from Gijs as I'm
> > > not familiar with "overflow-x: -moz-hidden-unscrollable;", but even
> > > notwithstanding the removal of most bars, if it would solve the problem I'd
> > > be fine with this landing.
> > 
> > Can we just use the default bottom notification box for sync instead of this
> > one, if there aren't multiple notifications anymore? That would make this a
> > code removal good-first-bug... :-)
> > 
> > (it'd also solve some other styling oddities that are now cropping up for
> > the sync bars on Windows because of bug 1162635)
> 
> If we do please link me the bug so I can do it, since I'm familiar with
> those files already.

I think we can do it here, if we do it... :-)
 
> If I got it right, the only difference is that the sync notification box
> holds multiple notifications at once ordered by priority, and the global one
> shows one at a time.

Yes, but it sounds like there's only 1 notification that will make it into this box, at which point I don't really see the point of having the separate box for them...
(In reply to :Gijs Kruitbosch from comment #13)

> Yes, but it sounds like there's only 1 notification that will make it into
> this box, at which point I don't really see the point of having the separate
> box for them...

Yep, that's correct. We can probably remove services/sync/modules/notifications.js and browser/base/content/sync/notification.xml at the same time as that remaining notification relies on almost none of those sync-specific semantics.
Flags: needinfo?(markh)
See also bug 1205928. I was speaking as though that had already landed as it reduces use of the notifications significantly. Sadly it just bounced (for trivial reasons; I'll re-land it tomorrow) but you should see that patch there if you plan on working on this in the next day or so.
(In reply to Mark Hammond [:markh] from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> 
> > Yes, but it sounds like there's only 1 notification that will make it into
> > this box, at which point I don't really see the point of having the separate
> > box for them...
> 
> Yep, that's correct. We can probably remove
> services/sync/modules/notifications.js and
> browser/base/content/sync/notification.xml at the same time as that
> remaining notification relies on almost none of those sync-specific
> semantics.

Just removing those files and related code will be enough? Where is the remaining notification handled?
(In reply to pablo_lm1 from comment #16)
> Just removing those files and related code will be enough? Where is the
> remaining notification handled?

After that patch, there should remain one caller to Weave.Notification() in browser-fxaccounts.js, and that notification is fairly simple - text, a link and a button. You'll need to replace that caller with something that uses the standard notification box (eg, see calls to .addNotification() in nsBrowserGlue.js). Then you'll be able to remove those files (there'll be a moz.build file in the same directory that will need the filename removed). There are also a few tests that will need changing, which is a bridge we can cross later :)
Comment on attachment 8676468 [details] [diff] [review]
Proposed fix

(In reply to Mark Hammond [:markh] from comment #17)
> (In reply to pablo_lm1 from comment #16)
> > Just removing those files and related code will be enough? Where is the
> > remaining notification handled?
> 
> After that patch, there should remain one caller to Weave.Notification() in
> browser-fxaccounts.js, and that notification is fairly simple - text, a link
> and a button. You'll need to replace that caller with something that uses
> the standard notification box (eg, see calls to .addNotification() in
> nsBrowserGlue.js). Then you'll be able to remove those files (there'll be a
> moz.build file in the same directory that will need the filename removed).
> There are also a few tests that will need changing, which is a bridge we can
> cross later :)

Pablo, is this something you'd be comfortable doing? I know it's a bit more work than this originally looked like. Let me know if you need more help. :-)

(clearing the review request on this patch for now)
Flags: needinfo?(pablo_lm1)
Attachment #8676468 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #18)
> Comment on attachment 8676468 [details] [diff] [review]
> Proposed fix
> 
> (In reply to Mark Hammond [:markh] from comment #17)
> > (In reply to pablo_lm1 from comment #16)
> > > Just removing those files and related code will be enough? Where is the
> > > remaining notification handled?
> > 
> > After that patch, there should remain one caller to Weave.Notification() in
> > browser-fxaccounts.js, and that notification is fairly simple - text, a link
> > and a button. You'll need to replace that caller with something that uses
> > the standard notification box (eg, see calls to .addNotification() in
> > nsBrowserGlue.js). Then you'll be able to remove those files (there'll be a
> > moz.build file in the same directory that will need the filename removed).
> > There are also a few tests that will need changing, which is a bridge we can
> > cross later :)
> 
> Pablo, is this something you'd be comfortable doing? I know it's a bit more
> work than this originally looked like. Let me know if you need more help. :-)
> 
> (clearing the review request on this patch for now)

Yes, I will get to it, no problem :)

I will let you know if I run into trouble
Flags: needinfo?(pablo_lm1)
Attached patch notifications.patch (obsolete) — Splinter Review
My first attempt at doing the requested changes, uploading the patch for you to see, please review.

It should be okay, it's based on the usage of notifications in other files, but I have had trouble testing it because I'm not sure how to trigger the migration notification.
Attachment #8677476 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8677476 [details] [diff] [review]
notifications.patch

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

Great! This is looking really good already. I have a few notes about the usage of the notification bar APIs, which are below. I don't know the sync code amazingly well, so when you address my feedback, can you request your next review from :markh ?

::: browser/base/content/browser-fxaccounts.js
@@ +94,5 @@
>    },
>  
>    init: function () {
> +    XPCOMUtils.defineLazyModuleGetter(this, "RecentWindow",
> +                                      "resource:///modules/RecentWindow.jsm");

Nit: these lines aren't necessary, the recentwindow module is loaded in the main window, so you should be able to just use it.

@@ +439,5 @@
> +        let node = nb.appendNotification(msg,
> +                                        this.SYNC_MIGRATION_NOTIFICATION_TITLE,
> +                                        undefined,
> +                                        nb.PRIORITY_WARNING_LOW,
> +                                        buttons);

Right, so, instead of this approach, where you add the link afterwards, you can create a DOM Fragment that you pass as the first argument (I added support for this not super-long ago, so there isn't much sample code on how to use it, sadly). Something like this:

let fragment = document.createDocumentFragment();
let msgNode = document.createTextNode(msg);
fragment.appendChild(msgNode);
if (learnMoreLink) {
  let link = document.createElement("label");
  link.className = "text-link";
  link.setAttribute("value", learnMoreLink.text);
  link.href = learnMoreLink.href;
  fragment.appendChild(link);
}

and then pass fragment as the first argument to appendNotification. I believe that should work.

::: browser/base/content/browser-syncui.js
@@ -407,5 @@
>      }
>    }),
>  
>    clearError: function SUI_clearError(errorString) {
> -    Weave.Notifications.removeAll(errorString);

Mark will know for sure, but I *think* this makes sense - but it looks like it might break some tests... did you run any of the tests locally? Don't worry if you didn't... there might just be more stuff to remove. We can figure that out by doing a trypush.

::: browser/base/jar.mn
@@ -131,5 @@
>          content/browser/sync/setup.js                 (content/sync/setup.js)
>          content/browser/sync/genericChange.xul        (content/sync/genericChange.xul)
>          content/browser/sync/genericChange.js         (content/sync/genericChange.js)
>          content/browser/sync/key.xhtml                (content/sync/key.xhtml)
> -        content/browser/sync/notification.xml         (content/sync/notification.xml)

I'm guessing notifications.js also needs removing from a jar.mn or makefile?
Attachment #8677476 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch notifications.patch (obsolete) — Splinter Review
New patch with the feedback from Gijs, I haven't run the tests yet.
Attachment #8677507 - Flags: review?(markh)
Comment on attachment 8677507 [details] [diff] [review]
notifications.patch

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

This is looking good. As mentioned it will need to be rebased in a day or so as bug 1205928 makes its way to mozilla central.

Re tests, browser/base/content/test/general/browser_fxa_migrate.js will certainly need to be updated - you can run that locally with |./mach test browser/base/content/test/general/browser_fxa_migrate.js|. Similarly, check all tests in services/sync - |./mach test services/sync| should run them.

Can you please update the patch once you've done that rebase and I'll test it locally and push it to try, but nice work so far! :)

(Also, FYI, "review" requests are typically made when you think it is ready to push while "feedback" requests are for when it is still being worked on but feedback is desired. Accordingly, I've cleared the review request but given feedback+.)

::: browser/base/content/browser-fxaccounts.js
@@ +93,5 @@
>      return fm.activeWindow == window;
>    },
>  
>    init: function () {
> +

nit: no need for this additional blank line.

@@ +379,5 @@
>      this.panelUIFooter.setAttribute("fxastatus", status);
>    }),
>  
>    updateMigrationNotification: Task.async(function* () {
> +    let win = RecentWindow.getMostRecentBrowserWindow();

we should just be able to use |window| here - browser-fxaccounts is loaded as part of browser.js. If multiple windows are open, each of those windows should end up reaching this code, so there should be no need to find windows at all.

@@ +420,5 @@
>            upgradeLabel = this.strings.GetStringFromName("upgradeToFxA.label");
>            upgradeAccessKey = this.strings.GetStringFromName("upgradeToFxA.accessKey");
>            learnMoreLink = this.fxaMigrator.learnMoreLink;
>          }
> +

nit: a remove one of these blank lines.

@@ +431,1 @@
>                this._expectingNotifyClose = true;

sadly this is going to need to be rebased soon, but hopefully that will not be too painful.
Attachment #8677507 - Flags: review?(markh) → feedback+
Summary: Text in Sync error / notification bar doesn't wrap; long messages break Firefox UI → Replace Sync notification bars with standard notifications
New attempt at the patch that replaces Sync notification bars with standard notifications, improved as per Mark's feedback and rebased with the latest version of the codebase. Tests updated and passing.

Please tell me what you think.
Attachment #8676468 - Attachment is obsolete: true
Attachment #8677476 - Attachment is obsolete: true
Attachment #8677507 - Attachment is obsolete: true
Attachment #8678225 - Flags: feedback?(markh)
Comment on attachment 8678225 [details] [diff] [review]
Patch improved as per Mark's feedback, rebased with the latest version of the codebase.

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

Looks good to me, thanks! I've pushed your version of the patch to https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3ed2819617a. If that looks green, please make the requested changes and request review and we can get this landed :)

::: browser/base/content/browser-syncui.js
@@ +384,5 @@
>      }
>    }),
>  
>    clearError: function SUI_clearError(errorString) {
> +    let n = nb.getNotificationWithValue(errorString);

It looks like you are missing the decl of |nb| here - but let's just remove clearError() too. That will mean you can kill the "weave:ui:clear-error" and "weave:service:start-over" notifications (both from observe() and the list of observers and kill onStartOver() too.
Attachment #8678225 - Flags: feedback?(markh) → feedback+
(In reply to Mark Hammond [:markh] from comment #25)
> Comment on attachment 8678225 [details] [diff] [review]
> Patch improved as per Mark's feedback, rebased with the latest version of
> the codebase.
> 
> Review of attachment 8678225 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, thanks! I've pushed your version of the patch to
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3ed2819617a. If
> that looks green, please make the requested changes and request review and
> we can get this landed :)
> 
> ::: browser/base/content/browser-syncui.js
> @@ +384,5 @@
> >      }
> >    }),
> >  
> >    clearError: function SUI_clearError(errorString) {
> > +    let n = nb.getNotificationWithValue(errorString);
> 
> It looks like you are missing the decl of |nb| here - but let's just remove
> clearError() too. That will mean you can kill the "weave:ui:clear-error" and
> "weave:service:start-over" notifications (both from observe() and the list
> of observers and kill onStartOver() too.

Excuse me but, how does the treeherder thing work? I have a patch with the requested changes ready, should I submit it?

Thank you!
Flags: needinfo?(markh)
I already pushed the patch to https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3ed2819617a - if you look at the orange there, you will see it failed due to |nb| not being defined, as I noticed in my comments.  So an updated version of the patch will need to be ran again.

Info on pushing to the try server can be found at https://wiki.mozilla.org/ReleaseEngineering/TryServer, but you will need level 1 hg access described at https://www.mozilla.org/en-US/about/governance/policies/commit/. If you decide to get level 1 access, you should CC me on the bug you open and I'll vouch for you, but it will probably take a few days to happen.
Flags: needinfo?(markh)
Added the changes you requested. I'm not requesting review yet since I dont know if it will pass the try server.

Thanks!
Attachment #8678448 - Flags: feedback?(markh)
Pablo, I see you now have level 1 access but I also know if can be complicated to setup, so please let me know if you need help pushing your patch to try, or if you would like me to do it for you. Also, feel free to find me on IRC as :markh
Flags: needinfo?(pablo_lm1)
(In reply to Mark Hammond [:markh] from comment #29)
> Pablo, I see you now have level 1 access but I also know if can be
> complicated to setup, so please let me know if you need help pushing your
> patch to try, or if you would like me to do it for you. Also, feel free to
> find me on IRC as :markh

Thank you a lot!

I tried pushing to the try server myself and I got this link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95ff14b1103f

I'm not sure I understand the results though
Flags: needinfo?(pablo_lm1)
(In reply to Pablo Almenar from comment #30)
> (In reply to Mark Hammond [:markh] from comment #29)
> > Pablo, I see you now have level 1 access but I also know if can be
> > complicated to setup, so please let me know if you need help pushing your
> > patch to try, or if you would like me to do it for you. Also, feel free to
> > find me on IRC as :markh
> 
> Thank you a lot!
> 
> I tried pushing to the try server myself and I got this link:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=95ff14b1103f
> 
> I'm not sure I understand the results though

In brief, green letters are good, orange letters or bits with a red background is bad.

In particular, it looks like a lot of browser-chrome tests ("bc" followed by some number) are failing, because this file:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_syncui.js

imports notifications.js, which your patch removes. I'm not sure the test is still useful with the notifications gone. Mark, can you advise?
Flags: needinfo?(markh)
(In reply to :Gijs Kruitbosch from comment #31)
> (In reply to Pablo Almenar from comment #30)
> > (In reply to Mark Hammond [:markh] from comment #29)
> > > Pablo, I see you now have level 1 access but I also know if can be
> > > complicated to setup, so please let me know if you need help pushing your
> > > patch to try, or if you would like me to do it for you. Also, feel free to
> > > find me on IRC as :markh
> > 
> > Thank you a lot!
> > 
> > I tried pushing to the try server myself and I got this link:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=95ff14b1103f
> > 
> > I'm not sure I understand the results though
> 
> In brief, green letters are good, orange letters or bits with a red
> background is bad.

Oh, but to be clear: You can find out what happened to make things fail by clicking the bits that are orange/red, and then the error messages will appear in the bottom bit of the screen.

Sometimes tests fail "randomly" through no fault of your patch. The messages at the bottom include links to bugs that we have on file for those tests being randomly orange, if applicable. That will help you judge if the failure is related to your patch or not.

It looks like the "bc" failures that I mentioned in my previous comments are the only ones we need to worry about wrt your patch, everything else looks pretty good (ie the other bits of orange/red seem like they're not related to your patch).
oops, I forgot about that test :) That should:

* remove the import
* The test testSyncNeedsVerification() should have that 1 line (the first) referencing Notifications removed.
* The test testSyncLoginError should have the 3 lines with |Assert.equal(Notifications.notifications.length, 0, ...| removed.

And I think that test will pass. Pablo, it would be great if you can make those changes, and if the test then passes locally push a new try run, then update with bug with the updated patch and try URL.

Thanks for following through with this - killing sync-specific notifications has been on my list for some time :)
Flags: needinfo?(markh)
(In reply to Mark Hammond [:markh] from comment #33)
> oops, I forgot about that test :) That should:
> 
> * remove the import
> * The test testSyncNeedsVerification() should have that 1 line (the first)
> referencing Notifications removed.
> * The test testSyncLoginError should have the 3 lines with
> |Assert.equal(Notifications.notifications.length, 0, ...| removed.
> 
> And I think that test will pass. Pablo, it would be great if you can make
> those changes, and if the test then passes locally push a new try run, then
> update with bug with the updated patch and try URL.
> 
> Thanks for following through with this - killing sync-specific notifications
> has been on my list for some time :)

Thanks a lot ;)

Unfortunately, the test is failing for some reason. It fails when I remove "weave:ui:clear-error" and "weave:service:start-over" notification topics from browser/base/content/browser-syncui.js as you said. I tried replacing them in the test's relevant code for "weave:service:start-over:finish" and it's still failing for some reason.

In particular, it says that 'button' is null after it looks for it with 'let button = document.getElementById("sync-button");', which might be because the Sync service ins't properly initialized. Unfortunately I'm not sure how to tackle that so I'd really appreciate some help. Also, why do you suggest removing those topics if I may ask?
Flags: needinfo?(markh)
(In reply to Pablo Almenar from comment #34)

> Unfortunately, the test is failing for some reason.

I see the test timeout, which is due to me giving you bad advice re the test - I earlier asked you to remove "weave:ui:clear-error" but failed to notice the test uses that notification. The test is a bit sloppy in that it uses that notification as a general "please update the UI" notification.

So I changed that test to use different notifications - generally "weave:service:login:error" or "weave:service:login:finish" depending on what the test was simulating, and then noticed a different small problem - browser-syncui lists "weave:service:login:error" as a topic, but fails to handle it!

> I tried replacing
> them in the test's relevant code for "weave:service:start-over:finish" and
> it's still failing for some reason.

Yeah - the "start over" notification is sent when Sync does a full local reset. I'm re-thinking my suggestion of removing that topic entirely - I think we should probably keep it, but just have it call this.updateUI() - so clearError can still be removed.

> In particular, it says that 'button' is null after it looks for it with 'let
> button = document.getElementById("sync-button");', which might be because
> the Sync service ins't properly initialized.

I never saw that particular problem, just a timeout. If you see that again, can you please paste the logs here as it may point to a different problem.

> Also, why do you suggest
> removing those topics if I may ask?

Mainly to kill un-needed and complicated code - eg, "weave:ui:clear-error" is sent by exactly 1 place - https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/policies.js#595 - which is in response to a "weave:service:login:error" notification - and browser-syncui is already listening for ""weave:service:login:*" notifications (even though it listens but ignores the specific :error notification!) - the check against |.shouldReportError()| is now nearly dead because the answer is always "no" because we have killed all error UI. ie, at some point I hope to also kill the convoluted logic in policies.py)

So, long story short, can you please:
* re-instate the listener for the :startOver notification as mentioned above, but just have it call .updateUI() - just lump it with the other notifications that do exactly that.
* apply the attached patch - that fixes the test and fixes the missing handler for weave:service:login:error, and that test passes for me with that change (but I didn't run any other tests)
* make sure the test passes locally and re-push to try.
* if try passes, attach and request review.

Thanks heaps, and feel free to let me know if you'd prefer me to do any of the above.
Flags: needinfo?(markh)
Thanks a lot Mark!

Are you sure that's the right patch? I went ahead and did those things myself anyway. I'm submitting a new patch that fixes the test and the missing handler and re-instates the listener for the start-over notification.

This is the treeherder link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26228f9daee4 I see some timeouts but I think they arent related to my changes, so I'm requesting review, please let me know if I'm wrong.
Attachment #8678225 - Attachment is obsolete: true
Attachment #8678448 - Attachment is obsolete: true
Attachment #8678448 - Flags: feedback?(markh)
Attachment #8682438 - Flags: review?(markh)
(Meant to say this on IRC but you left before I got around to it, so bugspam it is: Pablo, I'm really impressed with how quickly you've taken this from a simple CSS fix to full-on patches with test adjustments and so on. Kudos!)
(In reply to :Gijs Kruitbosch from comment #37)
> (Meant to say this on IRC but you left before I got around to it, so bugspam
> it is: Pablo, I'm really impressed with how quickly you've taken this from a
> simple CSS fix to full-on patches with test adjustments and so on. Kudos!)

Thank you so much Gijs, this really means a lot to me ;)

You guys did great helping me out tbh, I wouldnt have gotten this far without you.
Comment on attachment 8682438 [details] [diff] [review]
More modifications

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

Perfect, thanks very much - and it appears congratulations are needed for your first patch :) To reflect more on what Gijs said, I'm super impressed with how well you have taken this task on and managed to navigate the code-base to get an excellent result with very little hand-holding. We need many more contributors like you, so please reach out to Gijs or myself on future bugs, or if you need help finding your next bug :)
Attachment #8682438 - Flags: review?(markh) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
(In reply to Mark Hammond [:markh] from comment #39)
> Comment on attachment 8682438 [details] [diff] [review]
> More modifications
> 
> Review of attachment 8682438 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Perfect, thanks very much - and it appears congratulations are needed for
> your first patch :) To reflect more on what Gijs said, I'm super impressed
> with how well you have taken this task on and managed to navigate the
> code-base to get an excellent result with very little hand-holding. We need
> many more contributors like you, so please reach out to Gijs or myself on
> future bugs, or if you need help finding your next bug :)

Thank you so much man :)

As I told Gijs, I couldn't have done it without you. I will reach out to you guys when I need some help or guidance, thanks a lot!

Is there something I have to do to get it checked in?
(In reply to Pablo Almenar from comment #40)
> Thank you so much man :)

My pleasure :)

> Is there something I have to do to get it checked in?

Nope, that "checkin-needed" keyword should cause someone to push it within the next day or so.

Thanks!
I tested the 1 remaining notification. Given setting up for that test is relatively complicated I don't think there's value in getting explicit QA on this.
Flags: qe-verify-
https://hg.mozilla.org/mozilla-central/rev/d30fb6c19789
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.